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

crypto/ecdsa: generate RFC 6979 signatures if rand is nil #64802

Closed
FiloSottile opened this issue Dec 19, 2023 · 14 comments
Closed

crypto/ecdsa: generate RFC 6979 signatures if rand is nil #64802

FiloSottile opened this issue Dec 19, 2023 · 14 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Dec 19, 2023

crypto/ecdsa currently generates "hedged" signatures, by drawing the random nonce from an AES-CTR CSPRNG keyed by SHA2-512(priv.D || entropy || hash)[:32]. This is great, as it provides the best of both worlds: fault injection tolerance and RNG failure resistance.

However, sometimes you do need deterministic signatures, not because they are more secure (as hedged signatures are even better) but because you actually need the signature not to change over time. I propose that when the rand parameter is nil (which currently panics), we produce RFC 6979 deterministic signatures.

Morevoer, Section 3.6 of RFC 6979 specifies how to add additional data to the RNG input, so we can replace our AES-CTR construction with that. The advantage of this would be that although the RFC doesn't have test vectors for this variant, we can generate some and share them with other implementations.

o  Additional data may be added to the input of HMAC, concatenated
      after bits2octets(H(m)):

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

      A use case may be a protocol that requires a non-deterministic
      signature algorithm on a system that does not have access to a
      high-quality random source.  It suffices that the additional data
      k' is non-repeating (e.g., a signature counter or a monotonic
      clock) to ensure "random-looking" signatures are
      indistinguishable, in a cryptographic way, from plain (EC)DSA
      signatures.  In [SP800-90A] terminology, k' is the "additional
      input" that can be set as a parameter when generating pseudorandom
      bits.  This variant can be thought of as a "strengthening" of the
      randomness of the source of the additional data k'.

(The CFRG has a forever-stalled draft on "deterministic with noise" ECDSA signatures, draft-irtf-cfrg-det-sigs-with-noise, but it's unclear to me why we'd want that instead of the existing RFC 6979 mechanism.)

The properties of signatures generated by current applications wouldn't change: they would still be hendged, randomized, and unpredictable (because we use MaybeReadByte, so we can change the internals, yay!).

/cc @golang/security

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Dec 19, 2023
@gopherbot gopherbot added this to the Proposal milestone Dec 19, 2023
@FiloSottile
Copy link
Contributor Author

Unfortunately, for RFC 6979 we need to instantiate HMAC with the hash that was used to hash the message. In Sign and SignASN1, we don't have this information, only in PrivateKey.Sign.

We could implement this only for PrivateKey.Sign, or take a pragmatic route and map hash lengths to SHA-256/SHA-512 in SignASN1. (We'd document that, and still do the right thing in PrivateKey.Sign.)

Another wrinkle is that opts is currently ignored in PrivateKey.Sign, so we need to be prepared for current applications setting it wrong.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/552215 mentions this issue: crypto/ecdsa: implement RFC6979

@FiloSottile
Copy link
Contributor Author

Ok, since we can't do this properly in Sign and SignASN1, I propose

  1. when the rand parameter to (*PrivateKey).Sign) is nil we produce RFC 6979 deterministic signatures

  2. we add SignDeterministic(priv *PrivateKey, digest []byte, hash crypto.Hash) ([]byte, error)

@FiloSottile
Copy link
Contributor Author

Actually, the top level functions might be worth rethinking in a v2 anyway, especially if we adopt some standard hedged signature scheme such as draft-irtf-cfrg-det-sigs-with-noise, maybe while doing FIPS work.

What I'm proposing here then is just that when the rand parameter to (*PrivateKey).Sign is nil we produce RFC 6979 deterministic signatures. Currently, the function panics if rand is nil.

Deterministic signatures are enough of an uncommon use case that it's ok if they are not super easy to access.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is that ecdsa.PrivateKey.Sign start accepting rand==nil and in that case return a deterministic RFC 6979 signature. There is no new API, only the redefinition of that failure mode.

A new GODEBUG setting will control the behavior. GODEBUG=ecdsarfc6979=1 (the new default for go 1.23+ main modules) means enable these deterministic signatures when rand==nil; GODEBUG=ecdsarfc6979=0 means don’t.

@rsc rsc moved this from Incoming to Likely Accept in Proposals May 8, 2024
@FiloSottile
Copy link
Contributor Author

Given rand==nil currently panics, do we need the GODEBUG? I can't imagine an application relying on a panic here.

@rsc
Copy link
Contributor

rsc commented May 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is that ecdsa.PrivateKey.Sign start accepting rand==nil and in that case return a deterministic RFC 6979 signature. There is no new API, only the redefinition of that failure mode.

A new GODEBUG setting will control the behavior. GODEBUG=ecdsarfc6979=1 (the new default for go 1.23+ main modules) means enable these deterministic signatures when rand==nil; GODEBUG=ecdsarfc6979=0 means don’t.

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 15, 2024
@rsc rsc changed the title proposal: crypto/ecdsa: generate RFC 6979 signatures if rand is nil crypto/ecdsa: generate RFC 6979 signatures if rand is nil May 15, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 15, 2024
@mateusz834
Copy link
Member

@rsc Was this #64802 (comment) addressed? Is there a need for this GODEBUG?

@rolandshoemaker
Copy link
Member

The previous behavior here (rand == nil) would cause a panic. This change will "fix" otherwise broken old code, by causing usage of deterministic signatures, rather than panicking, but that seems reasonable. I don't think a GODEBUG is strictly necessary here.

@rsc
Copy link
Contributor

rsc commented May 23, 2024

Since we missed Go 1.23, let's land this at the start of Go 1.24 and wait for feedback before adding the GODEBUG. We will have the whole cycle for someone to point out a convoluted sequence that makes this a breaking change.

@FiloSottile
Copy link
Contributor Author

Sounds good, will target early Go 1.24 for this. (I chose not to land it in Go 1.23 because it turned out to be a larger change than expected, touching very delicate parts of the code, and I didn't feel like we had enough review bandwidth or brewing time to do it safely.)

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Jun 13, 2024
@dmitshur dmitshur added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jun 13, 2024
@gopherbot
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.24.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/628681 mentions this issue: crypto/ecdsa: implement deterministic and hedged signatures

@dmitshur
Copy link
Contributor

@FiloSottile Does this change need to covered in Go 1.24 release notes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

6 participants