fix: hash value in rfc6979 process should be modulo CURVE.n

[kklash]

Summary

Fixes a bug which would have resulted in incorrect values of k produced by the RFC6979 nonce-generation code, when signing any hash value greater than or equal to CURVE.n. This would not affect the validity of signatures, but was resulting in mismatching signatures compared to other secp256k1 implementations. It's doubtful that this would result in skewing of k values in any measurable amount relevant to security, since it only affects hash values >= CURVE.n, which would be rare.

I only noticed it because I was using your very awesome test vectors for my own purposes and discovered a mismatch in two of the 'strange hash' test-vectors.

Details

RFC6979 Section 3.2 Step D specifies the first HMAC pass to set K should be:

K = HMAC_K(V || 0x00 || int2octets(x) || bits2octets(h1))

RFC6979 specifies that the bits2octets function should ensure the result is modulo the order of the finite field (CURVE.n)

The code in this library wasn't properly implementing the bits2octets function, instead it was simply converting the hash into a number, then converting it back to a padded 32-length byte array without the modulo operation. This unprocessed byte array was concatenated into the HMAC message content. So what we were really doing was:

K = HMAC_K(V || 0x00 || int2octets(x) || h1)

This was previously unnoticed because this would only have any effect on hash values greater than or equal to the order of the finite field, which for large order curves is extremely rare. Finding a hash preimage which satisfies this condition would be like winning a really shitty lottery.

However, if it does happen, it would result in the RFC6979 code producing incorrect values for k compared to other compliant implementations, thus resulting in different (but valid) signatures.

Checking my Work

• Tests pass
• Added one extra test to cover case where hash is exactly equal to CURVE.n
• Ran the two broken test cases and the one new case with other secp256k1 implementations, such as btcec to confirm signatures match

[paulmillr]

Thanks for the contribution, that makes sense.

Where did you get 7cb38cc5712e9e11a767615f6080dbc111c9cdd613eb98999fd92a86bafd45407923ca1f4d03471d2866f776ef8a6d3cac099b427331aeb245aa9dafeddcf115 test result?

[paulmillr]

Are there concrete test vectors from other implementations that we can validate against?

[kklash]

Are there concrete test vectors from other implementations that we can validate against?

Unfortunately not that I could find. Most implementations seem to test with input message values which are then hashed, rather than testing specific hashes directly to root-out edgecases :( The best I've been able to is write my own using other implementations.

Here's a test against the python ecdsa package:

import ecdsa
import binascii
import hashlib


class Fixture:
    def __init__(self, d, hash, sig):
        self.hash = binascii.unhexlify(hash)
        self.d = int(d, 16)
        self.sig = sig


fixtures = [
    Fixture(
        d="0000000000000000000000000000000000000000000000000000000000000001",
        hash="ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
        sig="7cb38cc5712e9e11a767615f6080dbc111c9cdd613eb98999fd92a86bafd45407923ca1f4d03471d2866f776ef8a6d3cac099b427331aeb245aa9dafeddcf115",
    ),

    Fixture(
        d="fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140",
        hash="ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
        sig="a7f83b5963eaf5332c633327cc967be8f4166d3f1e0b77f9761d8f4e42211e9aa7551ce41e14e1b696dc441735a17bc1bf24e80d287282ecdffa879093994b15",
    ),

    Fixture(
        d="fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140",
        hash="fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141",
        sig="919026f3e239ea52cf530eb6d345dc2b56ef0928f1e9ad20d8f360284dc6504814395e7137e2204f15b69239010f3c34fbb3c858a29b0d106b1fa65bc0047263",
    ),
]


for fixture in fixtures:
    key = ecdsa.SigningKey.from_secret_exponent(fixture.d, curve=ecdsa.SECP256k1)
    sig = key.sign_digest_deterministic(fixture.hash, hashfunc=hashlib.sha256)
    sig_hex = binascii.hexlify(sig).decode("utf8")

    assert sig_hex == fixture.sig

Note that the 2nd test fixture there differs from the one in this PR, but only because python ecdsa package does not canonicalize their signatures. The raw 's' component of the signature in that fixture is larger than CURVE.n/2. Subtract it from CURVE.n to get the correct 's' value, which is what I've submitted in this PR.

>>> s = 0xa7551ce41e14e1b696dc441735a17bc1bf24e80d287282ecdffa879093994b15
>>> n = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
>>> hex(n - s)
'0x58aae31be1eb1e496923bbe8ca5e843cfb89f4d986d61d4edfd7d6fc3c9cf62c'

I also ran a similar test with the secp256k1 implementation from btcsuite/btcd: https://github.com/btcsuite/btcd/tree/master/btcec , as well as with a closed-source secp256k1 implementation that I unfortunately can't share at the moment

[paulmillr]

I have ran your vectors against official libsecp256k1 (using node.js module secp256k1. They match whatever we have in noble. You should report this issue to them.

const secp256k1 = require('secp256k1')
const hex = b => Buffer.from(b).toString('hex')
const bytes = h => Uint8Array.from(Buffer.from(h, 'hex'))
const priv = bytes('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140')
const pub = secp256k1.publicKeyCreate(priv)
const msg = bytes('ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff')
const sig = secp256k1.ecdsaSign(msg, priv)
console.log(hex(sig.signature));

[paulmillr]

libsecp256k1

1. uses msg32 as-is without reducing it in nonce_function_rfc6979 to generate k https://github.com/bitcoin-core/secp256k1/blob/0559fc6e41b65af6e52c32eb9b1286494412a162/src/secp256k1.c#L435
2. reduces msg32 to msg mod n at this line: https://github.com/bitcoin-core/secp256k1/blob/0559fc6e41b65af6e52c32eb9b1286494412a162/src/secp256k1.c#L473
3. Seems to use reduced msg mod n to actually sign the msg https://github.com/bitcoin-core/secp256k1/blob/0559fc6e41b65af6e52c32eb9b1286494412a162/src/secp256k1.c#L484

[vae520283995]

Here is the sample code for the draft
https://datatracker.ietf.org/doc/html/rfc6979#page-70

private byte[] bits2octets(byte[] in)
          {
                  BigInteger z1 = bits2int(in);
                  BigInteger z2 = z1.subtract(q);
                  return int2octets(z2.signum() < 0 ? z1 : z2);
          }

[kklash]

@paulmillr has opened an issue in libsecp256k1 to address this: bitcoin-core/secp256k1#1063

[paulmillr]

You mean me?

[kklash]

Yep, i was writing for posterity :D

[paulmillr]

Thanks, 35e66d4 and v1.5.0 are out

[paulmillr]

Now waiting for libsecp256k1 PR to get merged.

[kklash]

Thanks! Glad i could help =)