[PM-5347] Fix buggy P1363 to DER conversion (#7303)

* [PM-5347] feat: add tests

* fixed miscalculated lengths for R and S components by not by prepending an extra byte when the MSB of the first byte in a component is greater than or equal to 0x80

* renamed to correct format

* fixed issue when there is a leading zero and a negative

* removed comment

* Made test cases clearer and more informative, renamed joseToDer, removed alg parameter and defaulted to use ES256 algorithm

---------

Co-authored-by: Andreas Coroiu <andreas.coroiu@gmail.com>

libs/common/src/vault/services/fido2/ecdsa-utils.spec.ts

@@ -0,0 +1,83 @@
+import { p1363ToDer } from "./ecdsa-utils";
+
+describe("p1363ToDer", () => {
+  let r: Uint8Array;
+  let s: Uint8Array;
+
+  beforeEach(() => {
+    r = randomBytes(32);
+    s = randomBytes(32);
+  });
+
+  it("should convert P1336 to DER when 'R' is positive and 'S' is positive", () => {
+    r[0] = 0x14;
+    s[0] = 0x32;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(new Uint8Array([0x30, 0x44, 0x02, 0x20, ...r, 0x02, 0x20, ...s]));
+  });
+
+  it("should convert P1336 to DER when 'R' is negative and 'S' is negative", () => {
+    r[0] = 0x94;
+    s[0] = 0xaa;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(
+      new Uint8Array([0x30, 0x46, 0x02, 0x21, 0x00, ...r, 0x02, 0x21, 0x00, ...s]),
+    );
+  });
+
+  it("should convert P1336 to DER when 'R' is negative and 'S' is positive", () => {
+    r[0] = 0x94;
+    s[0] = 0x32;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(new Uint8Array([0x30, 0x45, 0x02, 0x21, 0x00, ...r, 0x02, 0x20, ...s]));
+  });
+
+  it("should convert P1336 to DER when 'R' is positive and 'S' is negative", () => {
+    r[0] = 0x32;
+    s[0] = 0x94;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(new Uint8Array([0x30, 0x45, 0x02, 0x20, ...r, 0x02, 0x21, 0x00, ...s]));
+  });
+
+  it("should convert P1336 to DER when 'R' has leading zero and is negative and 'S' is positive", () => {
+    r[0] = 0x00;
+    r[1] = 0x94;
+    s[0] = 0x32;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(
+      new Uint8Array([0x30, 0x44, 0x02, 0x20, 0x00, ...r.slice(1), 0x02, 0x20, ...s]),
+    );
+  });
+
+  it("should convert P1336 to DER when 'R' is positive and 'S' has leading zero and is negative ", () => {
+    r[0] = 0x32;
+    s[0] = 0x00;
+    s[1] = 0x94;
+    const signature = new Uint8Array([...r, ...s]);
+
+    const result = p1363ToDer(signature);
+
+    expect(result).toEqual(
+      new Uint8Array([0x30, 0x44, 0x02, 0x20, ...r, 0x02, 0x20, 0x00, ...s.slice(1)]),
+    );
+  });
+});
+
+function randomBytes(length: number): Uint8Array {
+  return new Uint8Array(Array.from({ length }, (_, k) => k % 255));
+}

libs/common/src/vault/services/fido2/ecdsa-utils.ts

@@ -52,21 +52,23 @@ const MAX_OCTET = 0x80,
   ENCODED_TAG_SEQ = TAG_SEQ | PRIMITIVE_BIT | (CLASS_UNIVERSAL << 6),
   ENCODED_TAG_INT = TAG_INT | (CLASS_UNIVERSAL << 6);
 
-function countPadding(buf: Uint8Array, start: number, stop: number) {
+// Counts leading zeros and determines if there's a need for 0x00 padding
+function countPadding(
+  buf: Uint8Array,
+  start: number,
+  end: number,
+): { padding: number; needs0x00: boolean } {
   let padding = 0;
-  while (start + padding < stop && buf[start + padding] === 0) {
+  while (start + padding < end && buf[start + padding] === 0) {
-    ++padding;
+    padding++;
   }
 
-  const needsSign = buf[start + padding] >= MAX_OCTET;
+  const needs0x00 = (buf[start + padding] & MAX_OCTET) === MAX_OCTET;
-  if (needsSign) {
+  return { padding, needs0x00 };
-    --padding;
-  }
-
-  return padding;
 }
 
-export function joseToDer(signature: Uint8Array, alg: Alg) {
+export function p1363ToDer(signature: Uint8Array) {
+  const alg = "ES256";
   const paramBytes = getParamBytesForAlg(alg);
 
   const signatureBytes = signature.length;
@@ -82,12 +84,20 @@ export function joseToDer(signature: Uint8Array, alg: Alg) {
     );
   }
 
-  const rPadding = countPadding(signature, 0, paramBytes);
+  const { padding: rPadding, needs0x00: rNeeds0x00 } = countPadding(signature, 0, paramBytes);
-  const sPadding = countPadding(signature, paramBytes, signature.length);
+  const { padding: sPadding, needs0x00: sNeeds0x00 } = countPadding(
-  const rLength = paramBytes - rPadding;
+    signature,
-  const sLength = paramBytes - sPadding;
+    paramBytes,
+    signature.length,
+  );
 
-  const rsBytes = 1 + 1 + rLength + 1 + 1 + sLength;
+  const rActualLength = paramBytes - rPadding;
+  const sActualLength = paramBytes - sPadding;
+
+  const rLength = rActualLength + (rNeeds0x00 ? 1 : 0);
+  const sLength = sActualLength + (sNeeds0x00 ? 1 : 0);
+
+  const rsBytes = 2 + rLength + 2 + sLength;
 
   const shortLength = rsBytes < MAX_OCTET;
 
@@ -101,24 +111,23 @@ export function joseToDer(signature: Uint8Array, alg: Alg) {
     dst[offset++] = MAX_OCTET | 1;
     dst[offset++] = rsBytes & 0xff;
   }
+
+  // Encoding 'R' component
   dst[offset++] = ENCODED_TAG_INT;
   dst[offset++] = rLength;
-  if (rPadding < 0) {
+  if (rNeeds0x00) {
     dst[offset++] = 0;
-    dst.set(signature.subarray(0, paramBytes), offset);
-    offset += paramBytes;
-  } else {
-    dst.set(signature.subarray(rPadding, paramBytes), offset);
-    offset += paramBytes;
   }
+  dst.set(signature.subarray(rPadding, rPadding + rActualLength), offset);
+  offset += rActualLength;
+
+  // Encoding 'S' component
   dst[offset++] = ENCODED_TAG_INT;
   dst[offset++] = sLength;
-  if (sPadding < 0) {
+  if (sNeeds0x00) {
     dst[offset++] = 0;
-    dst.set(signature.subarray(paramBytes), offset);
-  } else {
-    dst.set(signature.subarray(paramBytes + sPadding), offset);
   }
+  dst.set(signature.subarray(paramBytes + sPadding, paramBytes + sPadding + sActualLength), offset);
 
   return dst;
 }

libs/common/src/vault/services/fido2/fido2-authenticator.service.ts

@@ -21,7 +21,7 @@ import { CipherView } from "../../models/view/cipher.view";
 import { Fido2CredentialView } from "../../models/view/fido2-credential.view";
 
 import { CBOR } from "./cbor";
-import { joseToDer } from "./ecdsa-utils";
+import { p1363ToDer } from "./ecdsa-utils";
 import { Fido2Utils } from "./fido2-utils";
 import { guidToRawFormat, guidToStandardFormat } from "./guid-utils";
 
@@ -508,7 +508,7 @@ async function generateSignature(params: SignatureParams) {
     ...params.authData,
     ...Fido2Utils.bufferSourceToUint8Array(params.clientDataHash),
   ]);
-  const p1336_signature = new Uint8Array(
+  const p1363_signature = new Uint8Array(
     await crypto.subtle.sign(
       {
         name: "ECDSA",
@@ -519,7 +519,7 @@ async function generateSignature(params: SignatureParams) {
     ),
   );
 
-  const asn1Der_signature = joseToDer(p1336_signature, "ES256");
+  const asn1Der_signature = p1363ToDer(p1363_signature);
 
   return asn1Der_signature;
 }