Merge pull request #594 from nlordell/fix-ecdsa-extraction

Fix ECDSA Signature Unwrapping

packages/server/src/helpers/iso/isoCrypto/unwrapEC2Signature.ts

@@ -1,36 +1,78 @@
 import { AsnParser, ECDSASigValue } from '../../../deps.ts';
+import { COSECRV } from '../../cose.ts';
 import { isoUint8Array } from '../index.ts';
 
 /**
  * In WebAuthn, EC2 signatures are wrapped in ASN.1 structure so we need to peel r and s apart.
  *
  * See https://www.w3.org/TR/webauthn-2/#sctn-signature-attestation-types
  */
-export function unwrapEC2Signature(signature: Uint8Array): Uint8Array {
+export function unwrapEC2Signature(signature: Uint8Array, crv: COSECRV): Uint8Array {
   const parsedSignature = AsnParser.parse(signature, ECDSASigValue);
-  let rBytes = new Uint8Array(parsedSignature.r);
-  let sBytes = new Uint8Array(parsedSignature.s);
+  const rBytes = new Uint8Array(parsedSignature.r);
+  const sBytes = new Uint8Array(parsedSignature.s);
 
-  if (shouldRemoveLeadingZero(rBytes)) {
-    rBytes = rBytes.slice(1);
-  }
-
-  if (shouldRemoveLeadingZero(sBytes)) {
-    sBytes = sBytes.slice(1);
-  }
+  const componentLength = getSignatureComponentLength(crv);
+  const rNormalizedBytes = toNormalizedBytes(rBytes, componentLength);
+  const sNormalizedBytes = toNormalizedBytes(sBytes, componentLength);
 
-  const finalSignature = isoUint8Array.concat([rBytes, sBytes]);
+  const finalSignature = isoUint8Array.concat([
+    rNormalizedBytes,
+    sNormalizedBytes,
+  ]);
 
   return finalSignature;
 }
 
 /**
- * Determine if the DER-specific `00` byte at the start of an ECDSA signature byte sequence
- * should be removed based on the following logic:
+ * The SubtleCrypto Web Crypto API expects ECDSA signatures with `r` and `s` values to be encoded
+ * to a specific length depending on the order of the curve. This function returns the expected
+ * byte-length for each of the `r` and `s` signature components.
+ *
+ * See <https://www.w3.org/TR/WebCryptoAPI/#ecdsa-operations>
+ */
+function getSignatureComponentLength(crv: COSECRV): number {
+  switch (crv) {
+    case COSECRV.P256:
+      return 32;
+    case COSECRV.P384:
+      return 48;
+    case COSECRV.P521:
+      return 66;
+    default:
+      throw new Error(`Unexpected COSE crv value of ${crv} (EC2)`);
+  }
+}
+
+/**
+ * Converts the ASN.1 integer representation to bytes of a specific length `n`.
  *
- * "If the leading byte is 0x0, and the the high order bit on the second byte is not set to 0,
- * then remove the leading 0x0 byte"
+ * DER encodes integers as big-endian byte arrays, with as small as possible representation and
+ * requires a leading `0` byte to disambiguate between negative and positive numbers. This means
+ * that `r` and `s` can potentially not be the expected byte-length that is needed by the
+ * SubtleCrypto Web Crypto API: if there are leading `0`s it can be shorter than expected, and if
+ * it has a leading `1` bit, it can be one byte longer.
+ *
+ * See <https://www.itu.int/rec/T-REC-X.690-202102-I/en>
+ * See <https://www.w3.org/TR/WebCryptoAPI/#ecdsa-operations>
  */
-function shouldRemoveLeadingZero(bytes: Uint8Array): boolean {
-  return bytes[0] === 0x0 && (bytes[1] & (1 << 7)) !== 0;
+function toNormalizedBytes(bytes: Uint8Array, componentLength: number): Uint8Array {
+  let normalizedBytes;
+  if (bytes.length < componentLength) {
+    // In case the bytes are shorter than expected, we need to pad it with leading `0`s.
+    normalizedBytes = new Uint8Array(componentLength);
+    normalizedBytes.set(bytes, componentLength - bytes.length);
+  } else if (bytes.length === componentLength) {
+    normalizedBytes = bytes;
+  } else if (bytes.length === componentLength + 1 && bytes[0] === 0 && (bytes[1] & 0x80) === 0x80) {
+    // The bytes contain a leading `0` to encode that the integer is positive. This leading `0`
+    // needs to be removed for compatibility with the SubtleCrypto Web Crypto API.
+    normalizedBytes = bytes.subarray(1);
+  } else {
+    throw new Error(
+      `invalid signature component length ${bytes.length} (expected ${componentLength})`,
+    );
+  }
+
+  return normalizedBytes;
 }

packages/server/src/helpers/iso/isoCrypto/verify.ts

@@ -2,6 +2,7 @@ import {
   COSEALG,
   COSEKEYS,
   COSEPublicKey,
+  isCOSECrv,
   isCOSEPublicKeyEC2,
   isCOSEPublicKeyOKP,
   isCOSEPublicKeyRSA,
@@ -23,7 +24,11 @@ export function verify(opts: {
   const { cosePublicKey, signature, data, shaHashOverride } = opts;
 
   if (isCOSEPublicKeyEC2(cosePublicKey)) {
-    const unwrappedSignature = unwrapEC2Signature(signature);
+    const crv = cosePublicKey.get(COSEKEYS.crv);
+    if (!isCOSECrv(crv)) {
+      throw new Error(`unknown COSE curve ${crv}`);
+    }
+    const unwrappedSignature = unwrapEC2Signature(signature, crv);
     return verifyEC2({
       cosePublicKey,
       signature: unwrappedSignature,

packages/server/src/helpers/iso/isoCrypto/verifyEC2.test.ts

@@ -0,0 +1,106 @@
+import { assert } from 'https://deno.land/std@0.198.0/assert/mod.ts';
+
+import { COSEALG, COSECRV, COSEKEYS, COSEKTY, COSEPublicKeyEC2 } from '../../cose.ts';
+import { verifyEC2 } from './verifyEC2.ts';
+import { unwrapEC2Signature } from './unwrapEC2Signature.ts';
+import { isoBase64URL } from '../index.ts';
+
+Deno.test(
+  'should verify a signature signed with an P-256 public key',
+  async () => {
+    const cosePublicKey: COSEPublicKeyEC2 = new Map();
+    cosePublicKey.set(COSEKEYS.kty, COSEKTY.EC2);
+    cosePublicKey.set(COSEKEYS.alg, COSEALG.ES256);
+    cosePublicKey.set(COSEKEYS.crv, COSECRV.P256);
+    cosePublicKey.set(
+      COSEKEYS.x,
+      isoBase64URL.toBuffer('_qRi-kwOVobsqJ_1GAHZYfC77QoIdsVFYkx2Mw20UM4'),
+    );
+    cosePublicKey.set(
+      COSEKEYS.y,
+      isoBase64URL.toBuffer('BXEathwyOK_uQRmlZ_m4wReHLujSXk_-e3-9co5B2MY'),
+    );
+
+    const data = isoBase64URL.toBuffer('Bt81jmu3ieajF4w1at8HmieVOTDymHd7xJguJCUsL-Q');
+    const signature = isoBase64URL.toBuffer(
+      'MEQCH1h_F7TPTMVh_kwb_ssjD0_2U77bbXazz2ux-P6khLQCIQCutHs9eCBkCIMP3yA9mmNRKEfFd-REmhGY2GbHozaC7w',
+    );
+
+    const verified = await verifyEC2({
+      cosePublicKey,
+      data,
+      signature: unwrapEC2Signature(signature, COSECRV.P256),
+    });
+
+    assert(verified);
+  },
+);
+
+Deno.test(
+  'should verify a signature signed with an P-384 public key',
+  async () => {
+    const cosePublicKey: COSEPublicKeyEC2 = new Map();
+    cosePublicKey.set(COSEKEYS.kty, COSEKTY.EC2);
+    cosePublicKey.set(COSEKEYS.alg, COSEALG.ES384);
+    cosePublicKey.set(COSEKEYS.crv, COSECRV.P384);
+    cosePublicKey.set(
+      COSEKEYS.x,
+      isoBase64URL.toBuffer('pm-0exykk1x0O72S9sm6fl-iXxFrGikjQHi1CgONIiEz_yDJdCPxN453qg6HLkOx'),
+    );
+    cosePublicKey.set(
+      COSEKEYS.y,
+      isoBase64URL.toBuffer('2B7yW7sgza8Sf7ifznQlGJqmJxgupkAevUqqOJTWaWBZiQ7sAf-TfAaNBukiz12K'),
+    );
+
+    const data = isoBase64URL.toBuffer('D7mI8UwWXv4rpfSQUNqtUXAhZEPbRLugmWclPpJ9m7c');
+    const signature = isoBase64URL.toBuffer(
+      'MGMCL3lZ2Rjxo5WcmTCdWyB6jTE9PVuduOR_AsJu956J9S_mFNbHP_-MbyWem4dfb5iqAjABJhTRltNl5Y0O4XC7YLNsYKq2WxYQ1HFOMGsr6oNkUPsX3UAr2zeeWL_Tp1VgHeM',
+    );
+
+    const verified = await verifyEC2({
+      cosePublicKey,
+      data,
+      signature: unwrapEC2Signature(signature, COSECRV.P384),
+    });
+
+    assert(verified);
+  },
+);
+
+Deno.test({
+  // This test is currently ignored, as Deno's implementation of `WebCrypto.subtle` API does not
+  // support the P-521 curve at the moment.
+  ignore: true,
+  name: 'should verify a signature signed with an P-521 public key',
+  async fn() {
+    const cosePublicKey: COSEPublicKeyEC2 = new Map();
+    cosePublicKey.set(COSEKEYS.kty, COSEKTY.EC2);
+    cosePublicKey.set(COSEKEYS.alg, COSEALG.ES512);
+    cosePublicKey.set(COSEKEYS.crv, COSECRV.P521);
+    cosePublicKey.set(
+      COSEKEYS.x,
+      isoBase64URL.toBuffer(
+        'AaLbnrCvCuQivbknRW50FjdqPQv4NRF9tHsN4QuVQ3sw8uSspd33o-NTBfjg5JzX9rnpbkKDigb6NugmrVjzNMNK',
+      ),
+    );
+    cosePublicKey.set(
+      COSEKEYS.y,
+      isoBase64URL.toBuffer(
+        'AE64axa8L8PkLX5Td0GaX79cLOW9E2-8-ObhL9XT_ih-1XxbGQcA5VhL1gI0xIQq5zYAxgZYey6PmbbqgtcUPRVt',
+      ),
+    );
+
+    const data = isoBase64URL.toBuffer('5p0h9RZTjLoBlnL2nY5pqOnhGy4q60NzbjDe2rVDR7o');
+    const signature = isoBase64URL.toBuffer(
+      'MIGHAkFRpbGknlgpETORypMprGBXMkJMfuqgJupy3NcgCOaJJdj3Voz74kV2pjPqkLNpuO9FqVtXeEsUw-jYsBHcMqHZhwJCAQ88uFDJS5g81XVBcLMIgf6ro-F-5jgRAmHx3CRVNGdk81MYbFJhT3hd2w9RdhT8qBG0zzRBXYAcHrKo0qJwQZot',
+    );
+
+    const verified = await verifyEC2({
+      cosePublicKey,
+      data,
+      signature: unwrapEC2Signature(signature, COSECRV.P521),
+    });
+
+    assert(verified);
+  },
+});