fix: padding occuring when applying malleability (#541)

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Quorum Key Manager Release Notes
 
+## v21.12.4 (2022-3-02)
+### 🛠 Bug fixes
+* Fix padding issue with malleable ECDSA signatures
+
+## v21.12.3 (2022-2-24)
+### 🛠 Bug fixes
+* Mathematically transform malleable ECDSA signatures into non-malleable signatures.
+
 ## v21.12.2 (2022-2-16)
 ### 🆕 Features
 * Support for OIDC token custom claims `AUTH_OIDC_CUSTOM_CLAIMS` for tenant_id and permissions.

src/auth/service/roles/roles.go

@@ -46,7 +46,5 @@ func (i *Roles) getRole(_ context.Context, name string) (*entities.Role, error)
 		return role, nil
 	}
 
-	errMessage := "role was not found"
-	i.logger.Warn(errMessage, "name", name)
-	return nil, errors.NotFoundError(errMessage)
+	return nil, errors.NotFoundError("role was not found")
 }

src/infra/log/zap/logger.go

@@ -87,21 +87,7 @@ func (l Logger) WithComponent(component string) log.Logger {
 }
 
 func (l *Logger) Write(p []byte) (n int, err error) {
-	switch l.cfg.Level {
-	case DebugLevel:
-		l.Debug(string(p))
-	case InfoLevel:
-		l.Info(string(p))
-	case WarnLevel:
-		l.Warn(string(p))
-	case ErrorLevel:
-		l.Error(string(p))
-	case PanicLevel:
-		l.Panic(string(p))
-	default:
-		l.Info(string(p))
-	}
-
+	l.Info(string(p)) // Only used for access logs so it's always INFO
 	return 0, nil
 }
 

src/stores/connectors/ethereum/sign.go

@@ -24,7 +24,6 @@ import (
 var (
 	secp256k1N, _     = new(big.Int).SetString("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16)
 	secp256k1halfN, _ = new(big.Int).SetString("7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0", 16)
-	maxRetries        = 3
 )
 
 func (c Connector) Sign(ctx context.Context, addr common.Address, data []byte) ([]byte, error) {
@@ -202,6 +201,8 @@ func (c Connector) SignPrivate(ctx context.Context, addr common.Address, tx *quo
 }
 
 func (c Connector) sign(ctx context.Context, addr common.Address, data []byte) ([]byte, error) {
+	logger := c.logger.With("address", addr.Hex())
+
 	err := c.authorizator.CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount})
 	if err != nil {
 		return nil, err
@@ -212,36 +213,29 @@ func (c Connector) sign(ctx context.Context, addr common.Address, data []byte) (
 		return nil, err
 	}
 
-	for retry := maxRetries; retry > 0; retry-- {
-		signature, err := c.store.Sign(ctx, acc.KeyID, data, ethAlgo)
+	signature, err := c.store.Sign(ctx, acc.KeyID, data, ethAlgo)
+	if err != nil {
+		return nil, err
+	}
+	signature = malleabilityECDSASignature(signature)
+
+	// Recover the recID, please read: http://coders-errand.com/ecrecover-signature-verification-ethereum/
+	for _, recID := range []byte{0, 1} {
+		appendedSignature := append(signature, recID)
+		recoveredPubKey, err := crypto.SigToPub(data, appendedSignature)
 		if err != nil {
-			return nil, err
+			errMessage := "failed to recover public key candidate with appended recID"
+			logger.WithError(err).Error(errMessage, "recID", recID, "signature", hexutil.Encode(signature))
+			return nil, errors.CryptoOperationError(errMessage)
 		}
 
-		signature = malleabilityECDSASignature(signature)
-
-		// Recover the recID, please read: http://coders-errand.com/ecrecover-signature-verification-ethereum/
-		for _, recID := range []byte{0, 1} {
-			appendedSignature := append(signature, recID)
-			recoveredPubKey, err := crypto.SigToPub(data, appendedSignature)
-			if err != nil {
-				errMessage := "failed to recover public key candidate with appended recID"
-				c.logger.WithError(err).Debug(errMessage, "recID", recID)
-
-				// If we fail to recover the pubkey, we continue and get a new signature
-				break
-			}
-
-			if bytes.Equal(crypto.FromECDSAPub(recoveredPubKey), acc.PublicKey) {
-				return appendedSignature, nil
-			}
+		if bytes.Equal(crypto.FromECDSAPub(recoveredPubKey), acc.PublicKey) {
+			return appendedSignature, nil
 		}
-
-		c.logger.Debug("malleable signature retrieved, retrying", "signature", hexutil.Encode(signature))
 	}
 
 	errMessage := "failed to recover public key candidate"
-	c.logger.Error(errMessage)
+	logger.Error(errMessage)
 	return nil, errors.DependencyFailureError(errMessage)
 }
 
@@ -304,5 +298,5 @@ func malleabilityECDSASignature(signature []byte) []byte {
 	}
 
 	S2 := new(big.Int).Sub(secp256k1N, S)
-	return append(signature[:32], S2.Bytes()...)
+	return append(signature[:32], common.LeftPadBytes(S2.Bytes(), 32)...)
 }

src/stores/connectors/ethereum/sign_test.go

@@ -7,7 +7,6 @@ import (
 	"testing"
 
 	common2 "github.com/consensys/quorum-key-manager/pkg/common"
-	"github.com/consensys/quorum-key-manager/pkg/errors"
 	"github.com/consensys/quorum-key-manager/pkg/ethereum"
 	authtypes "github.com/consensys/quorum-key-manager/src/auth/entities"
 	mock3 "github.com/consensys/quorum-key-manager/src/auth/mock"
@@ -57,46 +56,36 @@ func TestSignMessage(t *testing.T) {
 		assert.Equal(t, hexutil.Encode(signature), expectedSignature)
 	})
 
-	t.Run("should retry to sign if signature is malleable successfully", func(t *testing.T) {
+	t.Run("should sign if signature is malleable successfully", func(t *testing.T) {
 		acc := testutils2.FakeETHAccount()
 		malleableSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc9ff4be64347b5fb58d355eb261fff1f1e284608a2d259ca7e6041c2b829bb4802")
-		acc.PublicKey = hexutil.MustDecode("0x04e2e7621c0c08e43905648be731a482e8eb3d3186023335812f52130e4a18dd729b22d88fbf0f22b8fa4390267ef0c54367dc638a25b38ea74290bdb9f79ff917")
-		ecdsaSignature := hexutil.MustDecode("0xe276fd7524ed7af67b7f914de5be16fad6b9038009d2d78f2315351fbd48deee57a897964e80e041c674942ef4dbd860cb79a6906fb965d5e4645f5c44f7eae4")
-		expectedSignature := hexutil.Encode(ecdsaSignature) + "1b"
+		acc.PublicKey = hexutil.MustDecode("0x04d7c03955db8a8fa33dd2b8cf4a4a97b09bb744c77aa8e8ea48b2a3fdc0be2624ad2d26e672f0d81b96223e4aa7e8e92142b6adc583bda3b35684f4b614bf8e69")
+		ecdsaSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc900b419bcb84a04a72caa14d9e000e0e09268d443dceed5bd5f909bd4a67af93f")
+		expectedSignature := hexutil.Encode(ecdsaSignature) + "1c"
 
 		auth.EXPECT().CheckPermission(&authtypes.Operation{Action: authtypes.ActionSign, Resource: authtypes.ResourceEthAccount}).Return(nil)
 		db.EXPECT().Get(gomock.Any(), acc.Address.Hex()).Return(acc, 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),
-		)
+		store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(malleableSignature, nil)
 
 		signature, err := connector.SignMessage(ctx, acc.Address, data)
 
 		require.NoError(t, err)
-		assert.Equal(t, hexutil.Encode(signature), expectedSignature)
+		assert.Equal(t, expectedSignature, hexutil.Encode(signature))
 	})
 
 	t.Run("should fail to sign if address is not recoverable", func(t *testing.T) {
 		R, _ := new(big.Int).SetString("63341e2c837449de3735b6f4402b154aa0a118d02e45a2b311fba39c444025dd", 16)
 		S, _ := new(big.Int).SetString("39db7699cb3d8a5caf7728a87e778c2cdccc4085cf2a346e37c1823dec5ce2ed", 16)
 		ecdsaSignatureNonRecoverable := append(R.Bytes(), S.Bytes()...)
 		acc := testutils2.FakeETHAccount()
-		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)
-		gomock.InOrder(
-			store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignatureNonRecoverable, nil),
-			store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignature, nil),
-		)
+		store.EXPECT().Sign(gomock.Any(), acc.KeyID, crypto.Keccak256([]byte(expectedData)), ethAlgo).Return(ecdsaSignatureNonRecoverable, nil)
 
 		signature, err := connector.SignMessage(ctx, acc.Address, data)
-
-		require.NoError(t, err)
-		assert.Equal(t, hexutil.Encode(signature), expectedSignature)
+		assert.Empty(t, signature)
+		assert.Error(t, err)
 	})
 
 	t.Run("should fail with same error if authorization fails", func(t *testing.T) {
@@ -173,30 +162,18 @@ func TestSignTransaction(t *testing.T) {
 	})
 
 	t.Run("should sign a payload successfully if signature is malleable", func(t *testing.T) {
-		malleableSignature := hexutil.MustDecode("0x4eea3840a056c717a02f3b73229416d48696cbedd16627a47e9e4e7ba8063cc9ff4be64347b5fb58d355eb261fff1f1e284608a2d259ca7e6041c2b829bb4802")
+		// This signature is malleable (s > n/2) and also needs padding (s' = n - s ; s' < 32bytes)
+		malleableSignature := hexutil.MustDecode("0x0c07c6f83969949f14a6b48a65fc13abe7b72637c88ce2be836659fe40e03440e90310a9c60b3fab63c579b2c956b19cb4bd3a11259d9447783530c485763000")
+		account := testutils2.FakeETHAccount()
+		account.PublicKey = hexutil.MustDecode("0x0455a3406df13f78f80a6f574577b9b80f52665ac045106c1c8918fefa4b77a21db9aa721d0cbd54fc5d20fbaf39b5457a04af06d7e315755f7036274458ce08e3")
 
 		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),
-		)
+		db.EXPECT().Get(ctx, acc.Address.Hex()).Return(account, nil)
+		gomock.InOrder(store.EXPECT().Sign(ctx, account.KeyID, types.NewEIP155Signer(chainID).Hash(tx).Bytes(), ethAlgo).Return(malleableSignature, nil))
 
-		signedRaw, err := connector.SignTransaction(ctx, acc.Address, chainID, tx)
+		signedRaw, err := connector.SignTransaction(ctx, account.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))
+		assert.Equal(t, "0xf85d80808094905b88eff8bda1543d4d6f4aa05afef143d27e18808025a00c07c6f83969949f14a6b48a65fc13abe7b72637c88ce2be836659fe40e03440a016fcef5639f4c0549c3a864d36a94e6205f1a2d589ab0bf4479d2dc84ac01141", hexutil.Encode(signedRaw))
 	})
 
 	t.Run("should fail with same error if authorization fails", func(t *testing.T) {

tests/e2e/ethereum_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/consensys/quorum-key-manager/pkg/client"
 	"github.com/consensys/quorum-key-manager/pkg/common"
@@ -45,8 +46,8 @@ func TestKeyManagerEth(t *testing.T) {
 	require.NoError(t, err)
 	s.env = env
 
-	if len(s.env.cfg.SecretStores) == 0 {
-		t.Error("list of secret stores cannot be empty")
+	if len(s.env.cfg.EthStores) == 0 {
+		t.Error("list of ethereum stores cannot be empty")
 		return
 	}
 
@@ -61,19 +62,18 @@ func TestKeyManagerEth(t *testing.T) {
 }
 
 func (s *ethTestSuite) SetupSuite() {
-	if s.err != nil {
-		s.T().Error(s.err)
-	}
-
 	var err error
 	s.signAccount, err = s.env.client.CreateEthAccount(s.env.ctx, s.storeName, testutils.FakeCreateEthAccountRequest())
 	require.NoError(s.T(), err)
 }
 
 func (s *ethTestSuite) TearDownSuite() {
-	if s.err != nil {
-		s.T().Error(s.err)
-	}
+	err := s.env.client.DeleteEthAccount(s.env.ctx, s.storeName, s.signAccount.Address.Hex())
+	require.NoError(s.T(), err)
+
+	time.Sleep(100 * time.Millisecond)
+
+	_ = s.env.client.DestroyEthAccount(s.env.ctx, s.storeName, s.signAccount.Address.Hex())
 }
 
 func (s *ethTestSuite) TestCreate() {
@@ -83,6 +83,7 @@ func (s *ethTestSuite) TestCreate() {
 
 		acc, err := s.env.client.CreateEthAccount(s.env.ctx, s.storeName, request)
 		require.NoError(s.T(), err)
+		defer s.queueToDelete(acc)
 
 		assert.NotEmpty(s.T(), acc.Address)
 		assert.NotEmpty(s.T(), acc.PublicKey)
@@ -100,6 +101,7 @@ func (s *ethTestSuite) TestCreate() {
 
 		acc, err := s.env.client.CreateEthAccount(s.env.ctx, s.storeName, request)
 		require.NoError(s.T(), err)
+		defer s.queueToDelete(acc)
 
 		assert.NotEmpty(s.T(), acc.Address)
 		assert.NotEmpty(s.T(), acc.PublicKey)
@@ -152,6 +154,7 @@ func (s *ethTestSuite) TestImport() {
 
 		acc, err := s.env.client.ImportEthAccount(s.env.ctx, s.storeName, request)
 		require.NoError(s.T(), err)
+		defer s.queueToDelete(acc)
 
 		assert.NotEmpty(s.T(), acc.Address)
 		assert.NotEmpty(s.T(), acc.PublicKey)