Fix malleability issue on AKV by retrying (#515)

* done * done * done * done * 20 max retries * 10 max retries * lint * coverage
Consensys · Feb 16, 2022 · 9403b83 · 9403b83
1 parent 1381b18
commit 9403b83
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 28 deletions.
diff --git a/src/stores/connectors/ethereum/sign.go b/src/stores/connectors/ethereum/sign.go
@@ -7,6 +7,8 @@ import (
 	"encoding/base64"
 	"math/big"
 
+	"github.com/ethereum/go-ethereum/common/hexutil"
+
 	authtypes "github.com/consensys/quorum-key-manager/src/auth/entities"
 
 	"github.com/consensys/quorum-key-manager/pkg/errors"
@@ -21,8 +23,8 @@ import (
 )
 
 var (
-	secp256k1N, _  = new(big.Int).SetString("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16)
-	secp256k1halfN = new(big.Int).Div(secp256k1N, big.NewInt(2))
+	secp256k1halfN, _ = new(big.Int).SetString("7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0", 16)
+	maxRetries        = 10
 )
 
 func (c Connector) Sign(ctx context.Context, addr common.Address, data []byte) ([]byte, error) {
@@ -210,14 +212,31 @@ func (c Connector) sign(ctx context.Context, addr common.Address, data []byte) (
 		return nil, err
 	}
 
-	signature, err := c.store.Sign(ctx, acc.KeyID, data, ethAlgo)
-	if err != nil {
-		return nil, err
+	var signature []byte
+	var retry int
+	for retry = maxRetries; retry > 0; retry-- {
+		signature, err = c.store.Sign(ctx, acc.KeyID, data, ethAlgo)
+		if err != nil {
+			return nil, err
+		}
+
+		// If we get a malleable signature, we retry
+		if !isMalleableECDSASignature(signature) {
+			break
+		}
+
+		c.logger.Debug("malleable signature retrieved, retryng", "signature", hexutil.Encode(signature))
+	}
+
+	if retry == 0 {
+		errMessage := "failed to generate a non malleable signature"
+		c.logger.Error(errMessage)
+		return nil, errors.DependencyFailureError(errMessage)
 	}
 
 	// Recover the recID, please read: http://coders-errand.com/ecrecover-signature-verification-ethereum/
 	for _, recID := range []byte{0, 1} {
-		appendedSignature := append(malleabilityECDSASignature(signature), recID)
+		appendedSignature := append(signature, recID)
 		var recoveredPubKey *ecdsa.PublicKey
 		recoveredPubKey, err = crypto.SigToPub(data, appendedSignature)
 		if err != nil {
@@ -288,16 +307,6 @@ func getEncodedPrivateRecipient(privacyGroupID *string, privateFor *[]string) (i
 // https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys-details
 // More info about the issue: http://coders-errand.com/malleability-ecdsa-signatures/
 // More info about the fix: https://en.bitcoin.it/wiki/BIP_0062
-func malleabilityECDSASignature(signature []byte) []byte {
-	l := len(signature)
-	hl := l / 2
-
-	R := new(big.Int).SetBytes(signature[:hl])
-	S := new(big.Int).SetBytes(signature[hl:l])
-	if S.Cmp(secp256k1halfN) <= 0 {
-		return signature
-	}
-
-	S2 := new(big.Int).Sub(secp256k1N, S)
-	return append(R.Bytes(), S2.Bytes()...)
+func isMalleableECDSASignature(signature []byte) bool {
+	return new(big.Int).SetBytes(signature[32:]).Cmp(secp256k1halfN) >= 0
 }
diff --git a/src/stores/connectors/ethereum/sign_test.go b/src/stores/connectors/ethereum/sign_test.go
@@ -43,33 +43,34 @@ func TestSignMessage(t *testing.T) {
 
 	t.Run("should sign successfully", func(t *testing.T) {
 		acc := testutils2.FakeETHAccount()
-		ecdsaSignature := hexutil.MustDecode("0xe276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deee57a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4")
 		acc.PublicKey = hexutil.MustDecode("0x04e2e7621c0c08e43905648be731a482e8eb3d3186023335812f52130e4a18dd729b22d88fbf0f22b8fa4390267ef0c54367dc638a25b38ea74290bdb9f79ff917")
+		ecdsaSignature := hexutil.MustDecode("0xe276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deee57a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4")
+		expectedSignature := hexutil.Encode(ecdsaSignature) + "1b"
 
 		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(nil)
 		db.EXPECT().Get(gomock.Any(), acc.Address.Hex()).Return(acc, nil)
 		store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignature, nil)
 
-		expectedSignature := hexutil.Encode(ecdsaSignature) + "1b"
 		signature, err := connector.SignMessage(ctx, acc.Address, data)
 
 		require.NoError(t, err)
 		assert.Equal(t, hexutil.Encode(signature), expectedSignature)
 	})
 
-	t.Run("should sign and convert malleable signature successfully", func(t *testing.T) {
+	t.Run("should retry to sign if signature is malleable successfully", func(t *testing.T) {
 		acc := testutils2.FakeETHAccount()
-		R, _ := new(big.Int).SetString("63341e2c837449de3735b6f4402b154aa0a118d02e45a2b311fba39c444025dd", 16)
-		S, _ := new(big.Int).SetString("39db7699cb3d8a5caf7728a87e778c2cdccc4085cf2a346e37c1823dec5ce2ed", 16)
-		S2 := new(big.Int).Add(S, secp256k1N)
-		ecdsaSignatureMalleable := append(R.Bytes(), S2.Bytes()...)
-		acc.PublicKey = hexutil.MustDecode("0x048e12a3333368c1c4c7c8fe9d1fd172444f6de328cd1650b76836928be9e71e952f61d75d87133f881bf4d25e02c3147aebf69abf7587c39cb5d177693ce1ca8f")
+		malleableSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc9ff4be64347b5fb58d355eb261fff1f1e284608a2d259ca7e6041c2b829bb4802")
+		acc.PublicKey = hexutil.MustDecode("0x04e2e7621c0c08e43905648be731a482e8eb3d3186023335812f52130e4a18dd729b22d88fbf0f22b8fa4390267ef0c54367dc638a25b38ea74290bdb9f79ff917")
+		ecdsaSignature := hexutil.MustDecode("0xe276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deee57a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4")
+		expectedSignature := hexutil.Encode(ecdsaSignature) + "1b"
 
 		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(nil)
 		db.EXPECT().Get(gomock.Any(), acc.Address.Hex()).Return(acc, nil)
-		store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignatureMalleable, nil)
+		gomock.InOrder(
+			store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(malleableSignature, nil),
+			store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignature, nil),
+		)
 
-		expectedSignature := hexutil.Encode(append(R.Bytes(), S.Bytes()...)) + "1c"
 		signature, err := connector.SignMessage(ctx, acc.Address, data)
 
 		require.NoError(t, err)
@@ -166,6 +167,33 @@ func TestSignTransaction(t *testing.T) {
 		assert.Equal(t, "0xf85d80808094905b88eff8bda1543d4d6f4aa05afef143d27e18808025a0e276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deeea057a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4", hexutil.Encode(signedRaw))
 	})
 
+	t.Run("should sign a payload successfully if signature is malleable", func(t *testing.T) {
+		malleableSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc9ff4be64347b5fb58d355eb261fff1f1e284608a2d259ca7e6041c2b829bb4802")
+
+		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(nil)
+		db.EXPECT().Get(ctx, acc.Address.Hex()).Return(acc, nil)
+		gomock.InOrder(
+			store.EXPECT().Sign(ctx, acc.KeyID, types.NewEIP155Signer(chainID).Hash(tx).Bytes(), ethAlgo).Return(malleableSignature, nil),
+			store.EXPECT().Sign(ctx, acc.KeyID, types.NewEIP155Signer(chainID).Hash(tx).Bytes(), ethAlgo).Return(ecdsaSignature, nil),
+		)
+
+		signedRaw, err := connector.SignTransaction(ctx, acc.Address, chainID, tx)
+		assert.NoError(t, err)
+		assert.Equal(t, "0xf85d80808094905b88eff8bda1543d4d6f4aa05afef143d27e18808025a0e276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deeea057a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4", hexutil.Encode(signedRaw))
+	})
+
+	t.Run("should fail with DependencyError if we obtain a malleable signature maxRetries times", func(t *testing.T) {
+		malleableSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc9ff4be64347b5fb58d355eb261fff1f1e284608a2d259ca7e6041c2b829bb4802")
+
+		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(nil)
+		db.EXPECT().Get(ctx, acc.Address.Hex()).Return(acc, nil)
+		store.EXPECT().Sign(ctx, acc.KeyID, types.NewEIP155Signer(chainID).Hash(tx).Bytes(), ethAlgo).Return(malleableSignature, nil).Times(maxRetries)
+
+		signedRaw, err := connector.SignTransaction(ctx, acc.Address, chainID, tx)
+		assert.Nil(t, signedRaw)
+		assert.True(t, errors.IsDependencyFailureError(err))
+	})
+
 	t.Run("should fail with same error if authorization fails", func(t *testing.T) {
 		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(expectedErr)