From 50ddda0116bb6f38f3270ab8529c1e6a6d4b8c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Mon, 10 Aug 2026 14:36:13 -0700 Subject: [PATCH 1/3] fix ECDSA edge cases and harden new implementation - 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 --- ecdsa.go | 92 +++++++++++++++++------- ecdsa_p256.go | 38 +++++----- ecdsa_secp256k1.go | 38 ++++++---- ecdsa_test.go | 171 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 57 deletions(-) diff --git a/ecdsa.go b/ecdsa.go index 841486c1..ffbcfffd 100644 --- a/ecdsa.go +++ b/ecdsa.go @@ -58,19 +58,29 @@ func bitsToBytes(bits int) int { return (bits + 7) >> 3 } -func (a *ecdsaContext) checkAlgoAndComputeHash(msg []byte, hasher hash.Hasher) (hash.Hash, error) { +// checkHasherAndComputeHash checks the hasher is valid for ECDSA +// on the receiver curve and returns the hash of the input message. +func (a *ecdsaContext) checkHasherAndComputeHash(msg []byte, hasher hash.Hasher) (hash.Hash, error) { if hasher == nil { return nil, errNilHasher } // check hasher's size is at least the curve order in bytes - nLen := (a.curveN).BitLen() - if (hasher.Size() << 3) < nLen { + nLen := bitsToBytes((a.curveN).BitLen()) + if hasher.Size() < nLen { return nil, invalidHasherSizeErrorf( - "hasher's bit-size should be at least %d, got %d", nLen, hasher.Size()<<3) + "hasher's size should be at least %d bytes, got %d bytes", nLen, hasher.Size()) } h := hasher.ComputeHash(msg) + // guard against hasher implementations that compute fewer bytes + // than their declared size, + // since callers truncate the hash to the curve order size + // and would panic on a shorter slice + if len(h) < nLen { + return nil, invalidHasherSizeErrorf( + "hasher's output should be at least %d bytes, got %d bytes", nLen, len(h)) + } return h, nil } @@ -106,7 +116,6 @@ func (a *ecdsaContext) signatureFormatCheck(sig Signature) bool { } var one = new(big.Int).SetInt64(1) -var two = new(big.Int).SetInt64(2) // mapToPrivateKey simply maps the input seed to an ECDSA private key // The private scalar `d` satisfies 0 < d < n. @@ -139,7 +148,14 @@ func (a *ecdsaContext) privateKey(d *big.Int) (PrivateKey, error) { // build the private key depending on the curve switch a.algo { case ECDSAP256: - return privateKeyECDSAP256(a, dBytes) + sk, err := privateKeyECDSAP256(a, dBytes) + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return sk, nil case ECDSASecp256k1: return privateKeyECDSASecp256k1(a, dBytes), nil default: @@ -221,9 +237,23 @@ func (a *ecdsaContext) decodePrivateKey(der []byte) (PrivateKey, error) { func (a *ecdsaContext) rawDecodePublicKey(input []byte) (PublicKey, error) { switch a.algo { case ECDSAP256: - return publicKeyECDSAP256(input) + pk, err := publicKeyECDSAP256(input) + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil case ECDSASecp256k1: - return publicKeyECDSASecp256k1(a, input) + pk, err := publicKeyECDSASecp256k1(a, input) + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil default: return nil, invalidInputsErrorf("curve is not supported") } @@ -243,9 +273,23 @@ func (a *ecdsaContext) decodePublicKey(der []byte) (PublicKey, error) { func (a *ecdsaContext) decodePublicKeyCompressed(pkBytes []byte) (PublicKey, error) { switch a.algo { case ECDSAP256: - return p256DecodePublicKeyCompressed(pkBytes) + pk, err := p256DecodePublicKeyCompressed(pkBytes) + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil case ECDSASecp256k1: - return secp256k1DecodePublicKeyCompressed(pkBytes) + pk, err := secp256k1DecodePublicKeyCompressed(pkBytes) + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil default: return nil, invalidInputsErrorf("the input curve is not supported") } @@ -280,6 +324,10 @@ func pubKeyCommonECDSAString(pk PublicKey) string { // Equals tests the equality of two private keys func prKeyCommonECDSAEquals(sk, other PrivateKey) bool { + // a nil key is not equal to any key + if other == nil { + return false + } // check the algorithm if sk.Algorithm() != other.Algorithm() { return false @@ -300,6 +348,10 @@ func (pk *pubKeyCommonECDSA) Size() int { // Equals tests the equality of two private keys func pubKeyCommonECDSAEquals(pk, other PublicKey) bool { + // a nil key is not equal to any key + if other == nil { + return false + } // check the algorithm if pk.Algorithm() != other.Algorithm() { return false @@ -313,7 +365,7 @@ func pubKeyCommonECDSAEquals(pk, other PublicKey) bool { // It assumes the output buffer has at least 2*size byte-length func padToSizeAndConcat(output []byte, a, b *big.Int, size int) { a.FillBytes(output[:size]) - b.FillBytes(output[size:]) + b.FillBytes(output[size : 2*size]) } // Helper function to read two big integers of "size" bytes each from a concatenated input buffer. @@ -333,7 +385,9 @@ func (a *ecdsaContext) isLowS(s *big.Int) bool { // signatureNormalizeS returns a signature with S normalized to low S. // (same slice is returned if S is already normalized) // It assumes len(sig) == 2*nLen where nLen is the byte-length of the curve order. -// This is needed when the underlying signature verification requires S to be in the lower range (to avoid signature malleability). In this package, verification allows high S signatures to be accepted. +// This is needed when the underlying signature verification requires S to be +// in the lower range (to avoid signature malleability). +// In this package, verification allows high S signatures to be accepted. // The function checks that S is in the range [0, n-1] before normalizing it. // If S is not in this range, the function returns a false boolean. // (S and R values will be checked by the go-ethereum verification function - only S check against N is included here, S=0 check is deferred to the signature verification) @@ -361,17 +415,3 @@ func (a *ecdsaContext) signatureNormalizeS(sig []byte) ([]byte, bool) { sComplement.FillBytes(newSig[nLen:]) // write S complement return newSig, true } - -// Test function only to flip S in a signature. It is used for testing signature malleability -func (a *ecdsaContext) signatureFlipS(sig []byte) []byte { - // read S - nLen := bitsToBytes(a.curveN.BitLen()) - s := new(big.Int).SetBytes(sig[nLen:]) - // compute N-S - sComplement := new(big.Int).Sub(a.curveN, s) - // write it into a new signature - newSig := make([]byte, len(sig)) - copy(newSig, sig[:nLen]) // copy R - sComplement.FillBytes(newSig[nLen:]) // write S complement - return newSig -} diff --git a/ecdsa_p256.go b/ecdsa_p256.go index a5b8e02f..2f218277 100644 --- a/ecdsa_p256.go +++ b/ecdsa_p256.go @@ -24,6 +24,7 @@ import ( "crypto/rand" "fmt" "math/big" + "sync" "github.com/onflow/crypto/hash" ) @@ -50,12 +51,11 @@ var p256Instance *ecdsaContext func initECDSAP256() { curve := elliptic.P256() n := curve.Params().N - nMinus1 := new(big.Int).Sub(n, one) p256Instance = &(ecdsaContext{ curveP: curve.Params().P, curveN: n, - curveNdiv2: new(big.Int).Div(nMinus1, two), // (N-1)/2 + curveNdiv2: new(big.Int).Rsh(n, 1), // (N-1)/2, since N is odd algo: ECDSAP256, }) } @@ -66,6 +66,9 @@ type prKeyECDSAP256 struct { *prKeyCommonECDSA // go ecdsa standard lib private key goPrKey *ecdsa.PrivateKey + // pubKeyOnce guards the lazy construction of pubKey, + // making concurrent calls to PublicKey safe + pubKeyOnce sync.Once // public key pubKey *pubKeyECDSAP256 } @@ -109,7 +112,7 @@ func privateKeyECDSAP256(a *ecdsaContext, dBytes []byte) (*prKeyECDSAP256, error // - (nil, error) if an unexpected error occurs // - (signature, nil) otherwise func (sk *prKeyECDSAP256) Sign(msg []byte, hasher hash.Hasher) (Signature, error) { - hash, err := sk.checkAlgoAndComputeHash(msg, hasher) + hash, err := sk.checkHasherAndComputeHash(msg, hasher) if err != nil { return nil, err } @@ -130,6 +133,11 @@ func (sk *prKeyECDSAP256) String() string { // returns a publicKeyECDSAP256 from (bytes(x) || bytes(y)) bytes func publicKeyECDSAP256(XYBytes []byte) (*pubKeyECDSAP256, error) { + if len(XYBytes) != 2*pLenP256 { + return nil, invalidInputsErrorf("input has incorrect %s key size, got %d, expects %d", + ECDSAP256, len(XYBytes), 2*pLenP256) + } + // deserialization uses SEC1 version 2 (https://www.secg.org/sec1-v2.pdf section 2.3.3) // and includes on curve check. // The bytes serialization for non-infinity points is `0x04 || X || Y` and infinity point should be rejected anyway @@ -154,12 +162,12 @@ func (pk *pubKeyECDSAP256) String() string { // PublicKey returns the public key associated to the private key func (sk *prKeyECDSAP256) PublicKey() PublicKey { // construct the public key once - if sk.pubKey == nil { + sk.pubKeyOnce.Do(func() { sk.pubKey = &pubKeyECDSAP256{ pubKeyCommonECDSA: &pubKeyCommonECDSA{p256Instance}, goPubKey: &sk.goPrKey.PublicKey, } - } + }) return sk.pubKey } @@ -177,7 +185,7 @@ func (sk *prKeyECDSAP256) PublicKey() PublicKey { // - (false, error) if an unexpected error occurs // - (validity, nil) otherwise func (pk *pubKeyECDSAP256) Verify(sig Signature, data []byte, alg hash.Hasher) (bool, error) { - h, err := pk.checkAlgoAndComputeHash(data, alg) + h, err := pk.checkHasherAndComputeHash(data, alg) if err != nil { return false, err } @@ -266,17 +274,9 @@ func p256DecodePublicKeyCompressed(pkBytes []byte) (*pubKeyECDSAP256, error) { if x == nil || y == nil { return nil, invalidInputsErrorf("input %x isn't a compressed serialization of a point on P256", pkBytes) } - uncompressedPointBytes := make([]byte, 2*pLenP256+1) - uncompressedPointBytes[0] = ecEncodingUncompressed - padToSizeAndConcat(uncompressedPointBytes[1:], x, y, pLenP256) - - internalPK, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), uncompressedPointBytes) - if err != nil { - // unexpected error since prior deserialization succeeded - return nil, invalidInputsErrorf("unexpected error: input is not a point on curve P-256: %w", err) - } - return &pubKeyECDSAP256{ - &pubKeyCommonECDSA{p256Instance}, - internalPK, - }, nil + // serialize the coordinates and delegate to the uncompressed decoding, + // so that both decoding paths construct the key the same way + xyBytes := make([]byte, 2*pLenP256) + padToSizeAndConcat(xyBytes, x, y, pLenP256) + return publicKeyECDSAP256(xyBytes) } diff --git a/ecdsa_secp256k1.go b/ecdsa_secp256k1.go index 11453a38..8c3c1eae 100644 --- a/ecdsa_secp256k1.go +++ b/ecdsa_secp256k1.go @@ -21,6 +21,7 @@ package crypto import ( "fmt" "math/big" + "sync" "github.com/ethereum/go-ethereum/crypto/secp256k1" @@ -38,8 +39,6 @@ const ( nLenSecp256k1 = 32 pLenSecp256k1 = 32 - - secp256k1Ndiv2Hex = "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0" ) const ( @@ -62,14 +61,16 @@ func initECDSASecp256k1() { if !ok { panic("failed to initialize ECDSA with secp256k1 curve") } - curveNdiv2, ok := new(big.Int).SetString(secp256k1Ndiv2Hex, 16) - if !ok { - panic("failed to initialize ECDSA with secp256k1 curve") + // cross-check the hard-coded SEC2 constants against the constants + // of the underlying go-ethereum implementation, + // so that a typo in either place is caught at initialization + if curveP.Cmp(secp256k1.S256().P) != 0 || curveN.Cmp(secp256k1.S256().N) != 0 { + panic("secp256k1 curve constants do not match the underlying go-ethereum implementation") } secp256k1Instance = &(ecdsaContext{ curveP: curveP, curveN: curveN, - curveNdiv2: curveNdiv2, + curveNdiv2: new(big.Int).Rsh(curveN, 1), // (N-1)/2, since N is odd algo: ECDSASecp256k1, }) } @@ -80,6 +81,9 @@ type prKeyECDSASecp256k1 struct { *prKeyCommonECDSA // bytes(D) of private scalar D in big endian, padded to the curve order size (32 bytes) dBytes []byte + // pubKeyOnce guards the lazy construction of pubKey, + // making concurrent calls to PublicKey safe + pubKeyOnce sync.Once // public key pubKey *pubKeyECDSASecp256k1 } @@ -120,7 +124,7 @@ func privateKeyECDSASecp256k1(a *ecdsaContext, dBytes []byte) *prKeyECDSASecp256 // - (nil, error) if an unexpected error occurs // - (signature, nil) otherwise func (sk *prKeyECDSASecp256k1) Sign(msg []byte, hasher hash.Hasher) (Signature, error) { - hash, err := sk.checkAlgoAndComputeHash(msg, hasher) + hash, err := sk.checkHasherAndComputeHash(msg, hasher) if err != nil { return nil, err } @@ -131,8 +135,10 @@ func (sk *prKeyECDSASecp256k1) Sign(msg []byte, hasher hash.Hasher) (Signature, if err != nil { return nil, fmt.Errorf("failed to sign hash: %w", err) } - // remove the EC recover byte (last byte) - return signature[:SignatureLenECDSASecp256k1], nil + // remove the EC recover byte (last byte). + // The capacity is clipped so that an append to the returned signature + // cannot reach the recovery byte in the shared backing array. + return signature[:SignatureLenECDSASecp256k1:SignatureLenECDSASecp256k1], nil } // String returns the hex string representation of the private key @@ -190,14 +196,22 @@ func secp256k1PkBytes(x, y *big.Int) []byte { // PublicKey returns the public key associated to the private key func (sk *prKeyECDSASecp256k1) PublicKey() PublicKey { // construct the public key once - if sk.pubKey == nil { + sk.pubKeyOnce.Do(func() { x, y := secp256k1.S256().ScalarBaseMult(sk.dBytes) + // `ScalarBaseMult` returns nil coordinates only if the scalar is zero + // or not less than the curve order, + // which all constructors of `prKeyECDSASecp256k1` rule out. + // The check turns an unexpected invariant break into an explicit panic + // instead of a nil dereference inside `secp256k1PkBytes`. + if x == nil || y == nil { + panic("unexpected error: the private key scalar is invalid") + } sk.pubKey = &pubKeyECDSASecp256k1{ pubKeyCommonECDSA: &pubKeyCommonECDSA{secp256k1Instance}, pkBytes: secp256k1PkBytes(x, y), } - } + }) return sk.pubKey } @@ -215,7 +229,7 @@ func (sk *prKeyECDSASecp256k1) PublicKey() PublicKey { // - (false, error) if an unexpected error occurs // - (validity, nil) otherwise func (pk *pubKeyECDSASecp256k1) Verify(sig Signature, msg []byte, hasher hash.Hasher) (bool, error) { - hash, err := pk.checkAlgoAndComputeHash(msg, hasher) + hash, err := pk.checkHasherAndComputeHash(msg, hasher) if err != nil { return false, err } diff --git a/ecdsa_test.go b/ecdsa_test.go index 26f5f857..8c9e3c35 100644 --- a/ecdsa_test.go +++ b/ecdsa_test.go @@ -21,6 +21,8 @@ package crypto import ( "encoding/hex" "fmt" + "math/big" + "sync" "testing" crand "crypto/rand" @@ -70,6 +72,13 @@ func (d *dummyHasher) Write([]byte) (int, error) { return 0, nil } func (d *dummyHasher) SumHash() hash.Hash { return make([]byte, d.size) } func (d *dummyHasher) Reset() {} +// dishonestHasher declares a size but computes hashes one byte shorter, +// simulating a hash.Hasher implementation that breaks the interface contract +type dishonestHasher struct{ dummyHasher } + +func newDishonestHasher(size int) hash.Hasher { return &dishonestHasher{dummyHasher{size}} } +func (d *dishonestHasher) ComputeHash([]byte) hash.Hash { return make([]byte, d.size-1) } + func TestECDSAHasher(t *testing.T) { for _, curve := range ecdsaCurves { // generate a key pair @@ -110,6 +119,17 @@ func TestECDSAHasher(t *testing.T) { assert.Error(t, err) assert.True(t, IsInvalidHasherSizeError(err)) }) + + // hasher whose computed hash is shorter than its declared size + t.Run("dishonest hasher is rejected without a panic", func(t *testing.T) { + dummy := newDishonestHasher(32) + _, err := sk.Sign(seed, dummy) + assert.Error(t, err) + assert.True(t, IsInvalidHasherSizeError(err)) + _, err = sk.PublicKey().Verify(sig, seed, dummy) + assert.Error(t, err) + assert.True(t, IsInvalidHasherSizeError(err)) + }) } } @@ -524,3 +544,154 @@ func TestECDSAHighAndLowS(t *testing.T) { } }) } + +// Test function only to flip S in a signature. It is used for testing signature malleability +func (a *ecdsaContext) signatureFlipS(sig []byte) []byte { + // read S + nLen := bitsToBytes(a.curveN.BitLen()) + s := new(big.Int).SetBytes(sig[nLen:]) + // compute N-S + sComplement := new(big.Int).Sub(a.curveN, s) + // write it into a new signature + newSig := make([]byte, len(sig)) + copy(newSig, sig[:nLen]) // copy R + sComplement.FillBytes(newSig[nLen:]) // write S complement + return newSig +} + +// TestECDSASecp256k1DeterministicSigning checks deterministic ECDSA signatures +// on secp256k1 against RFC 6979 known-answer test vectors. +// The vectors are the community secp256k1/SHA-256 vectors +// replicated in trezor-crypto and python-ecdsa. +// The expected signatures are the low-S normalized (r || s) pairs. +func TestECDSASecp256k1DeterministicSigning(t *testing.T) { + vectors := []struct { + sk string + msg string + sig string + }{ + { + sk: "0000000000000000000000000000000000000000000000000000000000000001", + msg: "Satoshi Nakamoto", + sig: "934b1ea10a4b3c1757e2b0c017d0b6143ce3c9a7e6a4a49860d7a6ab210ee3d82442ce9d2b916064108014783e923ec36b49743e2ffa1c4496f01a512aafd9e5", + }, + { + // the private key is the curve order minus 1 + sk: "fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140", + msg: "Satoshi Nakamoto", + sig: "fd567d121db66e382991534ada77a6bd3106f0a1098c231e47993447cd6af2d06b39cd0eb1bc8603e159ef5c20a5c8ad685a45b06ce9bebed3f153d10d93bed5", + }, + { + sk: "f8b8af8ce3c7cca5e300d33939540c10d45ce001b8f252bfbc57ba0342904181", + msg: "Alan Turing", + sig: "7063ae83e7f62bbb171798131b4a0564b956930092b33b07b395615d9ec7e15c58dfcc1e00a35e1572f366ffe34ba0fc47db1e7189759b9fb233c5b05ab388ea", + }, + } + + for i, v := range vectors { + skBytes, err := hex.DecodeString(v.sk) + require.NoError(t, err) + sk, err := DecodePrivateKey(ECDSASecp256k1, skBytes) + require.NoError(t, err) + + sig, err := sk.Sign([]byte(v.msg), hash.NewSHA2_256()) + require.NoError(t, err) + assert.Equal(t, v.sig, hex.EncodeToString(sig), "vector %d", i) + + // the signature must verify under the matching public key + valid, err := sk.PublicKey().Verify(sig, []byte(v.msg), hash.NewSHA2_256()) + require.NoError(t, err) + assert.True(t, valid, "vector %d", i) + } +} + +// TestECDSAConcurrentPublicKey checks that concurrent calls to PublicKey +// on the same private key are safe and return equal keys. +// The test is effective when the race detector is enabled. +func TestECDSAConcurrentPublicKey(t *testing.T) { + for _, curve := range ecdsaCurves { + t.Run(curve.String(), func(t *testing.T) { + seed := make([]byte, KeyGenSeedMinLen) + _, err := crand.Read(seed) + require.NoError(t, err) + sk, err := GeneratePrivateKey(curve, seed) + require.NoError(t, err) + + pks := make([]PublicKey, 10) + var wg sync.WaitGroup + for i := range pks { + wg.Add(1) + go func() { + defer wg.Done() + pks[i] = sk.PublicKey() + }() + } + wg.Wait() + + for _, pk := range pks { + require.NotNil(t, pk) + assert.True(t, pk.Equals(pks[0])) + } + }) + } +} + +// TestECDSANilChecks covers the nil-related edge cases of the public API: +// decoding errors must return untyped nil interfaces, +// and Equals with a nil input must return false instead of panicking. +func TestECDSANilChecks(t *testing.T) { + t.Run("decode error paths return untyped nil interfaces", func(t *testing.T) { + for _, curve := range ecdsaCurves { + sk, err := DecodePrivateKey(curve, make([]byte, ecdsaPrKeyLen[curve]-1)) + require.Error(t, err) + // `assert.Nil` treats a typed-nil pointer inside an interface as nil, + // so compare against nil directly instead + assert.True(t, sk == nil) + + pk, err := DecodePublicKey(curve, make([]byte, ecdsaPubKeyLen[curve]-1)) + require.Error(t, err) + assert.True(t, pk == nil) + + pk, err = DecodePublicKeyCompressed(curve, make([]byte, ecdsaPubKeyLen[curve]-1)) + require.Error(t, err) + assert.True(t, pk == nil) + } + }) + + t.Run("Equals with a nil input returns false", func(t *testing.T) { + for _, curve := range ecdsaCurves { + seed := make([]byte, KeyGenSeedMinLen) + _, err := crand.Read(seed) + require.NoError(t, err) + sk, err := GeneratePrivateKey(curve, seed) + require.NoError(t, err) + assert.False(t, sk.Equals(nil)) + assert.False(t, sk.PublicKey().Equals(nil)) + } + }) +} + +// TestECDSASecp256k1CompressedDecoding checks compressed point decoding on secp256k1 +// using edge-case points where a generic (crypto/elliptic style) decompression +// either fails or computes a square root that doesn't match secp256k1 arithmetic. +func TestECDSASecp256k1CompressedDecoding(t *testing.T) { + testVectors := []string{ + "028b10bf56476bf7da39a3286e29df389177a2fa0fca2d73348ff78887515d8da1", // IsOnCurve for elliptic returns false + "03d39427f07f680d202fe8504306eb29041aceaf4b628c2c69b0ec248155443166", // odd, IsOnCurve for elliptic returns false + "0267d1942a6cbe4daec242ea7e01c6cdb82dadb6e7077092deb55c845bf851433e", // arith of sqrt in elliptic doesn't match secp256k1 + "0345d45eda6d087918b041453a96303b78c478dce89a4ae9b3c933a018888c5e06", // odd, arith of sqrt in elliptic doesn't match secp256k1 + } + + for _, testVector := range testVectors { + // get the compressed bytes + publicBytes, err := hex.DecodeString(testVector) + require.NoError(t, err) + + // decompress, check that those are perfectly valid secp256k1 public keys + retrieved, err := DecodePublicKeyCompressed(ECDSASecp256k1, publicBytes) + require.NoError(t, err) + + // check the compression is canonical by re-compressing to the same bytes + require.Equal(t, retrieved.EncodeCompressed(), publicBytes) + } +} From b44ebccbcf46f989d52ff28faefee28e8a5c18e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20M=C3=BCller?= Date: Tue, 11 Aug 2026 11:35:39 -0700 Subject: [PATCH 2/3] address review comments - 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 --- ecdsa.go | 93 ++++++++++++++++++----------------------- ecdsa_p256.go | 27 ++++-------- ecdsa_secp256k1.go | 66 +++++++++++------------------ ecdsa_test.go | 101 ++++----------------------------------------- sign_test_utils.go | 48 +++++++++++---------- 5 files changed, 108 insertions(+), 227 deletions(-) diff --git a/ecdsa.go b/ecdsa.go index ffbcfffd..b7d2c848 100644 --- a/ecdsa.go +++ b/ecdsa.go @@ -65,18 +65,15 @@ func (a *ecdsaContext) checkHasherAndComputeHash(msg []byte, hasher hash.Hasher) return nil, errNilHasher } - // check hasher's size is at least the curve order in bytes - nLen := bitsToBytes((a.curveN).BitLen()) - if hasher.Size() < nLen { - return nil, invalidHasherSizeErrorf( - "hasher's size should be at least %d bytes, got %d bytes", nLen, hasher.Size()) - } - h := hasher.ComputeHash(msg) - // guard against hasher implementations that compute fewer bytes - // than their declared size, - // since callers truncate the hash to the curve order size - // and would panic on a shorter slice + // check the computed hash is at least the curve order in bytes. + // All curve orders supported by the package have a bit-length multiple of 8, + // so callers truncate the message hash in bytes + // and the check is done in bytes too. + // The check uses the computed hash length rather than the hasher's declared size, + // so that a hasher implementation computing fewer bytes than it declares + // is rejected instead of panicking in the caller's truncation. + nLen := bitsToBytes((a.curveN).BitLen()) if len(h) < nLen { return nil, invalidHasherSizeErrorf( "hasher's output should be at least %d bytes, got %d bytes", nLen, len(h)) @@ -146,21 +143,23 @@ func (a *ecdsaContext) privateKey(d *big.Int) (PrivateKey, error) { d.FillBytes(dBytes) // dBytes is the big-endian encoding of d padded to the curve order // build the private key depending on the curve + var sk PrivateKey + var err error switch a.algo { case ECDSAP256: - sk, err := privateKeyECDSAP256(a, dBytes) - if err != nil { - // return an untyped nil, - // otherwise the returned interface is non-nil - // although it holds a nil pointer - return nil, err - } - return sk, nil + sk, err = privateKeyECDSAP256(a, dBytes) case ECDSASecp256k1: - return privateKeyECDSASecp256k1(a, dBytes), nil + sk = privateKeyECDSASecp256k1(a, dBytes) default: return nil, invalidInputsErrorf("the curve is not supported") } + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return sk, nil } // generatePrivateKey generates a private key for ECDSA @@ -235,28 +234,23 @@ func (a *ecdsaContext) decodePrivateKey(der []byte) (PrivateKey, error) { // Error Returns: // - invalidInputsError if the input is not a valid serialization of a public key on the given curve. func (a *ecdsaContext) rawDecodePublicKey(input []byte) (PublicKey, error) { + var pk PublicKey + var err error switch a.algo { case ECDSAP256: - pk, err := publicKeyECDSAP256(input) - if err != nil { - // return an untyped nil, - // otherwise the returned interface is non-nil - // although it holds a nil pointer - return nil, err - } - return pk, nil + pk, err = publicKeyECDSAP256(input) case ECDSASecp256k1: - pk, err := publicKeyECDSASecp256k1(a, input) - if err != nil { - // return an untyped nil, - // otherwise the returned interface is non-nil - // although it holds a nil pointer - return nil, err - } - return pk, nil + pk, err = publicKeyECDSASecp256k1(a, input) default: return nil, invalidInputsErrorf("curve is not supported") } + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil } func (a *ecdsaContext) decodePublicKey(der []byte) (PublicKey, error) { @@ -271,28 +265,23 @@ func (a *ecdsaContext) decodePublicKey(der []byte) (PublicKey, error) { // - invalidInputsError if the curve isn't supported or the input isn't a valid key serialization // on the given curve. func (a *ecdsaContext) decodePublicKeyCompressed(pkBytes []byte) (PublicKey, error) { + var pk PublicKey + var err error switch a.algo { case ECDSAP256: - pk, err := p256DecodePublicKeyCompressed(pkBytes) - if err != nil { - // return an untyped nil, - // otherwise the returned interface is non-nil - // although it holds a nil pointer - return nil, err - } - return pk, nil + pk, err = p256DecodePublicKeyCompressed(pkBytes) case ECDSASecp256k1: - pk, err := secp256k1DecodePublicKeyCompressed(pkBytes) - if err != nil { - // return an untyped nil, - // otherwise the returned interface is non-nil - // although it holds a nil pointer - return nil, err - } - return pk, nil + pk, err = secp256k1DecodePublicKeyCompressed(pkBytes) default: return nil, invalidInputsErrorf("the input curve is not supported") } + if err != nil { + // return an untyped nil, + // otherwise the returned interface is non-nil + // although it holds a nil pointer + return nil, err + } + return pk, nil } // Algorithm returns the algo related to the private key diff --git a/ecdsa_p256.go b/ecdsa_p256.go index 2f218277..8f6ade9e 100644 --- a/ecdsa_p256.go +++ b/ecdsa_p256.go @@ -24,7 +24,6 @@ import ( "crypto/rand" "fmt" "math/big" - "sync" "github.com/onflow/crypto/hash" ) @@ -66,9 +65,6 @@ type prKeyECDSAP256 struct { *prKeyCommonECDSA // go ecdsa standard lib private key goPrKey *ecdsa.PrivateKey - // pubKeyOnce guards the lazy construction of pubKey, - // making concurrent calls to PublicKey safe - pubKeyOnce sync.Once // public key pubKey *pubKeyECDSAP256 } @@ -94,7 +90,10 @@ func privateKeyECDSAP256(a *ecdsaContext, dBytes []byte) (*prKeyECDSAP256, error sk := &prKeyECDSAP256{ prKeyCommonECDSA: &prKeyCommonECDSA{a}, goPrKey: internalSK, - pubKey: nil, // public key is not constructed yet + pubKey: &pubKeyECDSAP256{ + pubKeyCommonECDSA: &pubKeyCommonECDSA{p256Instance}, + goPubKey: &internalSK.PublicKey, + }, } return sk, nil } @@ -133,20 +132,17 @@ func (sk *prKeyECDSAP256) String() string { // returns a publicKeyECDSAP256 from (bytes(x) || bytes(y)) bytes func publicKeyECDSAP256(XYBytes []byte) (*pubKeyECDSAP256, error) { - if len(XYBytes) != 2*pLenP256 { - return nil, invalidInputsErrorf("input has incorrect %s key size, got %d, expects %d", - ECDSAP256, len(XYBytes), 2*pLenP256) - } - // deserialization uses SEC1 version 2 (https://www.secg.org/sec1-v2.pdf section 2.3.3) // and includes on curve check. // The bytes serialization for non-infinity points is `0x04 || X || Y` and infinity point should be rejected anyway parsingBytes := append([]byte{ecEncodingUncompressed}, XYBytes...) - // ParseUncompressedPublicKey includes x

Date: Tue, 11 Aug 2026 14:06:23 -0500 Subject: [PATCH 3/3] keep TestECDSASecp256k1DeterministicSigning strictly about signing determinism --- ecdsa_test.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/ecdsa_test.go b/ecdsa_test.go index 91531b72..7fa3daa6 100644 --- a/ecdsa_test.go +++ b/ecdsa_test.go @@ -558,11 +558,11 @@ func (a *ecdsaContext) signatureFlipS(sig []byte) []byte { return newSig } -// TestECDSASecp256k1DeterministicSigning checks deterministic ECDSA signatures -// on secp256k1 against RFC 6979 known-answer test vectors. +// TestECDSASecp256k1DeterministicSigning checks the current ECDSA signatures +// on secp256k1 against RFC 6979 known test vectors. // The vectors are the community secp256k1/SHA-256 vectors // replicated in trezor-crypto and python-ecdsa. -// The expected signatures are the low-S normalized (r || s) pairs. +// The expected signatures are low-S normalized. // // The test only makes sense while the underlying implementation (currently go-ethereum) // uses RFC 6979 nonces and outputs low-S signatures. @@ -605,10 +605,5 @@ func TestECDSASecp256k1DeterministicSigning(t *testing.T) { sig, err := sk.Sign([]byte(v.msg), hash.NewSHA2_256()) require.NoError(t, err) assert.Equal(t, v.sig, hex.EncodeToString(sig), "vector %d", i) - - // the signature must verify under the matching public key - valid, err := sk.PublicKey().Verify(sig, []byte(v.msg), hash.NewSHA2_256()) - require.NoError(t, err) - assert.True(t, valid, "vector %d", i) } }