-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Deterministic signatures according to RFC6979 #360
Conversation
Thanks for the pull request. I'm going to do a quick review now for general things, but I plan to dig into the RFC a little later and do a very careful analysis since this is obviously extremely important to get right given signature issues can easily lead to stolen funds. |
@@ -56,11 +56,7 @@ func (p *PrivateKey) ToECDSA() *ecdsa.PrivateKey { | |||
// Sign wraps ecdsa.Sign to sign the provided hash (which should be the result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment to reflect reality here.
@davecgh thanks, updated the code according to your remarks. |
// - https://github.com/trezor/trezor-crypto/blob/9fea8f8ab377dc514e40c6fd1f7c89a74c1d8dc6/tests.c#L432-L453 | ||
// - https://github.com/oleganza/CoreBitcoin/blob/e93dd71207861b5bf044415db5fa72405e7d8fbc/CoreBitcoin/BTCKey%2BTests.m#L23-L49 | ||
verifyRFC6979Nonce( | ||
"cca9fbcc1b41e5a95d369eaa6ddcff73b61a4efaa279cfc6567e8daa39cbaf50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these are hard coded strings in the test source code, I'd prefer all of these use a function like the following decodeHex
which panics if any errors occur parsing them rather than ignoring them.
// decodeHex converts the passed hex string into a big integer pointer and will
// panic is there is an error. This is only provided for the hard-coded
// constants so errors in the source code can bet detected. It will only (and
// must only) be called for initialization purposes.
func decodeHex(hexStr string) []byte {
hexBytes, err := hex.DecodeString(hexStr)
if err != nil {
panic("invalid hex in source file: " + hexStr)
}
return hexBytes
}
Alright that's it for now. I'll do an in-depth review of the algorithm against the RFC later tonight. |
@@ -396,3 +408,130 @@ func RecoverCompact(curve *KoblitzCurve, signature, | |||
|
|||
return key, ((signature[0] - 27) & 4) == 4, nil | |||
} | |||
|
|||
// SignRFC6979 generates a deterministic ECDSA signature according to RFC 6979 and BIP 62. | |||
func SignRFC6979(privateKey *PrivateKey, hash []byte) (*Signature, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need to export this since it's scoped to the private key.
@davecgh do you know what's wrong with |
Looks like it needs a gofmt on |
Thanks. The error was not very helpful. Lets see if it works. |
Yeah, I noticed that as well. I just made #361 to address that. |
Thanks for your work on this! It's looking good. I imagine it's right given the tests pass, but as I said previously, I still plan to give the algorithm a through review later tonight after I finish up with some other things I'm working on. |
Thanks for the comments, I appreciate your feedback. |
Oh, now that we have deterministic signatures, I think it would be worth updating the EDIT: If you're unaware, |
s.Mod(s, N) | ||
|
||
if s.Cmp(halforder) == 1 { | ||
s = new(big.Int).Sub(order, s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra allocation can be avoided here.
s.Sub(N, s)
That is s = N - s
.
Also, the rest of the code here is using N for the order, probably should use N here too.
Alright, I've finished carefully comparing this against the RFC and everything looks accurate. I would like to point out a few things that can be improved in the future:
All of that said, I see no reason any of those points should hold up this PR since it works properly and provides highly useful functionality. @oleganza However, I would like to see the the other comments addressed. |
@jimmysong Can you review and verify the algorithms conform to the RFC as well? |
@davecgh Looking at the RFC spec now. Will review the code soon. |
@davecgh thanks! It's my first time I write in Go. Sorry for some obvious things like unnecessary allocations. Thanks for correcting those. |
k := nonceRFC6979(privkey.D.Bytes(), hash) | ||
inv := new(big.Int).ModInverse(k, N) | ||
r, _ := privkey.Curve.ScalarBaseMult(k.Bytes()) | ||
r.Mod(r, N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit here, this is probably just a bit faster:
if r.cmp(N) == 1 {
r.Sub(r, N)
}
@jimmysong Thanks for taking the time to look it over! |
Addressed all the remarks. Please double-check. |
@davecgh @jimmysong Should I squash and rebase to make the merge neat? |
@oleganza I would check for s being zero, but that's about it. |
@oleganza Yes, I'd like a single commit after the final OKs. Please hold off until then though as it's much easier to review the incremental changes during the review process that address items as separate commits. Also, in the final commit, please add a |
if s.Cmp(halforder) == 1 { | ||
s.Sub(N, s) | ||
} | ||
if r.Sign() == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.Sign
All updates look good except the typo in commit 489d98e, which I commented on ( |
@@ -37,13 +37,14 @@ func Example_signMessage() { | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This NOTE can be removed now since it's no longer true with you change.
Oops, thanks. Waiting for the final OK. |
Looks good. OK. |
See #358 for discussion.
The test vectors are the same as in Trezor and CoreBitcoin implementations and cover both the raw nonce (k) and the complete DER-encoded signature (which must have canonical S).