From ac5b2ca4e37c252d28afd1b87e976dbd9954215b Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 31 Oct 2023 14:13:02 +0100 Subject: [PATCH] imp(statemachine)!: add length validation of string fields in messages Co-authored-by: Jacob Gadikian Co-authored-by: Du Nguyen Co-authored-by: Charly --- .../controller/types/msgs.go | 10 ++++++++++ .../controller/types/msgs_test.go | 14 ++++++++++++++ .../27-interchain-accounts/types/account_test.go | 2 +- modules/apps/29-fee/types/msgs.go | 6 ++++++ modules/apps/29-fee/types/msgs_test.go | 7 +++++++ modules/apps/transfer/types/errors.go | 1 + modules/apps/transfer/types/msgs.go | 11 +++++++++++ modules/apps/transfer/types/msgs_test.go | 2 ++ testing/utils.go | 10 ++++++++++ testing/values.go | 3 ++- 10 files changed, 64 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs.go b/modules/apps/27-interchain-accounts/controller/types/msgs.go index b600942bf01..213fcbfbc04 100644 --- a/modules/apps/27-interchain-accounts/controller/types/msgs.go +++ b/modules/apps/27-interchain-accounts/controller/types/msgs.go @@ -12,6 +12,8 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ) +const MaximumOwnerLength = 2048 // maximum length of the owner in bytes (value chosen arbitrarily) + var ( _ sdk.Msg = (*MsgRegisterInterchainAccount)(nil) _ sdk.Msg = (*MsgSendTx)(nil) @@ -41,6 +43,10 @@ func (msg MsgRegisterInterchainAccount) ValidateBasic() error { return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "owner address cannot be empty") } + if len(msg.Owner) > MaximumOwnerLength { + return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength) + } + return nil } @@ -74,6 +80,10 @@ func (msg MsgSendTx) ValidateBasic() error { return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "owner address cannot be empty") } + if len(msg.Owner) > MaximumOwnerLength { + return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength) + } + if err := msg.PacketData.ValidateBasic(); err != nil { return errorsmod.Wrap(err, "invalid interchain account packet data") } diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go index 13ef2c9ae1a..541f3830fb6 100644 --- a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go +++ b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go @@ -64,6 +64,13 @@ func TestMsgRegisterInterchainAccountValidateBasic(t *testing.T) { }, false, }, + { + "owner address is too long", + func() { + msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1) + }, + false, + }, } for i, tc := range testCases { @@ -121,6 +128,13 @@ func TestMsgSendTxValidateBasic(t *testing.T) { }, false, }, + { + "owner address is too long", + func() { + msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1) + }, + false, + }, { "relative timeout is not set", func() { diff --git a/modules/apps/27-interchain-accounts/types/account_test.go b/modules/apps/27-interchain-accounts/types/account_test.go index 2130a46edf4..9ff7dc19841 100644 --- a/modules/apps/27-interchain-accounts/types/account_test.go +++ b/modules/apps/27-interchain-accounts/types/account_test.go @@ -79,7 +79,7 @@ func (suite *TypesTestSuite) TestValidateAccountAddress() { }, { "address is too long", - ibctesting.LongString, + ibctesting.GenerateString(uint(types.DefaultMaxAddrLength) + 1), false, }, } diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index 095d36a604d..af2a9287a65 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/modules/apps/29-fee/types/msgs.go @@ -12,6 +12,8 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ) +const MaximumCounterpartyPayeeLength = 2048 // maximum length of the counterparty payee in bytes (value chosen arbitrarily) + var ( _ sdk.Msg = (*MsgRegisterPayee)(nil) _ sdk.Msg = (*MsgRegisterCounterpartyPayee)(nil) @@ -100,6 +102,10 @@ func (msg MsgRegisterCounterpartyPayee) ValidateBasic() error { return ErrCounterpartyPayeeEmpty } + if len(msg.CounterpartyPayee) > MaximumCounterpartyPayeeLength { + return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "counterparty payee address must not exceed %d bytes", MaximumCounterpartyPayeeLength) + } + return nil } diff --git a/modules/apps/29-fee/types/msgs_test.go b/modules/apps/29-fee/types/msgs_test.go index 4c59160cb1e..6f1d2bce4eb 100644 --- a/modules/apps/29-fee/types/msgs_test.go +++ b/modules/apps/29-fee/types/msgs_test.go @@ -139,6 +139,13 @@ func TestMsgRegisterCountepartyPayeeValidation(t *testing.T) { }, false, }, + { + "invalid counterparty payee address: too long", + func() { + msg.CounterpartyPayee = ibctesting.GenerateString(types.MaximumCounterpartyPayeeLength + 1) + }, + false, + }, } for i, tc := range testCases { diff --git a/modules/apps/transfer/types/errors.go b/modules/apps/transfer/types/errors.go index d62134b27cd..b3e45aa2ae2 100644 --- a/modules/apps/transfer/types/errors.go +++ b/modules/apps/transfer/types/errors.go @@ -15,4 +15,5 @@ var ( ErrReceiveDisabled = errorsmod.Register(ModuleName, 8, "fungible token transfers to this chain are disabled") ErrMaxTransferChannels = errorsmod.Register(ModuleName, 9, "max transfer channels") ErrInvalidAuthorization = errorsmod.Register(ModuleName, 10, "invalid transfer authorization") + ErrInvalidMemo = errorsmod.Register(ModuleName, 11, "invalid memo") ) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 33915637017..ad126d149c7 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -12,6 +12,11 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors" ) +const ( + MaximumReceiverLength = 2048 // maximum length of the receiver address in bytes (value chosen arbitrarily) + MaximumMemoLength = 32768 // maximum length of the memo in bytes (value chosen arbitrarily) +) + var ( _ sdk.Msg = (*MsgUpdateParams)(nil) _ sdk.Msg = (*MsgTransfer)(nil) @@ -91,6 +96,12 @@ func (msg MsgTransfer) ValidateBasic() error { if strings.TrimSpace(msg.Receiver) == "" { return errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "missing recipient address") } + if len(msg.Receiver) > MaximumReceiverLength { + return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "recipient address must not exceed %d bytes", MaximumReceiverLength) + } + if len(msg.Memo) > MaximumMemoLength { + return errorsmod.Wrapf(ErrInvalidMemo, "memo must not exceed %d bytes", MaximumMemoLength) + } return ValidateIBCDenom(msg.Token.Denom) } diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 3afae766edb..6dc2dedc31a 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -60,11 +60,13 @@ func TestMsgTransferValidation(t *testing.T) { {"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, + {"too long memo", types.NewMsgTransfer(validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), false}, {"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoin, sender, receiver, timeoutHeight, 0, ""), false}, {"zero coin", types.NewMsgTransfer(validPort, validChannel, zeroCoin, sender, receiver, timeoutHeight, 0, ""), false}, {"missing sender address", types.NewMsgTransfer(validPort, validChannel, coin, emptyAddr, receiver, timeoutHeight, 0, ""), false}, {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, "", timeoutHeight, 0, ""), false}, + {"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), false}, {"empty coin", types.NewMsgTransfer(validPort, validChannel, sdk.Coin{}, sender, receiver, timeoutHeight, 0, ""), false}, } diff --git a/testing/utils.go b/testing/utils.go index 302d0e70822..a1178929e3f 100644 --- a/testing/utils.go +++ b/testing/utils.go @@ -1,6 +1,7 @@ package ibctesting import ( + "math/rand" "testing" "github.com/stretchr/testify/require" @@ -23,3 +24,12 @@ func ApplyValSetChanges(tb testing.TB, valSet *tmtypes.ValidatorSet, valUpdates return newVals } + +// GenerateString generates a random string of the given length in bytes +func GenerateString(length uint) string { + bytes := make([]byte, length) + for i := range bytes { + bytes[i] = charset[rand.Intn(len(charset))] + } + return string(bytes) +} diff --git a/testing/values.go b/testing/values.go index 2741b1a5fed..115f3b39281 100644 --- a/testing/values.go +++ b/testing/values.go @@ -43,7 +43,8 @@ const ( Title = "title" Description = "description" - LongString = "LoremipsumdolorsitameconsecteturadipiscingeliseddoeiusmodtemporincididuntutlaboreetdoloremagnaaliquUtenimadminimveniamquisnostrudexercitationullamcolaborisnisiutaliquipexeacommodoconsequDuisauteiruredolorinreprehenderitinvoluptateelitsseillumoloreufugiatnullaariaturEcepteurintoccaectupidatatonroidentuntnulpauifficiaeseruntmollitanimidestlaborum" + // character set used for generating a random string in GenerateString + charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" ) var (