Skip to content
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

ECDSA does not conform to RFC6979 for messages > curve_order #1063

Closed
paulmillr opened this issue Jan 14, 2022 · 8 comments
Closed

ECDSA does not conform to RFC6979 for messages > curve_order #1063

paulmillr opened this issue Jan 14, 2022 · 8 comments

Comments

@paulmillr
Copy link
Contributor

paulmillr commented Jan 14, 2022

RFC6979 3.2.d says:

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

where bits2octets is, as per RFC6979 2.3.4 curve order modulo-reduced message:

   The bits2octets transform takes as input a sequence of blen bits and
   outputs a sequence of rlen bits.  It consists of the following steps:

   1.  The input sequence b is converted into an integer value z1
       through the bits2int transform:

          z1 = bits2int(b)
   2.  z1 is reduced modulo q, yielding z2 (an integer between 0 and
       q-1, inclusive):

          z2 = z1 mod q

       Note that since z1 is less than 2^qlen, that modular reduction
       can be implemented with a simple conditional subtraction:
       z2 = z1-q if that value is non-negative; otherwise, z2 = z1.

   3.  z2 is transformed into a sequence of octets (a sequence of rlen
       bits) by applying int2octets.

The implementation's sign takes msg32 — not modulo-reduced msg, and passes it forward.

ret = !!noncefp(nonce32, msg32, seckey, NULL, (void*)noncedata, count);

Seems like a bug, which does not exist in go-btcec etc.

@sipa
Copy link
Contributor

sipa commented Jan 14, 2022

Yes and no.

secp256k1_scalar_set_b32_seckey only accepts numbers in range [1,order), which are equal to their reductions modulo order.

So the behavior here is to reject nonces >= order, rather than to reduce them. This isn't an observable difference though, given how close the order is to 2^256.

@paulmillr
Copy link
Contributor Author

@sipa Ah, this isn't about nonces > curve_order, this is about message aka hashes over curve order. There isn't any check being done for it.

@sipa
Copy link
Contributor

sipa commented Jan 14, 2022

In that case there is no problem, I think. secp256k1_scalar_set_b32(&msg, msg32, NULL); reduces modulo the order (the scalar type is implicitly modulo the curve order).

@paulmillr
Copy link
Contributor Author

paulmillr commented Jan 14, 2022

The library reduces msg32 into msg:

secp256k1_scalar_set_b32(&msg, msg32, NULL);

But! it forgets to pass it 2 lines below:

ret = !!noncefp(nonce32, msg32, seckey, NULL, (void*)noncedata, count);

RFC6979 3.2.d says it should be reduced before being passed to noncefp

@sipa
Copy link
Contributor

sipa commented Jan 14, 2022

Oh, now I finally understand why you were linking to the nonce generation line.

I agree, it technically deviates from the spec, but not in an observable way.

@paulmillr
Copy link
Contributor Author

Here's a test case where it'll produce a signature which is different from some other implementations:

key=0000000000000000000000000000000000000000000000000000000000000001
msg=ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff

Which seems like a consensus bug between libraries? Is there any disadvantage in making it conform to spec?

@sipa
Copy link
Contributor

sipa commented Jan 14, 2022

It only matters on the signing side, so it's not a consensus issue. I also believe it is harmless (both because msg should be a hash for ecdsa_verify to be secure, and even if it isn't, it just strictly increases the information fed to the nonce functions, which never hurts).

I see no reason why it couldn't be fixed to make it follow the spec to the letter, though.

@kklash
Copy link

kklash commented Jan 15, 2022

More background here: paulmillr/noble-secp256k1#42

@sipa sipa closed this as completed in 45f37b6 Jan 22, 2022
sipa added a commit that referenced this issue Jan 22, 2022
Fixes #1063

45f37b6 Modulo-reduce msg32 inside RFC6979 nonce fn to match spec. Fixes #1063. (Paul Miller)

Pull request description:

ACKs for top commit:
  siv2r:
    ACK 45f37b6. The diff looks good. It reduces `msg32` to modulo curve order for rfc6979 nonce generation. All tests passed on my machine with `make check`.
  sipa:
    utACK 45f37b6
  real-or-random:
    ACK 45f37b6

Tree-SHA512: 4c36784b2d6f2983bc0c3f380ff59cd9f2bd1822b98116d70964cd15183742fcc1f2ccde225a76dd30d946b3678b2cf29caff018efc07f40a200ee85843b39dd
matteonardelli pushed a commit to bancaditalia/secp256k1-frost that referenced this issue Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants