8000 Fix Misclassification of ECDSA Signatures in `verifyMultisig()` by valentinfernandez1 · Pull Request #1973 · polkadot-js/common · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix Misclassification of ECDSA Signatures in verifyMultisig() #1973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to fi 8000 le
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions packages/util-crypto/src/signature/verify.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { hexToU8a, stringToU8a, u8aConcat, u8aToHex, u8aWrapBytes } from '@polka
import { waitReady } from '@polkadot/wasm-crypto';

import { decodeAddress } from '../address/index.js';
import { secp256k1Sign } from '../secp256k1/sign.js';
import { signatureVerify } from './index.js';

const ADDR_ED = 'DxN4uvzwPzJLtn17yew6jEffPhXQfdKHTp2brufb98vGbPN';
Expand Down Expand Up @@ -37,7 +38,7 @@ describe('signatureVerify', (): void => {
});

describe('verifyDetect', (): void => {
it('verifies an ed25519 signature', (): void => {
it('verifies ed25519 signature', (): void => {
expect(signatureVerify(MESSAGE, SIG_ED, ADDR_ED)).toEqual({
crypto: 'ed25519',
isValid: true,
Expand All @@ -46,7 +47,7 @@ describe('signatureVerify', (): void => {
});
});

it('verifies an ecdsa signature', (): void => {
it('verifies ecdsa signatures', (): void => {
expect(signatureVerify(MESSAGE, SIG_EC, ADDR_EC)).toEqual({
crypto: 'ecdsa',
isValid: true,
Expand Down Expand Up @@ -173,11 +174,57 @@ describe('signatureVerify', (): void => {

it('fails on an invalid signature', (): void => {
expect(signatureVerify(MESSAGE, MUL_SR, ADDR_ED)).toEqual({
crypto: 'sr25519',
crypto: 'none',
isValid: false,
isWrapped: false,
publicKey: new Uint8Array([61, 12, 55, 211, 0, 211, 97, 199, 4, 37, 17, 213, 81, 175, 166, 23, 251, 199, 144, 210, 19, 83, 186, 1, 196, 231, 14, 156, 171, 46, 141, 146])
});
});
/**
* ref: https://github.com/polkadot-js/common/issues/1898
*
* The following test ensures that we cover a reproduction that showed
* an inherent issue with verifying ecdsa signatures which is fixed in
* https://github.com/polkadot-js/common/pull/1973.
*
* It uses a random secretKey, and publicKey pair along with `secp256k1Sign`
* as the signer which is used for `ecdsa`.
*/
it('Ensure ecdsa can sign and verify 1000 messages', (): void => {
const verifyThousandMessages = () => {
const secretKey = new Uint8Array([
103, 97, 114, 98, 97, 103, 101, 32, 114, 105, 100,
103, 101, 32, 107, 105, 99, 107, 32, 114, 111, 115,
101, 32, 101, 110, 100, 32, 115, 113, 117, 101
]);
const publicKey = new Uint8Array([
2, 179, 102, 92, 246, 50, 172, 88,
81, 116, 8, 211, 192, 131, 154, 184,
122, 83, 180, 104, 4, 227, 214, 195,
140, 11, 82, 229, 49, 211, 185, 176,
63
]);

for (let i = 0; i < 1000; i++) {
const message = `message ${i}`;
const encodedMessage = stringToU8a(message);
const signature = secp256k1Sign(encodedMessage, { secretKey });

const { isValid: valid } = signatureVerify(
message,
signature,
publicKey
);

if (!valid) {
return false;
}
}

return true;
};

expect(verifyThousandMessages()).toEqual(true);
});
});
});
43 changes: 22 additions & 21 deletions packages/util-crypto/src/signature/verify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,10 @@ const VERIFIERS_ECDSA: Verifier[] = [

const VERIFIERS: Verifier[] = [
['ed25519', ed25519Verify],
['sr25519', sr25519Verify],
...VERIFIERS_ECDSA
['sr25519', sr25519Verify]
];

const CRYPTO_TYPES: ('ed25519' | 'sr25519' | 'ecdsa')[] = ['ed25519', 'sr25519', 'ecdsa'];

function verifyDetect (result: VerifyResult, { message, publicKey, signature }: VerifyInput, verifiers = VERIFIERS): VerifyResult {
function verifyDetect (result: VerifyResult, { message, publicKey, signature }: VerifyInput, verifiers = [...VERIFIERS, ...VERIFIERS_ECDSA]): VerifyResult {
result.isValid = verifiers.some(([crypto, verify]): boolean => {
try {
if (verify(message, signature, publicKey)) {
Expand All @@ -56,25 +53,29 @@ function verifyDetect (result: VerifyResult, { message, publicKey, signature }:
}

function verifyMultisig (result: VerifyResult, { message, publicKey, signature }: VerifyInput): VerifyResult {
if (![0, 1, 2].includes(signature[0])) {
if (![0, 1, 2].includes(signature[0]) || ![65, 66].includes(signature.length)) {
throw new Error(`Unknown crypto type, expected signature prefix [0..2], found ${signature[0]}`);
}

const type = CRYPTO_TYPES[signature[0]] || 'none';

result.crypto = type;

try {
result.isValid = {
ecdsa: () => verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS_ECDSA).isValid,
ed25519: () => ed25519Verify(message, signature.subarray(1), publicKey),
none: () => {
throw Error('no verify for `none` crypto type');
},
sr25519: () => sr25519Verify(message, signature.subarray(1), publicKey)
}[type]();
} catch {
// ignore, result.isValid still set to false
// If the signature is 66 bytes it must be an ecdsa signature
// containing: prefix [1 byte] + signature [65] bytes.
// Remove the and then verify
if (signature.length === 66) {
result = verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS_ECDSA);
} else {
// The signature contains 65 bytes which is either
// - A ed25519 or sr25519 signature [1 byte prefix + 64 bytes]
// - An ecdsa signature [65 bytes]
result = verifyDetect(result, { message, publicKey, signature: signature.subarray(1) }, VERIFIERS);

if (!result.isValid) {
result = verifyDetect(result, { message, publicKey, signature }, VERIFIERS_ECDSA);
}

// If both failed, explicitly set crypto to 'none'
if (!result.isValid) {
result.crypto = 'none';
}
}

return result;
Expand Down
0