From df2f89e6b6bbb9b40c53b4c158f328a3b4779d39 Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Thu, 9 Nov 2023 00:35:53 -0500 Subject: [PATCH] Correct double hashing issue Signed-off-by: Peter Broadhurst --- mocks/ethsignermocks/wallet.go | 2 +- mocks/rpcbackendmocks/backend.go | 2 +- mocks/rpcservermocks/server.go | 2 +- mocks/secp256k1mocks/signer.go | 28 +++++++++++- pkg/ethsigner/typed_data.go | 15 +++---- pkg/ethsigner/typed_data_test.go | 74 +++++++++++++++++++++++++++++++- pkg/secp256k1/signer.go | 27 +++++++++--- 7 files changed, 129 insertions(+), 21 deletions(-) diff --git a/mocks/ethsignermocks/wallet.go b/mocks/ethsignermocks/wallet.go index 58889f13..3e59b487 100644 --- a/mocks/ethsignermocks/wallet.go +++ b/mocks/ethsignermocks/wallet.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.35.4. DO NOT EDIT. +// Code generated by mockery v2.36.0. DO NOT EDIT. package ethsignermocks diff --git a/mocks/rpcbackendmocks/backend.go b/mocks/rpcbackendmocks/backend.go index 00bb48f4..9d03542c 100644 --- a/mocks/rpcbackendmocks/backend.go +++ b/mocks/rpcbackendmocks/backend.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.35.4. DO NOT EDIT. +// Code generated by mockery v2.36.0. DO NOT EDIT. package rpcbackendmocks diff --git a/mocks/rpcservermocks/server.go b/mocks/rpcservermocks/server.go index 02b12127..36090de0 100644 --- a/mocks/rpcservermocks/server.go +++ b/mocks/rpcservermocks/server.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.35.4. DO NOT EDIT. +// Code generated by mockery v2.36.0. DO NOT EDIT. package rpcservermocks diff --git a/mocks/secp256k1mocks/signer.go b/mocks/secp256k1mocks/signer.go index 5ab6e3b0..d52c4f28 100644 --- a/mocks/secp256k1mocks/signer.go +++ b/mocks/secp256k1mocks/signer.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.35.4. DO NOT EDIT. +// Code generated by mockery v2.36.0. DO NOT EDIT. package secp256k1mocks @@ -38,6 +38,32 @@ func (_m *Signer) Sign(message []byte) (*secp256k1.SignatureData, error) { return r0, r1 } +// SignDirect provides a mock function with given fields: message +func (_m *Signer) SignDirect(message []byte) (*secp256k1.SignatureData, error) { + ret := _m.Called(message) + + var r0 *secp256k1.SignatureData + var r1 error + if rf, ok := ret.Get(0).(func([]byte) (*secp256k1.SignatureData, error)); ok { + return rf(message) + } + if rf, ok := ret.Get(0).(func([]byte) *secp256k1.SignatureData); ok { + r0 = rf(message) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*secp256k1.SignatureData) + } + } + + if rf, ok := ret.Get(1).(func([]byte) error); ok { + r1 = rf(message) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // NewSigner creates a new instance of Signer. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewSigner(t interface { diff --git a/pkg/ethsigner/typed_data.go b/pkg/ethsigner/typed_data.go index 55324a48..4d7b7614 100644 --- a/pkg/ethsigner/typed_data.go +++ b/pkg/ethsigner/typed_data.go @@ -21,7 +21,6 @@ import ( "github.com/hyperledger/firefly-signer/pkg/eip712" "github.com/hyperledger/firefly-signer/pkg/ethtypes" - "github.com/hyperledger/firefly-signer/pkg/rlp" "github.com/hyperledger/firefly-signer/pkg/secp256k1" ) @@ -38,20 +37,20 @@ func SignTypedDataV4(ctx context.Context, signer secp256k1.Signer, payload *eip7 if err != nil { return nil, err } - sig, err := signer.Sign(encodedData) + // Note that signer.Sign performs the hash + sig, err := signer.SignDirect(encodedData) if err != nil { return nil, err } - rlpList := make(rlp.List, 0, 4) - rlpList = append(rlpList, rlp.Data(encodedData)) - rlpList = append(rlpList, rlp.WrapInt(sig.R)) - rlpList = append(rlpList, rlp.WrapInt(sig.S)) - rlpList = append(rlpList, rlp.WrapInt(sig.V)) + signatureBytes := make([]byte, 65) + signatureBytes[0] = byte(sig.V.Int64()) + sig.R.FillBytes(signatureBytes[1:33]) + sig.S.FillBytes(signatureBytes[33:65]) return &EIP712Result{ Hash: encodedData, - Signature: rlpList.Encode(), // per eth_signTypedData_v4 convention + Signature: signatureBytes, // compact ECDSA signature V: ethtypes.HexInteger(*sig.V), R: sig.R.FillBytes(make([]byte, 32)), S: sig.S.FillBytes(make([]byte, 32)), diff --git a/pkg/ethsigner/typed_data_test.go b/pkg/ethsigner/typed_data_test.go index 0208eeee..6bdfdaee 100644 --- a/pkg/ethsigner/typed_data_test.go +++ b/pkg/ethsigner/typed_data_test.go @@ -23,11 +23,13 @@ import ( "math/big" "testing" + "github.com/btcsuite/btcd/btcec/v2/ecdsa" "github.com/hyperledger/firefly-common/pkg/log" "github.com/hyperledger/firefly-signer/mocks/secp256k1mocks" "github.com/hyperledger/firefly-signer/pkg/eip712" "github.com/hyperledger/firefly-signer/pkg/ethtypes" "github.com/hyperledger/firefly-signer/pkg/secp256k1" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -58,7 +60,7 @@ func TestSignTypedDataV4(t *testing.T) { foundSig.S.SetBytes(sig.S) signaturePayload := ethtypes.HexBytes0xPrefix(sig.Hash) - addr, err := foundSig.Recover(signaturePayload, -1 /* chain id is in the domain (not applied EIP-155 style to the V value) */) + addr, err := foundSig.RecoverDirect(signaturePayload, -1 /* chain id is in the domain (not applied EIP-155 style to the V value) */) assert.NoError(t, err) assert.Equal(t, keypair.Address.String(), addr.String()) @@ -91,9 +93,77 @@ func TestSignTypedDataV4SignFail(t *testing.T) { } msn := &secp256k1mocks.Signer{} - msn.On("Sign", mock.Anything).Return(nil, fmt.Errorf("pop")) + msn.On("SignDirect", mock.Anything).Return(nil, fmt.Errorf("pop")) ctx := context.Background() _, err := SignTypedDataV4(ctx, msn, payload) assert.Regexp(t, "pop", err) } + +func TestMessage_2(t *testing.T) { + logrus.SetLevel(logrus.TraceLevel) + + var p eip712.TypedData + err := json.Unmarshal([]byte(`{ + "domain": { + "name": "test-app", + "version": "1", + "chainId": 31337, + "verifyingContract": "0x9fe46736679d2d9a65f0992f2272de9f3c7fa6e0" + }, + "types": { + "Issuance": [ + { + "name": "amount", + "type": "uint256" + }, + { + "name": "to", + "type": "address" + } + ], + "EIP712Domain": [ + { + "name": "name", + "type": "string" + }, + { + "name": "version", + "type": "string" + }, + { + "name": "chainId", + "type": "uint256" + }, + { + "name": "verifyingContract", + "type": "address" + } + ] + }, + "primaryType": "Issuance", + "message": { + "amount": "1000", + "to": "0xce3a47d24140cca16f8839357ca5fada44a1baef" + } + }`), &p) + assert.NoError(t, err) + + ctx := context.Background() + ed, err := eip712.EncodeTypedDataV4(ctx, &p) + assert.NoError(t, err) + assert.Equal(t, "0xb0132202fa81cafac0e405917f86705728ba02912d185065697cc4ba4e61aec3", ed.String()) + + keys, err := secp256k1.NewSecp256k1KeyPair([]byte(`8d01666832be7eb2dbd57cd3d4410d0231a91533f895de76d0930c689618aefd`)) + assert.NoError(t, err) + assert.Equal(t, "0xbcef501facf72ddacdb055acc2716786ff038728", keys.Address.String()) + + signed, err := SignTypedDataV4(ctx, keys, &p) + assert.NoError(t, err) + + assert.Equal(t, "0xb0132202fa81cafac0e405917f86705728ba02912d185065697cc4ba4e61aec3", signed.Hash.String()) + + pubKey, _, err := ecdsa.RecoverCompact(signed.Signature, signed.Hash) + assert.NoError(t, err) + assert.Equal(t, "0xbcef501facf72ddacdb055acc2716786ff038728", secp256k1.PublicKeyToAddress(pubKey).String()) +} diff --git a/pkg/secp256k1/signer.go b/pkg/secp256k1/signer.go index 4094ed01..06e22304 100644 --- a/pkg/secp256k1/signer.go +++ b/pkg/secp256k1/signer.go @@ -1,4 +1,4 @@ -// Copyright © 2022 Kaleido, Inc. +// Copyright © 2023 Kaleido, Inc. // // SPDX-License-Identifier: Apache-2.0 // @@ -34,6 +34,7 @@ type SignatureData struct { // Signer is the low level common interface that can be implemented by any module which provides signature capability type Signer interface { Sign(message []byte) (*SignatureData, error) + SignDirect(message []byte) (*SignatureData, error) } // getVNormalized returns the original 27/28 parity @@ -67,10 +68,16 @@ func (s *SignatureData) UpdateEIP2930() { s.V = s.V.Sub(s.V, big.NewInt(27)) } -// Recover obtains the original signer +// Recover obtains the original signer from the hash of the message func (s *SignatureData) Recover(message []byte, chainID int64) (a *ethtypes.Address0xHex, err error) { msgHash := sha3.NewLegacyKeccak256() msgHash.Write(message) + return s.RecoverDirect(msgHash.Sum(nil), chainID) +} + +// Recover obtains the original signer +func (s *SignatureData) RecoverDirect(message []byte, chainID int64) (a *ethtypes.Address0xHex, err error) { + signatureBytes := make([]byte, 65) signatureBytes[0], err = s.getVNormalized(chainID) if err != nil { @@ -78,21 +85,27 @@ func (s *SignatureData) Recover(message []byte, chainID int64) (a *ethtypes.Addr } s.R.FillBytes(signatureBytes[1:33]) s.S.FillBytes(signatureBytes[33:65]) - pubKey, _, err := ecdsa.RecoverCompact(signatureBytes, msgHash.Sum(nil)) // uses S256() by default + pubKey, _, err := ecdsa.RecoverCompact(signatureBytes, message) // uses S256() by default if err != nil { return nil, err } return PublicKeyToAddress(pubKey), nil } -// Sign performs raw signing - give legacy 27/28 V values +// Sign hashes the input then signs it func (k *KeyPair) Sign(message []byte) (ethSig *SignatureData, err error) { + msgHash := sha3.NewLegacyKeccak256() + msgHash.Write(message) + hashed := msgHash.Sum(nil) + return k.SignDirect(hashed) +} + +// SignDirect performs raw signing - give legacy 27/28 V values +func (k *KeyPair) SignDirect(message []byte) (ethSig *SignatureData, err error) { if k == nil { return nil, fmt.Errorf("nil signer") } - msgHash := sha3.NewLegacyKeccak256() - msgHash.Write(message) - sig, err := ecdsa.SignCompact(k.PrivateKey, msgHash.Sum(nil), false) // uses S256() by default + sig, err := ecdsa.SignCompact(k.PrivateKey, message, false) // uses S256() by default if err == nil { // btcec does all the hard work for us. However, the interface of btcec is such // that we need to unpack the result for Ethereum encoding.