diff --git a/CHANGELOG.md b/CHANGELOG.md index 807ffa1493f..f464e3a783a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * (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. * (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. * (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. diff --git a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go index 77f62a73c5d..3e36f809a14 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/migrations.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/migrations.go @@ -29,24 +29,23 @@ func NewMigrator(k *Keeper) Migrator { // are owned by the controller submodule and ibc. func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error { if m.keeper != nil { - logger := m.keeper.Logger(ctx) filteredChannels := m.keeper.channelKeeper.GetAllChannelsWithPortPrefix(ctx, icatypes.ControllerPortPrefix) for _, ch := range filteredChannels { name := host.ChannelCapabilityPath(ch.PortId, ch.ChannelId) capability, found := m.keeper.scopedKeeper.GetCapability(ctx, name) if !found { - logger.Error(fmt.Sprintf("failed to find capability: %s", name)) + m.keeper.Logger(ctx).Error(fmt.Sprintf("failed to find capability: %s", name)) return errorsmod.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", name) } isAuthenticated := m.keeper.scopedKeeper.AuthenticateCapability(ctx, capability, name) if !isAuthenticated { - logger.Error(fmt.Sprintf("expected capability owner: %s", controllertypes.SubModuleName)) + m.keeper.Logger(ctx).Error(fmt.Sprintf("expected capability owner: %s", controllertypes.SubModuleName)) return errorsmod.Wrapf(capabilitytypes.ErrCapabilityNotOwned, "expected capability owner: %s", controllertypes.SubModuleName) } m.keeper.SetMiddlewareEnabled(ctx, ch.PortId, ch.ConnectionHops[0]) - logger.Info("successfully migrated channel capability", "name", name) + m.keeper.Logger(ctx).Info("successfully migrated channel capability", "name", name) } } return nil diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index b5163393f67..3829a4c9acd 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -116,9 +116,8 @@ func (im IBCModule) OnRecvPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) ibcexported.Acknowledgement { - logger := im.keeper.Logger(ctx) if !im.keeper.GetParams(ctx).HostEnabled { - logger.Info("host submodule is disabled") + im.keeper.Logger(ctx).Info("host submodule is disabled") keeper.EmitHostDisabledEvent(ctx, packet) return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled) } @@ -127,9 +126,9 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement(txResponse) if err != nil { ack = channeltypes.NewErrorAcknowledgement(err) - logger.Error(fmt.Sprintf("%s sequence %d", err.Error(), packet.Sequence)) + im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", err.Error(), packet.Sequence)) } else { - logger.Info("successfully handled packet", "sequence", packet.Sequence) + im.keeper.Logger(ctx).Info("successfully handled packet", "sequence", packet.Sequence) } // Emit an event indicating a successful or failed acknowledgement. diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 2ec9dc8afbd..0613b8f32cf 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -30,7 +30,6 @@ func (k Keeper) OnChanOpenTry( counterparty channeltypes.Counterparty, counterpartyVersion string, ) (string, error) { - logger := k.Logger(ctx) if portID != icatypes.HostPortID { return "", errorsmod.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.HostPortID, portID) } @@ -43,7 +42,7 @@ func (k Keeper) OnChanOpenTry( return "", errorsmod.Wrapf(err, "failed to retrieve connection %s", connectionHops[0]) } - logger.Debug("counterparty version is invalid, proposing default metadata") + k.Logger(ctx).Debug("counterparty version is invalid, proposing default metadata") metadata = icatypes.NewDefaultMetadata(connection.Counterparty.ConnectionId, connectionHops[0]) } diff --git a/modules/apps/27-interchain-accounts/host/types/params.go b/modules/apps/27-interchain-accounts/host/types/params.go index 5c47fe65487..cb2fd40fa29 100644 --- a/modules/apps/27-interchain-accounts/host/types/params.go +++ b/modules/apps/27-interchain-accounts/host/types/params.go @@ -9,6 +9,8 @@ import ( const ( // DefaultHostEnabled is the default value for the host param (set to true) DefaultHostEnabled = true + // Maximum length of the allowlist + MaxAllowListLength = 500 ) // NewParams creates a new parameter configuration for the host submodule @@ -30,6 +32,10 @@ func (p Params) Validate() error { } 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) } 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 9cd7b46bc45..bc443707ba4 100644 --- a/modules/apps/27-interchain-accounts/host/types/params_test.go +++ b/modules/apps/27-interchain-accounts/host/types/params_test.go @@ -14,4 +14,5 @@ func TestValidateParams(t *testing.T) { 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()) } diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index 1424260f178..7e312b7b869 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -180,14 +180,13 @@ func (im IBCModule) OnRecvPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) ibcexported.Acknowledgement { - logger := im.keeper.Logger(ctx) ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) var data types.FungibleTokenPacketData var ackErr error if err := json.Unmarshal(packet.GetData(), &data); err != nil { ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data") - logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) + im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) ack = channeltypes.NewErrorAcknowledgement(ackErr) } @@ -198,9 +197,9 @@ func (im IBCModule) OnRecvPacket( if err != nil { ack = channeltypes.NewErrorAcknowledgement(err) ackErr = err - logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) + im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) } else { - logger.Info("successfully handled ICS-20 packet", "sequence", packet.Sequence) + im.keeper.Logger(ctx).Info("successfully handled ICS-20 packet", "sequence", packet.Sequence) } } diff --git a/modules/core/02-client/types/params.go b/modules/core/02-client/types/params.go index da5ce32ff46..35001ea5cd9 100644 --- a/modules/core/02-client/types/params.go +++ b/modules/core/02-client/types/params.go @@ -6,6 +6,9 @@ import ( "strings" ) +// 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} @@ -46,6 +49,10 @@ func (p Params) IsAllowedClient(clientType string) bool { // 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) } diff --git a/modules/core/02-client/types/params_test.go b/modules/core/02-client/types/params_test.go index 347b913450f..7a80749b375 100644 --- a/modules/core/02-client/types/params_test.go +++ b/modules/core/02-client/types/params_test.go @@ -40,6 +40,7 @@ func TestValidateParams(t *testing.T) { {"blank client", NewParams(" "), false}, {"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}, } for _, tc := range testCases { diff --git a/modules/core/03-connection/types/msgs.go b/modules/core/03-connection/types/msgs.go index 5b927af7347..43a45807ca6 100644 --- a/modules/core/03-connection/types/msgs.go +++ b/modules/core/03-connection/types/msgs.go @@ -129,6 +129,9 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error { if len(msg.CounterpartyVersions) == 0 { return errorsmod.Wrap(ibcerrors.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 errorsmod.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 9106e3aa15f..e02b79db071 100644 --- a/modules/core/03-connection/types/msgs_test.go +++ b/modules/core/03-connection/types/msgs_test.go @@ -159,8 +159,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 00aadcb8682..c30c709277e 100644 --- a/modules/core/03-connection/types/version.go +++ b/modules/core/03-connection/types/version.go @@ -27,6 +27,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 ) // NewVersion returns a new instance of Version. @@ -56,6 +61,9 @@ func ValidateVersion(version *Version) error { if strings.TrimSpace(version.Identifier) == "" { return errorsmod.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 errorsmod.Wrapf(ErrInvalidVersion, "feature cannot be blank, index %d", i) diff --git a/modules/light-clients/07-tendermint/migrations/migrations.go b/modules/light-clients/07-tendermint/migrations/migrations.go index 7eb6e91b1f2..eb80c602a46 100644 --- a/modules/light-clients/07-tendermint/migrations/migrations.go +++ b/modules/light-clients/07-tendermint/migrations/migrations.go @@ -40,8 +40,7 @@ func PruneExpiredConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, clientK totalPruned += ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) } - clientLogger := clientKeeper.Logger(ctx) - clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned) + clientKeeper.Logger(ctx).Info("pruned expired tendermint consensus states", "total", totalPruned) return totalPruned, nil }