diff --git a/CHANGELOG.md b/CHANGELOG.md index 15eeacea3..ae9d90e3a 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/auth/service/roles/roles.go b/src/auth/service/roles/roles.go index 88aabd524..9746fa0ef 100644 --- a/src/auth/service/roles/roles.go +++ b/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") } diff --git a/src/infra/log/zap/logger.go b/src/infra/log/zap/logger.go index de6af039b..fb00afb46 100644 --- a/src/infra/log/zap/logger.go +++ b/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 } diff --git a/src/stores/connectors/ethereum/sign.go b/src/stores/connectors/ethereum/sign.go index 0db03e689..10bf81b4e 100644 --- a/src/stores/connectors/ethereum/sign.go +++ b/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)...) } diff --git a/src/stores/connectors/ethereum/sign_test.go b/src/stores/connectors/ethereum/sign_test.go index dbf0c1d5c..e11d5cfc5 100644 --- a/src/stores/connectors/ethereum/sign_test.go +++ b/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,24 +56,21 @@ 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) { @@ -82,21 +78,14 @@ func TestSignMessage(t *testing.T) { 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) { diff --git a/tests/e2e/ethereum_test.go b/tests/e2e/ethereum_test.go index c5bd080e4..e83d16a24 100644 --- a/tests/e2e/ethereum_test.go +++ b/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)