Fix ECDSA edge cases and harden new implementation - #46
Open
turbolent wants to merge 2 commits into
Open
Conversation
- return untyped nil interfaces on key decoding errors - return false instead of panicking in Equals(nil) - reject hashers whose computed hash is shorter than their declared size - clip the capacity of secp256k1 signatures sliced from the [R||S||V] buffer - make lazy PublicKey() construction safe for concurrent use - derive (N-1)/2 from N and cross-check the secp256k1 constants against go-ethereum at initialization - restore the P-256 public key length check - add tests: RFC 6979 known-answer vectors, concurrent PublicKey, nil inputs, dishonest hasher, compressed edge points
2 tasks
tarakby
reviewed
Aug 11, 2026
- build the public key eagerly at private key construction instead of lazily in PublicKey(), which removes the concurrency guards - read the secp256k1 curve constants from go-ethereum instead of hardcoding them and cross-checking - check the hasher output in bytes only, based on len(h), and drop the redundant hasher.Size() check - drop the P-256 public key length check, already covered by ParseUncompressedPublicKey - return the untyped nil once after the switch in the decoding functions - move the nil-input checks to sign_test_utils.go so they cover all algos, and extend them to DecodePublicKeyCompressed - document that the RFC 6979 vectors only test the current implementation, not a property the package guarantees - drop the concurrent PublicKey and compressed edge point tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #44
from a single path after the curve switch
Equals(nil)len(h)rather thanhasher.Size(),which also rejects hashers computing fewer bytes than they declare
[R||S||V]buffer,so an append can't reach the recovery byte
instead of lazily in
PublicKey(), which was not safe for concurrent useand derive
(N-1)/2fromNfor both curvespadToSizeAndConcattooutput[size:2*size]publicKeyECDSAP256,so both decoding paths build the key the same way
and the length check stays in
ParseUncompressedPublicKeysignatureFlipStest helper out ofecdsa.gosign_test_utils.go, so they cover all algorithms:untyped nil returns on invalid key lengths (including
DecodePublicKeyCompressed)and
Equals(nil)The RFC 6979 vectors only assert the correctness of the current underlying
implementation. The package requires neither deterministic nor low-S signatures,
and the test is documented as removable if the underlying implementation stops
providing both.