diff --git a/.changeset/modern-planets-change.md b/.changeset/modern-planets-change.md new file mode 100644 index 00000000000..d019686c266 --- /dev/null +++ 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 index 7cdeb55ea75..2e41be58cc2 100644 --- 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 index 6f81b0371a9..e41c494ae55 100644 --- 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 => { + 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 index f470c2da54c..bc2415feb47 100644 --- 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 => { + 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 index a1089134f0a..be1dd4ec593 100644 --- 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 index 83d2b6b72f0..8ccaf7d992c 100644 --- 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 { 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 { + 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 index 0b0cbe03127..db6471aa23e 100644 --- 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, }) }