diff --git a/src/stores/connectors/ethereum/sign.go b/src/stores/connectors/ethereum/sign.go index 0a6257a77..916744bac 100644 --- 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 index 6930c3485..d8d31be38 100644 --- 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)