Missing checks during decoding of signatures leading to a certain degree of malleability of ECDSA and EDDSA signatures

[Markus-MS]

There are some checks that need to be included during the decoding stage of both ECDSA and EDDSA signatures.
The absence of these checks leads to some mailability issues for ECDSA and EDDSA signatures.
The code provided in this pull request fixes these issues.

The following test vectors from the Wycheproof project showcase these issues:

EDDSA

A missing length check during the signature creation makes removing or appending zeros bytes possible.

testvectors_v1/ed25519_test.json

var elliptic = require('elliptic'); // tested with version 6.5.6
var eddsa = elliptic.eddsa;

var ed25519 = new eddsa('ed25519');
var key = ed25519.keyFromPublic('7d4d0e7f6153a69b6242b522abbee685fda4420f8834b108c3bdae369ef549fa', 'hex');

// [tcId 37] appending 0 byte to signature
var msg = '54657374';
var sig =  '7c38e026f29e14aabd059a0f2db8b0cd783040609a8be684db12f82a27774ab07a9155711ecfaf7f99f277bad0c6ae7e39d4eef676573336a5c51eb6f946b30d00';
console.log(key.verify(msg, sig));

// [tcId 38] removing 0 byte from signature
msg = '546573743137';
sig =  '93de3ca252426c95f735cb9edd92e83321ac62372d5aa5b379786bae111ab6b17251330e8f9a7c30d6993137c596007d7b001409287535ac4804e662bc58a3';
console.log(key.verify(msg, sig));

The proposed changes inside the lib/elliptic/eddsa/signature.js file check the length of the signature to fix this issue.

ECDSA

The parsing of the ECDSA DER encode signatures has two minor issues.

Missing check if the leading bit of r and s is zero

According to the ASN encoding the leading bit for r and s should be zero.

testvectors_v1/ecdsa_secp256k1_sha256_test.json

var elliptic = require('elliptic'); // tested with version 6.5.6
var hash = require('hash.js');
var toArray = elliptic.utils.toArray;

var ec = new elliptic.ec('secp256k1');

// [tcId 6] Legacy: ASN encoding of r misses leading 0
var msg = '313233343030';
var sig = '30440220813ef79ccefa9a56f7ba805f0e478584fe5f0dd5f567bc09b5123ccbc983236502206ff18a52dcc0336f7af62400a6dd9b810732baf1ff758000d6f613a556eb31ba';
var pk = '04b838ff44e5bc177bf21189d0766082fc9d843226887fc9760371100b7ee20a6ff0c9d75bfba7b31a6bca1974496eeb56de357071955d83c4b1badaa0b21832e9';

var hashMsg = hash.sha256().update(toArray(msg, 'hex')).digest();
var pubKey = ec.keyFromPublic(pk, 'hex');
console.log('Valid signature: ' + pubKey.verify(hashMsg, sig));

Allowing BER-encoded signatures

DER requires a single valid encoding, and allowing for BER-encoded signatures creates the possibility of confusion and mailability.
Inside the lib/elliptic/ec/signature.js in the getLength function, allowing leading zeros for length sequence should not be permitted.

testvectors_v1/ecdsa_secp521r1_sha512_test.json

var elliptic = require('elliptic'); // tested with version 6.5.6
var hash = require('hash.js');
var toArray = elliptic.utils.toArray;

var ec = new elliptic.ec('p521');

// [tcId 7] length of sequence [r, s] contains a leading 0
var msg = '313233343030';
var sig = '3082008602414e4223ee43e8cb89de3b1339ffc279e582f82c7ab0f71bbde43dbe374ac75ffbef29acdf8e70750b9a04f66fda48351de7bbfd515720b0ec5cd736f9b73bdf8645024128b5d0926a4172b349b0fd2e929487a5edb94b142df923a697e7446acdacdba0a029e43d69111174dba2fe747122709a69ce69d5285e174a01a93022fea8318ac1';
var pk = '04005c6457ec088d532f482093965ae53ccd07e556ed59e2af945cd8c7a95c1c644f8a56a8a8a3cd77392ddd861e8a924dac99c69069093bd52a52fa6c56004a074508007878d6d42e4b4dd1e9c0696cb3e19f63033c3db4e60d473259b3ebe079aaf0a986ee6177f8217a78c68b813f7e149a4e56fd9562c07fed3d895942d7d101cb83f6';

var hashMsg = hash.sha512().update(toArray(msg, 'hex')).digest();
var pubKey = ec.keyFromPublic(pk, 'hex');
console.log('Valid signature: ' + pubKey.verify(hashMsg, sig));

To remedy this issue, an additional check was #added inside lib/elliptic/ec/signature.js in the getLength function if the current byte is zero, as this would not be permitted.

[StayPirate]

FTR: I found that the following three CVEs reference this PR.

• CVE-2024-42459
• CVE-2024-42460
• CVE-2024-42461

[kdenhartog]

It'd be useful to add the tests for this into the PR as well. Would you prefer to add them @Markus-MS or mind if I submit a patch to your branch?

[Markus-MS]

Great idea @kdenhartog
I planned to submit a separate PR containing a testing harness I wrote for all the Wycheproof testing vectors.
I think that would help to permanently improve the security level moving forward.

[justinclift]

Ahhh. This seems to be the source of the Dependabot alerts mentioned here: #319

[fevar54]

Solución para EDDSA
Verificación de la longitud de la firma: Asegúrate de que durante la creación de la firma no falten verificaciones de longitud. Esto previene la posibilidad de eliminar o agregar bytes cero.

Código:
javascript
Copiar código
var elliptic = require('elliptic');
var eddsa = elliptic.eddsa;
var ed25519 = new eddsa('ed25519');
var key = ed25519.keyFromPublic('7d4d0e7f6153a69b6242b522abbee685fda4420f8834b108c3bdae369ef549fa', 'hex');

// Verificación de longitud durante la creación de la firma
function verifySignatureLength(sig) {
if (sig.length !== 128) { // Longitud esperada para una firma ED25519
return false;
}
return true;
}

var msg = '54657374';
var sig = '7c38e026f29e14aabd059a0f2db8b0cd783040609a8be684db12f82a27774ab07a9155711ecfaf7f99f277bad0c6ae7e39d4eef676573336a5c51eb6f946b30d00';

if (verifySignatureLength(sig)) {
console.log(key.verify(msg, sig));
} else {
console.log('Invalid signature length');
}

msg = '546573743137';
sig = '93de3ca252426c95f735cb9edd92e83321ac62372d5aa5b379786bae111ab6b17251330e8f9a7c30d6993137c596007d7b001409287535ac4804e662bc58a3';

if (verifySignatureLength(sig)) {
console.log(key.verify(msg, sig));
} else {
console.log('Invalid signature length');
}

Solución para ECDSA
Comprobación del bit inicial de r y s: Asegúrate de que el bit inicial de r y s sea cero según la codificación ASN.
No permitir firmas codificadas con BER: Dentro de la función getLength, no permitas el uso de ceros a la izquierda para la secuencia de longitud.

Código:
javascript
var elliptic = require('elliptic');
var hash = require('hash.js');
var toArray = elliptic.utils.toArray;
var ec = new elliptic.ec('secp256k1');

// Verificación del bit inicial de r y s
function verifyLeadingZero(sig) {
var r = sig.slice(4, 68); // r en hexadecimal
var s = sig.slice(72, 136); // s en hexadecimal
if (r[0] === '0' || s[0] === '0') {
return false;
}
return true;
}

var msg = '313233343030';
var sig = '30440220813ef79ccefa9a56f7ba805f0e478584fe5f0dd5f567bc09b5123ccbc983236502206ff18a52dcc0336f7af62400a6dd9b810732baf1ff758000d6f613a556eb31ba';
var pk = '04b838ff44e5bc177bf21189d0766082fc9d843226887fc9760371100b7ee20a6ff0c9d75bfba7b31a6bca1974496eeb56de357071955d83c4b1badaa0b21832e9';

var hashMsg = hash.sha256().update(toArray(msg, 'hex')).digest();
var pubKey = ec.keyFromPublic(pk, 'hex');

if (verifyLeadingZero(sig)) {
console.log('Valid signature: ' + pubKey.verify(hashMsg, sig));
} else {
console.log('Invalid signature: leading zero detected');
}

[kdenhartog]

Great idea @kdenhartog
I planned to submit a separate PR containing a testing harness I wrote for all the Wycheproof testing vectors.
I think that would help to permanently improve the security level moving forward.

That sounds like a great idea in my opinion

[matteos025]

Any updates on when this will get merged?

[ysa-v]

Hello! Any updates on when this will get merged?

[indutny]

LGTM

[indutny]

Thank you so much @Markus-MS !

[xan187]

Yeh Marcus