From 58c029a63bfb151b3a8ce2f503cd67dbf371cdde Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 3 Jul 2024 01:58:34 +0000 Subject: [PATCH] feat: api: sanity check the "to" address of outgoing messages (#12135) * feat: api: sanity check the "to" address of outgoing messages If the "to" address of an outgoing message is a _delegated_ address, verify that it maps to a valid Ethereum address. This isn't a consensus critical change, but it'll help prevent client-side address conversion libraries from directing messages into oblivion (e.g., by mis-translating `0xff0000....` addresses into `f410f...` addresses instead of `f0...` addresses. * tests for invalid delegated addresses * fix lint --------- Co-authored-by: aarshkshah1992 --- itests/mempool_test.go | 28 +++++++++++++++ node/impl/full/mpool.go | 42 ++++++++++++++++++++++ node/impl/full/mpool_test.go | 67 ++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 node/impl/full/mpool_test.go diff --git a/itests/mempool_test.go b/itests/mempool_test.go index f07b46a737c..e366fe49bb4 100644 --- a/itests/mempool_test.go +++ b/itests/mempool_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/filecoin-project/go-address" "github.com/filecoin-project/go-state-types/big" "github.com/filecoin-project/lotus/api" @@ -18,6 +19,33 @@ import ( const mPoolThrottle = time.Millisecond * 100 const mPoolTimeout = time.Second * 10 +func TestMemPoolPushOutgoingInvalidDelegated(t *testing.T) { + //stm: @CHAIN_MEMPOOL_PENDING_001, @CHAIN_STATE_WAIT_MSG_001, @CHAIN_MEMPOOL_CAP_GAS_FEE_001 + //stm: @CHAIN_MEMPOOL_PUSH_002 + ctx := context.Background() + firstNode, _, _, ens := kit.EnsembleTwoOne(t, kit.MockProofs()) + ens.InterconnectAll() + kit.QuietMiningLogs() + + sender := firstNode.DefaultKey.Address + badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta") + require.NoError(t, err) + + bal, err := firstNode.WalletBalance(ctx, sender) + require.NoError(t, err) + toSend := big.Div(bal, big.NewInt(10)) + + msg := &types.Message{ + From: sender, + Value: toSend, + To: badTo, + } + + _, err = firstNode.MpoolPushMessage(ctx, msg, nil) + require.Error(t, err) + require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address") +} + func TestMemPoolPushSingleNode(t *testing.T) { //stm: @CHAIN_MEMPOOL_CREATE_MSG_CHAINS_001, @CHAIN_MEMPOOL_SELECT_001 //stm: @CHAIN_MEMPOOL_PENDING_001, @CHAIN_STATE_WAIT_MSG_001, @CHAIN_MEMPOOL_CAP_GAS_FEE_001 diff --git a/node/impl/full/mpool.go b/node/impl/full/mpool.go index fac48a3508f..13b7665e059 100644 --- a/node/impl/full/mpool.go +++ b/node/impl/full/mpool.go @@ -16,6 +16,7 @@ import ( "github.com/filecoin-project/lotus/chain/messagepool" "github.com/filecoin-project/lotus/chain/messagesigner" "github.com/filecoin-project/lotus/chain/types" + "github.com/filecoin-project/lotus/chain/types/ethtypes" "github.com/filecoin-project/lotus/node/modules/dtypes" ) @@ -131,14 +132,24 @@ func (a *MpoolAPI) MpoolClear(ctx context.Context, local bool) error { } func (m *MpoolModule) MpoolPush(ctx context.Context, smsg *types.SignedMessage) (cid.Cid, error) { + if err := sanityCheckOutgoingMessage(&smsg.Message); err != nil { + return cid.Undef, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", smsg.Cid(), smsg.Message.From, smsg.Message.Nonce, err) + } return m.Mpool.Push(ctx, smsg, true) } func (a *MpoolAPI) MpoolPushUntrusted(ctx context.Context, smsg *types.SignedMessage) (cid.Cid, error) { + if err := sanityCheckOutgoingMessage(&smsg.Message); err != nil { + return cid.Undef, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", smsg.Cid(), smsg.Message.From, smsg.Message.Nonce, err) + } return a.Mpool.PushUntrusted(ctx, smsg) } func (a *MpoolAPI) MpoolPushMessage(ctx context.Context, msg *types.Message, spec *api.MessageSendSpec) (*types.SignedMessage, error) { + if err := sanityCheckOutgoingMessage(msg); err != nil { + return nil, xerrors.Errorf("message from %s failed sanity check: %w", msg.From, err) + } + cp := *msg msg = &cp inMsg := *msg @@ -223,6 +234,11 @@ func (a *MpoolAPI) MpoolPushMessage(ctx context.Context, msg *types.Message, spe } func (a *MpoolAPI) MpoolBatchPush(ctx context.Context, smsgs []*types.SignedMessage) ([]cid.Cid, error) { + for _, msg := range smsgs { + if err := sanityCheckOutgoingMessage(&msg.Message); err != nil { + return nil, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", msg.Cid(), msg.Message.From, msg.Message.Nonce, err) + } + } var messageCids []cid.Cid for _, smsg := range smsgs { smsgCid, err := a.Mpool.Push(ctx, smsg, true) @@ -235,6 +251,11 @@ func (a *MpoolAPI) MpoolBatchPush(ctx context.Context, smsgs []*types.SignedMess } func (a *MpoolAPI) MpoolBatchPushUntrusted(ctx context.Context, smsgs []*types.SignedMessage) ([]cid.Cid, error) { + for _, msg := range smsgs { + if err := sanityCheckOutgoingMessage(&msg.Message); err != nil { + return nil, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", msg.Cid(), msg.Message.From, msg.Message.Nonce, err) + } + } var messageCids []cid.Cid for _, smsg := range smsgs { smsgCid, err := a.Mpool.PushUntrusted(ctx, smsg) @@ -247,6 +268,11 @@ func (a *MpoolAPI) MpoolBatchPushUntrusted(ctx context.Context, smsgs []*types.S } func (a *MpoolAPI) MpoolBatchPushMessage(ctx context.Context, msgs []*types.Message, spec *api.MessageSendSpec) ([]*types.SignedMessage, error) { + for i, msg := range msgs { + if err := sanityCheckOutgoingMessage(msg); err != nil { + return nil, xerrors.Errorf("message #%d from %s with failed sanity check: %w", i, msg.From, err) + } + } var smsgs []*types.SignedMessage for _, msg := range msgs { smsg, err := a.MpoolPushMessage(ctx, msg, spec) @@ -277,3 +303,19 @@ func (a *MpoolAPI) MpoolGetNonce(ctx context.Context, addr address.Address) (uin func (a *MpoolAPI) MpoolSub(ctx context.Context) (<-chan api.MpoolUpdate, error) { return a.Mpool.Updates(ctx) } + +func sanityCheckOutgoingMessage(msg *types.Message) error { + // Check that the message's TO address is a _valid_ Eth address if it's a delegated address. + // + // It's legal (from a consensus perspective) to send funds to any 0xf410f address as long as + // the payload is at most 54 bytes, but the vast majority of this address space is + // essentially a black-hole. Unfortunately, the conversion from 0x addresses to Filecoin + // native addresses has a few pitfalls (especially with respect to masked ID addresses), so + // we've added this check to the API to avoid accidentally (and avoidably) sending messages + // to these black-hole addresses. + if msg.To.Protocol() == address.Delegated && !ethtypes.IsEthAddress(msg.To) { + return xerrors.Errorf("message recipient %s is a delegated address but not a valid Eth Address", msg.To) + } + + return nil +} diff --git a/node/impl/full/mpool_test.go b/node/impl/full/mpool_test.go new file mode 100644 index 00000000000..c8e44edbfd8 --- /dev/null +++ b/node/impl/full/mpool_test.go @@ -0,0 +1,67 @@ +package full + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/filecoin-project/go-address" + "github.com/filecoin-project/go-state-types/crypto" + + "github.com/filecoin-project/lotus/chain/types" +) + +func TestSanityCheckOutgoingMessage(t *testing.T) { + // fails for invalid delegated address + badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta") + require.NoError(t, err) + msg := &types.Message{ + To: badTo, + } + + err = sanityCheckOutgoingMessage(msg) + require.Error(t, err) + require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address") + + // works for valid delegated address + goodTo, err := address.NewFromString("f410faxfebiima2gp4lduo2k3vt2iuqapuk3logeftky") + require.NoError(t, err) + msg = &types.Message{ + To: goodTo, + } + err = sanityCheckOutgoingMessage(msg) + require.NoError(t, err) + + // works for valid non-delegated address + goodTo, err = address.NewFromString("f1z762skeib2v6zlkvhywmjxbv3dxoiv4hmb6gs4y") + require.NoError(t, err) + msg = &types.Message{ + To: goodTo, + } + err = sanityCheckOutgoingMessage(msg) + require.NoError(t, err) +} + +func TestMpoolPushInvalidDelegatedAddressFails(t *testing.T) { + badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta") + require.NoError(t, err) + module := &MpoolModule{} + m := &MpoolAPI{ + MpoolModuleAPI: module, + } + smsg := &types.SignedMessage{ + Message: types.Message{ + From: badTo, + To: badTo, + }, + Signature: crypto.Signature{ + Type: crypto.SigTypeSecp256k1, + Data: []byte("signature"), + }, + } + _, err = m.MpoolPush(context.Background(), smsg) + require.Error(t, err) + + require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address") +}