Prevent signature malleability (#1839)

* prevent signature malleability * tidy & allow toggle * fix in xrpc-server * add changeset * add test vecotrs
bluesky-social · Nov 21, 2023 · e1b5f25 · e1b5f25
1 parent 59f70db
commit e1b5f25
Show file tree

Hide file tree

Showing 7 changed files with 156 additions and 6 deletions.
diff --git a/.changeset/modern-planets-change.md b/.changeset/modern-planets-change.md
@@ -0,0 +1,6 @@
+---
+'@atproto/crypto': minor
+'@atproto/xrpc-server': patch
+---
+
+Prevent signature malleability through DER-encoded signatures
diff --git a/interop-test-files/crypto/signature-fixtures.json b/interop-test-files/crypto/signature-fixtures.json
@@ -42,5 +42,27 @@
     "signatureBase64": "5WpdIuEUUfVUYaozsi8G0B3cWO09cgZbIIwg1t2YKdXYA67MYxYiTMAVfdnkDCMN9S5B3vHosRe07aORmoshoQ",
     "validSignature": false,
     "tags": ["high-s"]
+  },
+  {
+    "comment": "P-256 key and signature, with DER-encoded signature which is invalid in atproto",
+    "messageBase64": "oWVoZWxsb2V3b3JsZA",
+    "algorithm": "ES256",
+    "didDocSuite": "EcdsaSecp256r1VerificationKey2019",
+    "publicKeyDid": "did:key:zDnaeT6hL2RnTdUhAPLij1QBkhYZnmuKyM7puQLW1tkF4Zkt8",
+    "publicKeyMultibase": "ze8N2PPxnu19hmBQ58t5P3E9Yj6CqakJmTVCaKvf9Byq2",
+    "signatureBase64": "MEQCIFxYelWJ9lNcAVt+jK0y/T+DC/X4ohFZ+m8f9SEItkY1AiACX7eXz5sgtaRrz/SdPR8kprnbHMQVde0T2R8yOTBweA",
+    "validSignature": false,
+    "tags": ["der-encoded"]
+  },
+  {
+    "comment": "K-256 key and signature, with DER-encoded signature which is invalid in atproto",
+    "messageBase64": "oWVoZWxsb2V3b3JsZA",
+    "algorithm": "ES256K",
+    "didDocSuite": "EcdsaSecp256k1VerificationKey2019",
+    "publicKeyDid": "did:key:zQ3shnriYMXc8wvkbJqfNWh5GXn2bVAeqTC92YuNbek4npqGF",
+    "publicKeyMultibase": "z22uZXWP8fdHXi4jyx8cCDiBf9qQTsAe6VcycoMQPfcMQX",
+    "signatureBase64": "MEUCIQCWumUqJqOCqInXF7AzhIRg2MhwRz2rWZcOEsOjPmNItgIgXJH7RnqfYY6M0eg33wU0sFYDlprwdOcpRn78Sz5ePgk",
+    "validSignature": false,
+    "tags": ["der-encoded"]
   }
 ]
diff --git a/packages/crypto/src/p256/operations.ts b/packages/crypto/src/p256/operations.ts
@@ -1,5 +1,6 @@
 import { p256 } from '@noble/curves/p256'
 import { sha256 } from '@noble/hashes/sha256'
+import * as ui8 from 'uint8arrays'
 import { P256_JWT_ALG } from '../const'
 import { parseDidKey } from '../did'
 import { VerifyOptions } from '../types'
@@ -23,8 +24,23 @@ export const verifySig = async (
   sig: Uint8Array,
   opts?: VerifyOptions,
 ): Promise<boolean> => {
+  const allowMalleable = opts?.allowMalleableSig ?? false
   const msgHash = await sha256(data)
+  // parse as compact sig to prevent signature malleability
+  // library supports sigs in 2 different formats: https://github.com/paulmillr/noble-curves/issues/99
+  if (!allowMalleable && !isCompactFormat(sig)) {
+    return false
+  }
   return p256.verify(sig, msgHash, publicKey, {
-    lowS: opts?.lowS ?? true,
+    lowS: !allowMalleable,
   })
 }
+
+export const isCompactFormat = (sig: Uint8Array) => {
+  try {
+    const parsed = p256.Signature.fromCompact(sig)
+    return ui8.equals(parsed.toCompactRawBytes(), sig)
+  } catch {
+    return false
+  }
+}
diff --git a/packages/crypto/src/secp256k1/operations.ts b/packages/crypto/src/secp256k1/operations.ts
@@ -1,5 +1,6 @@
 import { secp256k1 as k256 } from '@noble/curves/secp256k1'
 import { sha256 } from '@noble/hashes/sha256'
+import * as ui8 from 'uint8arrays'
 import { SECP256K1_JWT_ALG } from '../const'
 import { parseDidKey } from '../did'
 import { VerifyOptions } from '../types'
@@ -23,8 +24,23 @@ export const verifySig = async (
   sig: Uint8Array,
   opts?: VerifyOptions,
 ): Promise<boolean> => {
+  const allowMalleable = opts?.allowMalleableSig ?? false
   const msgHash = await sha256(data)
+  // parse as compact sig to prevent signature malleability
+  // library supports sigs in 2 different formats: https://github.com/paulmillr/noble-curves/issues/99
+  if (!allowMalleable && !isCompactFormat(sig)) {
+    return false
+  }
   return k256.verify(sig, msgHash, publicKey, {
-    lowS: opts?.lowS ?? true,
+    lowS: !allowMalleable,
   })
 }
+
+export const isCompactFormat = (sig: Uint8Array) => {
+  try {
+    const parsed = k256.Signature.fromCompact(sig)
+    return ui8.equals(parsed.toCompactRawBytes(), sig)
+  } catch {
+    return false
+  }
+}
diff --git a/packages/crypto/src/types.ts b/packages/crypto/src/types.ts
@@ -21,5 +21,5 @@ export type DidKeyPlugin = {
 }
 
 export type VerifyOptions = {
-  lowS?: boolean
+  allowMalleableSig?: boolean
 }
diff --git a/packages/crypto/tests/signatures.test.ts b/packages/crypto/tests/signatures.test.ts
@@ -78,7 +78,7 @@ describe('signatures', () => {
           keyBytes,
           messageBytes,
           signatureBytes,
-          { lowS: false },
+          { allowMalleableSig: true },
         )
         expect(verified).toEqual(true)
         expect(vector.validSignature).toEqual(false) // otherwise would fail per low-s requirement
@@ -87,7 +87,46 @@ describe('signatures', () => {
           keyBytes,
           messageBytes,
           signatureBytes,
-          { lowS: false },
+          { allowMalleableSig: true },
+        )
+        expect(verified).toEqual(true)
+        expect(vector.validSignature).toEqual(false) // otherwise would fail per low-s requirement
+      } else {
+        throw new Error('Unsupported test vector')
+      }
+    }
+  })
+
+  it('verifies der-encoded signatures with explicit option', async () => {
+    const DERVectors = vectors.filter((vec) => vec.tags.includes('der-encoded'))
+    expect(DERVectors.length).toBeGreaterThanOrEqual(2)
+    for (const vector of DERVectors) {
+      const messageBytes = uint8arrays.fromString(
+        vector.messageBase64,
+        'base64',
+      )
+      const signatureBytes = uint8arrays.fromString(
+        vector.signatureBase64,
+        'base64',
+      )
+      const keyBytes = multibaseToBytes(vector.publicKeyMultibase)
+      const didKey = parseDidKey(vector.publicKeyDid)
+      expect(uint8arrays.equals(keyBytes, didKey.keyBytes))
+      if (vector.algorithm === P256_JWT_ALG) {
+        const verified = await p256.verifySig(
+          keyBytes,
+          messageBytes,
+          signatureBytes,
+          { allowMalleableSig: true },
+        )
+        expect(verified).toEqual(true)
+        expect(vector.validSignature).toEqual(false) // otherwise would fail per low-s requirement
+      } else if (vector.algorithm === SECP256K1_JWT_ALG) {
+        const verified = await secp.verifySig(
+          keyBytes,
+          messageBytes,
+          signatureBytes,
+          { allowMalleableSig: true },
         )
         expect(verified).toEqual(true)
         expect(vector.validSignature).toEqual(false) // otherwise would fail per low-s requirement
@@ -168,6 +207,39 @@ async function generateTestVectors(): Promise<TestVector[]> {
       validSignature: false,
       tags: ['high-s'],
     },
+    // these vectors test to ensure we don't allow der-encoded signatures
+    {
+      messageBase64,
+      algorithm: P256_JWT_ALG, // "ES256" / ecdsa p-256
+      publicKeyDid: p256Key.did(),
+      publicKeyMultibase: bytesToMultibase(
+        p256Key.publicKeyBytes(),
+        'base58btc',
+      ),
+      signatureBase64: await makeDerEncodedSig(
+        messageBytes,
+        await p256Key.export(),
+        P256_JWT_ALG,
+      ),
+      validSignature: false,
+      tags: ['der-encoded'],
+    },
+    {
+      messageBase64,
+      algorithm: SECP256K1_JWT_ALG, // "ES256K" / secp256k
+      publicKeyDid: secpKey.did(),
+      publicKeyMultibase: bytesToMultibase(
+        secpKey.publicKeyBytes(),
+        'base58btc',
+      ),
+      signatureBase64: await makeDerEncodedSig(
+        messageBytes,
+        await secpKey.export(),
+        SECP256K1_JWT_ALG,
+      ),
+      validSignature: false,
+      tags: ['der-encoded'],
+    },
   ]
 }
 
@@ -195,6 +267,24 @@ async function makeHighSSig(
   return sig
 }
 
+async function makeDerEncodedSig(
+  msgBytes: Uint8Array,
+  keyBytes: Uint8Array,
+  alg: string,
+): Promise<string> {
+  const hash = await sha256(msgBytes)
+
+  let sig: string
+  if (alg === SECP256K1_JWT_ALG) {
+    const attempt = await nobleK256.sign(hash, keyBytes, { lowS: true })
+    sig = uint8arrays.toString(attempt.toDERRawBytes(), 'base64')
+  } else {
+    const attempt = await nobleP256.sign(hash, keyBytes, { lowS: true })
+    sig = uint8arrays.toString(attempt.toDERRawBytes(), 'base64')
+  }
+  return sig
+}
+
 type TestVector = {
   algorithm: string
   publicKeyDid: string

diff --git a/packages/xrpc-server/src/auth.ts b/packages/xrpc-server/src/auth.ts
@@ -71,7 +71,7 @@ export const verifyJwt = async (
   const sigBytes = ui8.fromString(sig, 'base64url')
   const verifySignatureWithKey = (key: string) => {
     return crypto.verifySignature(key, msgBytes, sigBytes, {
-      lowS: false,
+      allowMalleableSig: true,
     })
   }