From 87dd82a2ff93fa9e00e1a07d2a4f35410e959c0c Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 8 May 2024 10:08:39 +0200 Subject: [PATCH] imp: check length of slices of messages (#6256) * imp: check length of slices of messages * add changelog * change test limits (cherry picked from commit 478f4c6068278c1e238a425aed5ba532eb397b50) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/types/params.go # modules/apps/27-interchain-accounts/host/types/params_test.go # modules/core/02-client/types/params.go # modules/core/02-client/types/params_test.go --- CHANGELOG.md | 5 +++++ .../host/types/params.go | 12 +++++++++++ .../host/types/params_test.go | 7 +++++++ modules/core/02-client/types/params.go | 21 +++++++++++++++++++ modules/core/02-client/types/params_test.go | 6 ++++++ modules/core/03-connection/types/msgs.go | 3 +++ modules/core/03-connection/types/msgs_test.go | 4 +++- modules/core/03-connection/types/version.go | 8 +++++++ 8 files changed, 65 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ca36a43c73..77e21c22dc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,8 +46,13 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +<<<<<<< HEAD * (apps/27-interchain-accounts) [\#6147](https://github.com/cosmos/ibc-go/pull/6147) Emit an event signalling that the host submodule is disabled. * (testing) [\#6180](https://github.com/cosmos/ibc-go/pull/6180) Add version to tm abci headers in ibctesting. +======= +* (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. +* (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages. +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) * (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other. ### Features diff --git a/modules/apps/27-interchain-accounts/host/types/params.go b/modules/apps/27-interchain-accounts/host/types/params.go index af28c413c9e..6c936c21915 100644 --- a/modules/apps/27-interchain-accounts/host/types/params.go +++ b/modules/apps/27-interchain-accounts/host/types/params.go @@ -10,6 +10,8 @@ import ( const ( // DefaultHostEnabled is the default value for the host param (set to true) DefaultHostEnabled = true + // Maximum length of the allowlist + MaxAllowListLength = 500 ) var ( @@ -50,6 +52,7 @@ func (p Params) Validate() error { return nil } +<<<<<<< HEAD // ParamSetPairs implements params.ParamSet func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs { return paramtypes.ParamSetPairs{ @@ -71,6 +74,15 @@ func validateAllowlist(i interface{}) error { allowMsgs, ok := i.([]string) if !ok { return fmt.Errorf("invalid parameter type: %T", i) +======= +func validateAllowlist(allowMsgs []string) error { + if len(allowMsgs) > MaxAllowListLength { + return fmt.Errorf("allow list length must not exceed %d items", MaxAllowListLength) + } + + if slices.Contains(allowMsgs, AllowAllHostMsgs) && len(allowMsgs) > 1 { + return fmt.Errorf("allow list must have only one element because the allow all host messages wildcard (%s) is present", AllowAllHostMsgs) +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) } for _, typeURL := range allowMsgs { diff --git a/modules/apps/27-interchain-accounts/host/types/params_test.go b/modules/apps/27-interchain-accounts/host/types/params_test.go index e83b82e7447..dcfba118557 100644 --- a/modules/apps/27-interchain-accounts/host/types/params_test.go +++ b/modules/apps/27-interchain-accounts/host/types/params_test.go @@ -11,4 +11,11 @@ import ( func TestValidateParams(t *testing.T) { require.NoError(t, types.DefaultParams().Validate()) require.NoError(t, types.NewParams(false, []string{}).Validate()) +<<<<<<< HEAD +======= + require.Error(t, types.NewParams(true, []string{""}).Validate()) + require.Error(t, types.NewParams(true, []string{" "}).Validate()) + require.Error(t, types.NewParams(true, []string{"*", "/cosmos.bank.v1beta1.MsgSend"}).Validate()) + require.Error(t, types.NewParams(true, make([]string, types.MaxAllowListLength+1)).Validate()) +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) } diff --git a/modules/core/02-client/types/params.go b/modules/core/02-client/types/params.go index b0c43b156ef..371b9b745b1 100644 --- a/modules/core/02-client/types/params.go +++ b/modules/core/02-client/types/params.go @@ -9,6 +9,7 @@ import ( "github.com/cosmos/ibc-go/v7/modules/core/exported" ) +<<<<<<< HEAD var ( // DefaultAllowedClients are the default clients for the AllowedClients parameter. DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint, exported.Localhost} @@ -21,6 +22,14 @@ var ( func ParamKeyTable() paramtypes.KeyTable { return paramtypes.NewKeyTable().RegisterParamSet(&Params{}) } +======= +// Maximum length of the allowed clients list +const MaxAllowedClientsLength = 200 + +// DefaultAllowedClients are the default clients for the AllowedClients parameter. +// By default it allows all client types. +var DefaultAllowedClients = []string{AllowAllClients} +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) // NewParams creates a new parameter configuration for the ibc client module func NewParams(allowedClients ...string) Params { @@ -46,6 +55,7 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs { } } +<<<<<<< HEAD // IsAllowedClient checks if the given client type is registered on the allowlist. func (p Params) IsAllowedClient(clientType string) bool { for _, allowedClient := range p.AllowedClients { @@ -60,6 +70,17 @@ func validateClients(i interface{}) error { clients, ok := i.([]string) if !ok { return fmt.Errorf("invalid parameter type: %T", i) +======= +// validateClients checks that the given clients are not blank and there are no duplicates. +// If AllowAllClients wildcard (*) is used, then there should no other client types in the allow list +func validateClients(clients []string) error { + if len(clients) > MaxAllowedClientsLength { + return fmt.Errorf("allowed clients length must not exceed %d items", MaxAllowedClientsLength) + } + + if slices.Contains(clients, AllowAllClients) && len(clients) > 1 { + return fmt.Errorf("allow list must have only one element because the allow all clients wildcard (%s) is present", AllowAllClients) +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) } for i, clientType := range clients { diff --git a/modules/core/02-client/types/params_test.go b/modules/core/02-client/types/params_test.go index 447b0ffa5c7..0f6b07d4715 100644 --- a/modules/core/02-client/types/params_test.go +++ b/modules/core/02-client/types/params_test.go @@ -17,6 +17,12 @@ func TestValidateParams(t *testing.T) { {"default params", DefaultParams(), true}, {"custom params", NewParams(exported.Tendermint), true}, {"blank client", NewParams(" "), false}, +<<<<<<< HEAD +======= + {"duplicate clients", NewParams(exported.Tendermint, exported.Tendermint), false}, + {"allow all clients plus valid client", NewParams(AllowAllClients, exported.Tendermint), false}, + {"too many allowed clients", NewParams(make([]string, MaxAllowedClientsLength+1)...), false}, +>>>>>>> 478f4c60 (imp: check length of slices of messages (#6256)) } for _, tc := range testCases { diff --git a/modules/core/03-connection/types/msgs.go b/modules/core/03-connection/types/msgs.go index 96a9f84a743..3580d976fba 100644 --- a/modules/core/03-connection/types/msgs.go +++ b/modules/core/03-connection/types/msgs.go @@ -133,6 +133,9 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error { if len(msg.CounterpartyVersions) == 0 { return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "empty counterparty versions") } + if len(msg.CounterpartyVersions) > MaxCounterpartyVersionsLength { + return errorsmod.Wrapf(ibcerrors.ErrInvalidVersion, "counterparty versions must not exceed %d items", MaxCounterpartyVersionsLength) + } for i, version := range msg.CounterpartyVersions { if err := ValidateVersion(version); err != nil { return sdkerrors.Wrapf(err, "basic validation failed on version with index %d", i) diff --git a/modules/core/03-connection/types/msgs_test.go b/modules/core/03-connection/types/msgs_test.go index bc75adf6116..2c73200e03f 100644 --- a/modules/core/03-connection/types/msgs_test.go +++ b/modules/core/03-connection/types/msgs_test.go @@ -150,8 +150,10 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() { {"empty proofConsensus", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, signer), false}, {"invalid consensusHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), signer), false}, {"empty singer", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ""), false}, - {"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true}, {"invalid version", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{{}}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false}, + {"too many counterparty versions", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, make([]*types.Version, types.MaxCounterpartyVersionsLength+1), 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false}, + {"too many features in counterparty version", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{{"v1", make([]string, types.MaxFeaturesLength+1)}}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false}, + {"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true}, } for _, tc := range testCases { diff --git a/modules/core/03-connection/types/version.go b/modules/core/03-connection/types/version.go index 827ced76ba9..9fe1e060268 100644 --- a/modules/core/03-connection/types/version.go +++ b/modules/core/03-connection/types/version.go @@ -28,6 +28,11 @@ var ( allowNilFeatureSet = map[string]bool{ DefaultIBCVersionIdentifier: false, } + + // MaxVersionsLength is the maximum number of versions that can be supported + MaxCounterpartyVersionsLength = 100 + // MaxFeaturesLength is the maximum number of features that can be supported + MaxFeaturesLength = 100 ) var _ exported.Version = &Version{} @@ -59,6 +64,9 @@ func ValidateVersion(version *Version) error { if strings.TrimSpace(version.Identifier) == "" { return sdkerrors.Wrap(ErrInvalidVersion, "version identifier cannot be blank") } + if len(version.Features) > MaxFeaturesLength { + return errorsmod.Wrapf(ErrInvalidVersion, "features length must not exceed %d items", MaxFeaturesLength) + } for i, feature := range version.Features { if strings.TrimSpace(feature) == "" { return sdkerrors.Wrapf(ErrInvalidVersion, "feature cannot be blank, index %d", i)