ECDSA signature verification error due to leading zero

[Markus-MS]

Summary

The Elliptic package 6.5.7 for Node.js ECDSA implementation does not correctly verify valid signatures if the hash contains at least 4 leading 0 bytes and when the order of the elliptic curve's base point is smaller than the hash. This leads to valid signatures being rejected.

This vulnerability was privately disclosed via the GitHub security advisory feature on July 17, 2024, with a public disclosure deadline of October 15, 2024. Unfortunately, we received no communication and are now publicly disclosing the issue. We reserved CVE-2024-48948 to track this vulnerability. This vulnerability has since also been discussed in Issue #321.

Tested Version

v6.5.7

Details

The ECDSA implementation contains an issue where valid signatures fail the validation check.
Hashes containing at least four leading zero bytes fail to validate even if they are correct if the hash size is greater than the order n of the base point of the elliptic curve.
An example of a problematic hash stems from hashing the hex string 343236343739373234 using SHA256, resulting in:

00000000690ed426ccf17803ebe2bd0884bcd58a1bb5e7477ead3645f356e7a9

According to FIPS 186-5, section 6.4.2 ECDSA Signature Verification Algorithm, the hash of the message must be adjusted based on the order n of the base point of the elliptic curve:

If log2(n) ≥ hashlen, set E = H. Otherwise, set E equal to the leftmost log2(n) bits of H.

To achieve this, the _truncateToN function is called, performing the necessary adjustment.
Before this function is called, the hashed message msg is converted from a hex string or array into a number object using new BN(msg, 16).

File: lib/elliptic/ec/index.js

EC.prototype._truncateToN = function _truncateToN(msg, truncOnly) {
  var delta = msg.byteLength() * 8 - this.n.bitLength();
  if (delta > 0)
    msg = msg.ushrn(delta);
  ...
};

The issue arises due to new BN(msg, 16) removing leading zeros (if there are at least 4), reducing the size of the original hash.
Removing the leading zeros results in a smaller hash, which takes up fewer bytes.
To illustrate the issue, we will use secp192r1, which uses 192 bits, and SHA256 as an example of an elliptic curve and hash.
The hash should be shifted by 64 bits to the right to retain the leftmost 192 bits.
However, during the delta calculation, msg.byteLength() returns 28 bytes instead of 32.
Consequently, the hashed message is not shifted correctly, causing verification to fail.

PoC

First, the library in question must be installed:

npm install elliptic@6.5.7

and then the POC can be executed using this commands:

node poc.js

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

var ec = new elliptic.ec('p192');
var msg = '343236343739373234';
var sig = '303502186f20676c0d04fc40ea55d5702f798355787363a91e97a7e50219009d1c8c171b2b02e7d791c204c17cea4cf556a2034288885b';
// Same public key just in different formats
var pk = '04cd35a0b18eeb8fcd87ff019780012828745f046e785deba28150de1be6cb4376523006beff30ff09b4049125ced29723';
var pkPem = '-----BEGIN PUBLIC KEY-----\nMEkwEwYHKoZIzj0CAQYIKoZIzj0DAQEDMgAEzTWgsY7rj82H/wGXgAEoKHRfBG54\nXeuigVDeG+bLQ3ZSMAa+/zD/CbQEkSXO0pcj\n-----END PUBLIC KEY-----\n';

// Create hash
var hashArray = hash.sha256().update(toArray(msg, 'hex')).digest();
// Convert array to string (just for showcase of the leading zeros)
var hashStr = Array.from(hashArray, function(byte) {
  return ('0' + (byte & 0xFF).toString(16)).slice(-2);
}).join('');
var hMsg = new BN(hashArray, 'hex');
// Hashed message contains 4 leading zeros bytes
console.log('sha256 hash(str): ' + hashStr);
// Due to using BN bitLength lib it does not calculate the bit length correctly (should be 32 since it is a sha256 hash)
console.log('Byte len of sha256 hash: ' + hMsg.byteLength());
console.log('sha256 hash(BN): ' + hMsg.toString(16));

// Due to the shift of the message to be within the order of the curve the delta computation is invalid
var pubKey = ec.keyFromPublic(toArray(pk, 'hex'));
console.log('Valid signature: ' + pubKey.verify(hashStr, sig));

// You can check that this hash should validate by consolidating openssl
const fs = require('fs');
fs.writeFile('msg.bin', new BN(msg, 16).toBuffer(), (err) => {
  if (err) throw err;
});
fs.writeFile('sig.bin', new BN(sig, 16).toBuffer(), (err) => {
  if (err) throw err;
});
fs.writeFile('cert.pem', pkPem, (err) => {
  if (err) throw err;
});

// To verify the correctness of the message signature and key one can run:
// openssl dgst -sha256 -verify cert.pem -signature sig.bin msg.bin
// Or run this python script
/*
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import ec


msg = '343236343739373234'
sig = '303502186f20676c0d04fc40ea55d5702f798355787363a91e97a7e50219009d1c8c171b2b02e7d791c204c17cea4cf556a2034288885b'
pk = '04cd35a0b18eeb8fcd87ff019780012828745f046e785deba28150de1be6cb4376523006beff30ff09b4049125ced29723'

p192 = ec.SECP192R1()
pk = ec.EllipticCurvePublicKey.from_encoded_point(p192, bytes.fromhex(pk))
pk.verify(bytes.fromhex(sig), bytes.fromhex(msg), ec.ECDSA(hashes.SHA256()))
*/

Impact

This issue prevents the successful validation of certain valid ECDSA signatures.
As a result, legitimate transactions or communications may be incorrectly flagged as invalid, leading to potential disruptions in service and a loss of trust in the system's reliability and security.
Furthermore, this vulnerability could cause a split in consensus among nodes in distributed systems, as some nodes might accept transactions that others reject, potentially leading to network fragmentation and inconsistent state across the system.

Remediation

We propose to remedy this issue by adding the bit size of the message as a required argument.

File: elliptic/lib/elliptic/ec/index.js

EC.prototype._truncateToN = function _truncateToN(msg, msgBitSize, truncOnly) {
  assert(msgBitSize != null, 'The bit size of the message (msgBitSize) is required to truncate the message correctly');
  var delta = msgBitSize - this.n.bitLength();
  if (delta > 0)
    msg = msg.ushrn(delta);
  if (!truncOnly && msg.cmp(this.n) >= 0)
    return msg.sub(this.n);
  else
    return msg;
};

Since the msgBitSize argument would be provided by the user of the library, this requires changes to the function signature of:

File: lib/elliptic/ec/index.js

EC.prototype.sign = function sign(msg, msgBitSize, key, enc, options) {
  if (typeof enc === 'object') {
    options = enc;
    enc = null;
  }
  if (!options)
    options = {};

  key = this.keyFromPrivate(key, enc);
  msg = this._truncateToN(new BN(msg, 16), msgBitSize);
...
}

EC.prototype.verify = function verify(msg, msgBitSize, signature, key, enc) {
  msg = this._truncateToN(new BN(msg, 16), msgBitSize);
...
}

as well as the usages of _truncateToN within these functions

Furthermore, the usage of EC.prototype.verify and EC.prototype.sign would need to be changed inside the corresponding KeyPair.prototype.sign and KeyPair.prototype.verify functions as well as their signatures.

File: lib/elliptic/ec/key.js

// ECDSA
KeyPair.prototype.sign = function sign(msg, msgBitSize, enc, options) {
  return this.ec.sign(msg, msgBitSize, this, enc, options);
};

KeyPair.prototype.verify = function verify(msg, msgBitSize, signature) {
  return this.ec.verify(msg, msgBitSize, signature, this);
};

Subsequently, the ECDSA tests test/ecdsa-test.js must be adjusted to provide the now-required msgBitSize to pass.

Discovered by

Markus Schiffermüller at Trail of Bits
Scott Arciszewski at Trail of Bits

using wycheproof

[hunkydoryrepair]

this solution appears breaking for existing apps using elliptic, which is very unfortunate since ethers 5 is still widely used but not being updated.
This would seem to be non-breaking if instead of requiring the library user to pass in the size, if the size were determined from the msg itself. It already assumes base16 so a msg.length * 4 is a pretty good start on getting the accurate byte length. Allowing an optional parameter to override the default would not hurt.

Of course, should check if msg is a number or if it contains a - sign and a couple other details, but breaking the library will make the fix less accessible.

[hunkydoryrepair]

what if we just did this
var msgBitSize = typeof msg === 'string' ? msg.replace(/\s+/g,'').length * 4 : 64;

leave the public function prototypes as they are?

[ChALkeR]

this seems like a semver-major breaking api change
can this be patched without api changes?

[avembankottu]

Can someone please merge this pr @Markus-MS

[hunkydoryrepair]

wow, ok.
Guess we will need to create a fork to continue using this package.

[Markus-MS]

@hunkydoryrepair @ChALkeR you are correct that the current changes require the hash size as a parameter, which means that a code update downstream would be necessary.

Requiring the hash size is arguably the most secure option (and, as far as I can tell, is the standard in most crypto libraries), but in this particular case, it is certainly not the most convenient option when it comes to backward compatibility.

From my perspective, there are two alternatives (though slightly less secure) that have already been discussed by @hunkydoryrepair which would avoid any breaking API changes:

1. Keep the hash size parameter but default to trying to guess the size if it is not provided.
2. Remove the hash size parameter and try to guess the size.

The proposed changes by @hunkydoryrepair are a good start, but going this route requires checking the hash size for all supported input types (not just strings).

For example, if one provides the hash as an Array (by first converting it using the toArray util function), the same issue is encountered:

console.log('Valid signature: ' + pubKey.verify(toArray(hashStr, 'hex'), sig));

Option 1 might be a reasonable middle ground, although you lose some security.

[Markus-MS]

Can someone please merge this pr @Markus-MS

@avembankottu I do not have the necessary permissions to merge any changes directly.

[hunkydoryrepair]

my concern is the ethers 5 package. It is not being updated, but dozens of packages are built on ethers 5, and ethers 6 did a HUGE number of breaking changes so things built on ethers 5 are not migrating quickly to 6 (if ever).
and ethers uses elliptic. As long as the API calls are compatible, we can override the elliptic version and get a working version in, but if the library API changes, I'm not sure how we'd ever get it working.

[avembankottu]

Cant we do this instead breaking api @Markus-MS
https://github.com/indutny/elliptic/pull/322/files#r1807077195

[paulmillr]

Why is this CVE? This is just an ordinary bug.

Kinda tired of using CVE for every little piece-of-shit change. This practice already degraded severity of a CVE.

[hunkydoryrepair]

Why is this CVE? This is just an ordinary bug.

Kinda tired of using CVE for every little piece-of-shit change. This practice already degraded severity of a CVE.

I tend to agree, although I don't know that this PR and Issue submitter is the same one that submitted the CVE. The CVE is just quoted here in the description.

I'm unclear on how breaking this is. CVE claims it could validate invalid signatures, but I'm not sure how that is possible. Sure, trimming a leading zero on a valid signature would still pass, but you'd need a valid signature to come up with the invalid one, so does not seem exploitable.

[avembankottu]

Any idea when will it get fixed ?
@hunkydoryrepair @Markus-MS

[paulmillr]

It would get fixed when someone fixes it. There is no need to ask these questions.

[bleichenbacher-daniel]

I think the correct API for implementations that accept prehashes is to define the hash and digest length when constructing the signer and verifier and any digest that does not have the correct length should be rejected. It feels very strange when the caller must know the length of the digest, but the signer and verfier do not have this information.

Accepting, arbitrary digest lengths just makes the implementation difficult to analyze. I.e. the question now becomes: "Is there a set of arbitrarily long digests, claimed digest lengths so that receiving their signatures either computed with the broken or the fixed version of the code leaks the private key?

As far as I can determine this is luckily not the case, and it appears that indeed some luck was needed here.

[paulmillr]

@bleichenbacher-daniel RFC6979 and FIPS 186-4 4.6 suggest the leftmost min(nBitLen, outLen) bits, which means for 1GB digest it would always take leftmost 32 bytes (for p256)

[indutny]

Hello everyone!

Sorry for the delay on my end with the original report and this open Pull Request. I'm evaluating options for applying this patch since as y'all have pointed out doing it as is means cutting a major release which we won't be helpful to anyone since people won't auto-upgrade.

My current thinking is along the lines of what have been suggested here:

• Either auto-compute the bit length of the message if it is a string or an array, and keep using .bitLength() if it is a BN
• and/or maybe just use byteLength() * 8 to begin with

Both are breaking changes regardless of how you look at them...

[paulmillr]

@indutny bn messages sound weird. I think changing behavior for them is fine.

Auto-computing bit length for strings / arrays seems easy and simple.

[indutny]

Sounds good. Here is the proposed alternative PR #326 .

I'd appreciate any feedback, and plan to merge it this evening unless someone has significant objections.

Thank y'all!

[indutny]

Published similar fix in 6.6.0. Thank you!