fix pub key decoding after downgrading go-eth crypto version and improve tests

Author: tarakby

README.md

@@ -65,8 +65,8 @@ All signature schemes use the generic interfaces of `PrivateKey` and `PublicKey`
 
  * ECDSA
     * public keys are compressed or uncompressed.
-    * ephemeral key is derived from the private key, hash and the system entropy (based on https://golang.org/pkg/crypto/ecdsa/).
     * supports NIST P-256 (secp256r1) and secp256k1 curves.
+    * For NIST P-256, ephemeral key is derived from the private key, hash and the system entropy (based on https://golang.org/pkg/crypto/ecdsa/). For secp256k1, ephemeral key is deterministically formed following RFC 6979 (based on github.com/ethereum/go-ethereum/crypto/secp256k1)
 
  * BLS
     * supports [BLS12-381](https://electriccoin.co/blog/new-snark-curve/) curve.

blst_src/README.md

@@ -21,7 +21,7 @@ The folder contains:
 To upgrade the BLST version:
 - [ ] audit all BLST updates, with focus on `<blst>/src`: https://github.com/supranational/blst/compare/v0.3.14...<new_version>
 - [ ] delete all files in this folder `./blst_src/` but `blst_src.c` and `README.md`.
-- [ ] delete all files in `./internal/blst/` but `non_cgo.go`.
+- [ ] delete all files in `./internal/blst/`.
 - [ ] open BLST repository on the new version.
 - [ ] copy all `.c` and `.h` files from `<blst>/src/` into `./blst_src/`.
 - [ ] delete newly copied `./blst_src/server.c`.

ecdsa.go

@@ -41,7 +41,7 @@ type ecdsaContext struct {
 	curveP *big.Int
 	// curve order
 	curveN *big.Int
-	// curve order minus 1 divided by 2 (used for signature malleability annalysis)
+	// curve order minus 1 divided by 2 (used for signature malleability analysis)
 	curveNdiv2 *big.Int
 }
 
@@ -122,7 +122,7 @@ func (a *ecdsaContext) mapToPrivateKey(seed []byte) (PrivateKey, error) {
 // privateKey returns an ECDSA private key using the
 // input scalar.
 //
-// Input scalar d is assumed to be satisfy 0 < d < n before calling this function.
+// Input scalar d is assumed to satisfy 0 < d < n before calling this function.
 //
 // The function returns:
 //   - (nil, invalidInputsError) if the curve is not supported
@@ -217,7 +217,7 @@ func (a *ecdsaContext) decodePrivateKey(der []byte) (PrivateKey, error) {
 func (a *ecdsaContext) rawDecodePublicKey(input []byte) (PublicKey, error) {
 	switch a.algo {
 	case ECDSAP256:
-		return publicKeyECDSAP256(a, input)
+		return publicKeyECDSAP256(input)
 	case ECDSASecp256k1:
 		return publicKeyECDSASecp256k1(a, input)
 	default:
@@ -330,7 +330,9 @@ func (a *ecdsaContext) isLowS(s *big.Int) bool {
 // (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.
-// The function checks that S is in the correct range [0, n-1] before normalizing it. If S is not in the correct range, the function returns a false boolean. (S will be checked against 0 in the verification function - check against N is inlcuded here)
+// The function checks that S is in the correct range [0, n-1] before normalizing it.
+// If S is not in the correct range, the function returns a false boolean.
+// (S will be checked against 0 in the verification function - check against N is inlcuded here)
 // returns:
 //   - newSig, true if S is in the valid range and was normalized to low S
 //   - nil, false if S was not in the correct range

ecdsa_p256.go

@@ -82,6 +82,7 @@ type pubKeyECDSAP256 struct {
 
 var _ PublicKey = (*pubKeyECDSAP256)(nil)
 
+// Input scalar d is assumed to satisfy 0 < d < n before calling this function.
 func privateKeyECDSAP256(a *ecdsaContext, dBytes []byte) (*prKeyECDSAP256, error) {
 	internalSK, err := ecdsa.ParseRawPrivateKey(elliptic.P256(), dBytes)
 	if err != nil {
@@ -128,12 +129,13 @@ func (sk *prKeyECDSAP256) String() string {
 }
 
 // returns a publicKeyECDSAP256 from (bytes(x) || bytes(y)) bytes
-func publicKeyECDSAP256(a *ecdsaContext, XYBytes []byte) (*pubKeyECDSAP256, error) {
+func publicKeyECDSAP256(XYBytes []byte) (*pubKeyECDSAP256, error) {
 	// 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<p and y<p checks, and on curve checks
 	internalPK, err := ecdsa.ParseUncompressedPublicKey(elliptic.P256(), parsingBytes)
 	if err != nil {
 		return nil, invalidInputsErrorf("input point has invalid coordinates or is not on curve: %w", err)

ecdsa_secp256k1.go

@@ -90,12 +90,14 @@ var _ PrivateKey = (*prKeyECDSASecp256k1)(nil)
 type pubKeyECDSASecp256k1 struct {
 	// ECDSA generic public key
 	*pubKeyCommonECDSA
-	// 0x4 || bytes(x) || bytes(y)  (65 bytes) where x and y are the coordinates of the public key point, padded to the field size (32 bytes)
+	// 0x4 || bytes(x) || bytes(y)  (65 bytes) where x and y are the coordinates of the public key point, padded to the field size (32 bytes).
+	// This is the form required by go-ethereum/crypto/secp256k1.
 	pkBytes []byte
 }
 
 var _ PublicKey = (*pubKeyECDSASecp256k1)(nil)
 
+// Input scalar d is assumed to be satisfy 0 < d < n before calling this function.
 func privateKeyECDSASecp256k1(a *ecdsaContext, dBytes []byte) *prKeyECDSASecp256k1 {
 	sk := &prKeyECDSASecp256k1{
 		prKeyCommonECDSA: &prKeyCommonECDSA{a},
@@ -148,14 +150,18 @@ func publicKeyECDSASecp256k1(a *ecdsaContext, XYBytes []byte) (*pubKeyECDSASecp2
 
 	x, y := readTwoBigInts(XYBytes, pLen)
 
-	// `IsOnCurve` includes checks for x<p and y<p
+	// check the coordinates are valid field elements (required for go-ethereum versions prior or equal to v1.16.8)
+	if x.Cmp(a.curveP) >= 0 || y.Cmp(a.curveP) >= 0 {
+		return nil, invalidInputsErrorf("at least one coordinate is larger than the field prime for %s", a.algo)
+	}
+
+	// `IsOnCurve` includes checks for x<p and y<p (in go-ethereum versions later than v1.16.9)
 	if !secp256k1.S256().IsOnCurve(x, y) {
 		return nil, invalidInputsErrorf("input point has invalid coordinates or is not on curve")
 	}
 	return &pubKeyECDSASecp256k1{
 		&pubKeyCommonECDSA{secp256k1Instance},
-		append([]byte{ecEncodingUncompressed}, XYBytes...),
-		// XYBytes is already the raw uncompressed encoding `bytes(x) || bytes(y)`
+		secp256k1PkBytes(x, y),
 	}, nil
 }
 
@@ -164,18 +170,27 @@ func (pk *pubKeyECDSASecp256k1) String() string {
 	return pubKeyCommonECDSAString(pk)
 }
 
+// 0x4 || bytes(x) || bytes(y)  (65 bytes) where x and y are the coordinates of the public key point, padded to the field size (32 bytes).
+// This is the form required by the underlying go-ethereum/crypto/secp256k1.
+//
+// The function assumes x and y are valid field elements and the point (x,y) is on curve
+func secp256k1PkBytes(x, y *big.Int) []byte {
+	pkBytes := make([]byte, 1+2*pLenSecp256k1)
+	pkBytes[0] = ecEncodingUncompressed
+	// pad x and y to the field size and concatenate them
+	padToSizeAndConcat(pkBytes[1:], x, y, pLenSecp256k1)
+	return pkBytes
+}
+
 // PublicKey returns the public key associated to the private key
 func (sk *prKeyECDSASecp256k1) PublicKey() PublicKey {
 	// construct the public key once
 	if sk.pubKey == nil {
 		x, y := secp256k1.S256().ScalarBaseMult(sk.dBytes)
-		pkBytes := make([]byte, 1+2*pLenSecp256k1)
-		pkBytes[0] = ecEncodingUncompressed
-		// pad x and y to the field size and concatenate them
-		padToSizeAndConcat(pkBytes[1:], x, y, pLenSecp256k1)
+
 		sk.pubKey = &pubKeyECDSASecp256k1{
 			pubKeyCommonECDSA: &pubKeyCommonECDSA{secp256k1Instance},
-			pkBytes:           pkBytes,
+			pkBytes:           secp256k1PkBytes(x, y),
 		}
 	}
 	return sk.pubKey
@@ -279,12 +294,9 @@ func secp256k1DecodePublicKeyCompressed(pkBytes []byte) (*pubKeyECDSASecp256k1,
 	if x == nil || y == nil {
 		return nil, invalidInputsErrorf("input %x isn't a compressed serialization of a point on secp256k1", pkBytes)
 	}
-	uncompressedPkBytes := make([]byte, 1+2*pLenSecp256k1)
-	uncompressedPkBytes[0] = ecEncodingUncompressed
-	padToSizeAndConcat(uncompressedPkBytes[1:], x, y, pLenSecp256k1)
 
 	return &pubKeyECDSASecp256k1{
 		&pubKeyCommonECDSA{secp256k1Instance},
-		uncompressedPkBytes,
+		secp256k1PkBytes(x, y),
 	}, nil
 }

ecdsa_test.go

@@ -185,24 +185,40 @@ func TestECDSAEncodeDecode(t *testing.T) {
 		//  - public key decoding handles input x-coordinates with x and y larger than p (doesn't result in an exception)
 		//  - public key decoding only accepts reduced x and y
 		t.Run("public key with non-reduced coordinates", func(t *testing.T) {
-			invalidPK1s := map[SigningAlgorithm]string{
-				ECDSASecp256k1: "0000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC30",
-				ECDSAP256:      "FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000",
+			onflowCryptoErr := "at least one coordinate is larger than the field prime"
+			goCryptoErr := "invalid P256 element encoding"
+
+			invalidPKs := []struct {
+				signin   SigningAlgorithm
+				pk       string
+				errorMsg string
+			}{
+				{
+					// x >= p
+					ECDSASecp256k1, "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F0000000000000000000000000000000000000000000000000000000000000000",
+					onflowCryptoErr,
+				}, {
+					ECDSAP256, "FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000",
+					goCryptoErr,
+				}, {
+					// y >= p
+					ECDSASecp256k1, "0000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC30",
+					onflowCryptoErr,
+				}, {
+					ECDSAP256, "0000000000000000000000000000000000000000000000000000000000000000FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF",
+					goCryptoErr,
+				},
 			}
-			invalidPK2s := map[SigningAlgorithm]string{
-				ECDSASecp256k1: "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F0000000000000000000000000000000000000000000000000000000000000000",
-				ECDSAP256:      "FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000",
+
+			for _, invalidPK := range invalidPKs {
+				pkBytes, err := hex.DecodeString(invalidPK.pk)
+				require.NoError(t, err)
+				pk, err := DecodePublicKey(invalidPK.signin, pkBytes)
+				require.Error(t, err)
+				assert.True(t, IsInvalidInputsError(err))
+				assert.ErrorContains(t, err, invalidPK.errorMsg)
+				assert.Nil(t, pk)
 			}
-			// invalidpk1 with x >= p
-			invalidPk1, err := hex.DecodeString(invalidPK1s[curve])
-			require.NoError(t, err)
-			_, err = DecodePublicKey(curve, invalidPk1)
-			assert.Error(t, err)
-			// invalidpk2 with y >= p
-			invalidPk2, err := hex.DecodeString(invalidPK2s[curve])
-			require.NoError(t, err)
-			_, err = DecodePublicKey(curve, invalidPk2)
-			assert.Error(t, err)
 		})
 	}
 }