imp: added length validation to MsgCreateClient and CounterpartyMerklePrefix.

CHANGELOG.md

@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * [\#8734](https://github.com/cosmos/ibc-go/pull/8734) Add extra validation for ProtoJSON unmarshalling in ICS-27 ICA.
+* [\#8774](https://github.com/cosmos/ibc-go/pull/8774) Add length validation to `MsgCreateClient` and `CounterpartyMerklePrefix`.
 
 ### Dependencies
 

modules/core/02-client/keeper/keeper_test.go

@@ -1,6 +1,7 @@
 package keeper_test
 
 import (
+	"crypto/sha256"
 	"errors"
 	"fmt"
 	"math/rand"
@@ -97,7 +98,8 @@ func (s *KeeperTestSuite) SetupTest() {
 	s.signers = make(map[string]cmttypes.PrivValidator, 1)
 	s.signers[validator.Address.String()] = s.privVal
 
-	s.consensusState = ibctm.NewConsensusState(s.now, commitmenttypes.NewMerkleRoot([]byte("hash")), s.valSetHash)
+	appHash := sha256.Sum256([]byte("app_hash"))
+	s.consensusState = ibctm.NewConsensusState(s.now, commitmenttypes.NewMerkleRoot(appHash[:]), s.valSetHash)
 
 	var validators stakingtypes.Validators
 	for i := 1; i < 11; i++ {

modules/core/02-client/types/msgs.go

@@ -35,6 +35,13 @@ var (
 	_ codectypes.UnpackInterfacesMessage = (*MsgIBCSoftwareUpgrade)(nil)
 )
 
+const (
+	// MaxClientStateSize is the maximum allowed size of the client state in bytes. (This is an arbitrarily chosen value)
+	MaxClientStateSize = 32768
+	// MaxConsensusStateSize is the maximum allowed size of the consensus state in bytes. (This is an arbitrarily chosen value)
+	MaxConsensusStateSize = 32768
+)
+
 // NewMsgCreateClient creates a new MsgCreateClient instance
 func NewMsgCreateClient(
 	clientState exported.ClientState, consensusState exported.ConsensusState, signer string,
@@ -62,13 +69,21 @@ func (msg MsgCreateClient) ValidateBasic() error {
 	if err != nil {
 		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
 	}
+	// validate the total size of client state
+	if len(msg.ClientState.Value) > MaxClientStateSize {
+		return errorsmod.Wrapf(ibcerrors.ErrTooLarge, "client state size %d exceeds max size %d", len(msg.ClientState.Value), MaxClientStateSize)
+	}
 	clientState, err := UnpackClientState(msg.ClientState)
 	if err != nil {
 		return err
 	}
 	if err := clientState.Validate(); err != nil {
 		return err
 	}
+	// validate the total size of consensus state
+	if len(msg.ConsensusState.Value) > MaxConsensusStateSize {
+		return errorsmod.Wrapf(ibcerrors.ErrTooLarge, "consensus state size %d exceeds max size %d", len(msg.ConsensusState.Value), MaxConsensusStateSize)
+	}
 	consensusState, err := UnpackConsensusState(msg.ConsensusState)
 	if err != nil {
 		return err

modules/core/02-client/types/msgs_test.go

@@ -110,7 +110,7 @@ func (s *TypesTestSuite) TestMsgCreateClient_ValidateBasic() {
 		expErr   error
 	}{
 		{
-			"valid - tendermint client",
+			"success: tendermint client",
 			func() {
 				tendermintClient := ibctm.NewClientState(s.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
 				msg, err = types.NewMsgCreateClient(tendermintClient, s.chainA.CurrentTMClientHeader().ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
@@ -119,48 +119,68 @@ func (s *TypesTestSuite) TestMsgCreateClient_ValidateBasic() {
 			nil,
 		},
 		{
-			"invalid tendermint client",
+			"success: solomachine client",
+			func() {
+				soloMachine := ibctesting.NewSolomachine(s.T(), s.chainA.Codec, "solomachine", "", 2)
+				msg, err = types.NewMsgCreateClient(soloMachine.ClientState(), soloMachine.ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
+				s.Require().NoError(err)
+			},
+			nil,
+		},
+		{
+			"failure: invalid tendermint client",
 			func() {
 				msg, err = types.NewMsgCreateClient(&ibctm.ClientState{}, s.chainA.CurrentTMClientHeader().ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
 				s.Require().NoError(err)
 			},
 			errorsmod.Wrap(ibctm.ErrInvalidChainID, "chain id cannot be empty string"),
 		},
 		{
-			"failed to unpack client",
+			"failure: client state too large",
 			func() {
-				msg.ClientState = nil
+				msg.ClientState.Value = []byte(ibctesting.GenerateString(types.MaxClientStateSize + 1))
 			},
-			errorsmod.Wrap(ibcerrors.ErrUnpackAny, "protobuf Any message cannot be nil"),
+			ibcerrors.ErrTooLarge,
 		},
 		{
-			"failed to unpack consensus state",
+			"failure: consensus state too large",
 			func() {
 				tendermintClient := ibctm.NewClientState(s.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
 				msg, err = types.NewMsgCreateClient(tendermintClient, s.chainA.CurrentTMClientHeader().ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
 				s.Require().NoError(err)
-				msg.ConsensusState = nil
+				msg.ConsensusState = &codectypes.Any{
+					TypeUrl: "lol",
+					Value:   []byte(ibctesting.GenerateString(types.MaxConsensusStateSize + 1)),
+				}
 			},
-			errorsmod.Wrap(ibcerrors.ErrUnpackAny, "protobuf Any message cannot be nil"),
+			ibcerrors.ErrTooLarge,
 		},
 		{
-			"invalid signer",
+			"failure: failed to unpack client",
 			func() {
-				msg.Signer = ""
+				msg.ClientState = &codectypes.Any{}
 			},
-			errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: empty address string is not allowed"),
+			ibcerrors.ErrUnpackAny,
 		},
 		{
-			"valid - solomachine client",
+			"failure: failed to unpack consensus state",
 			func() {
-				soloMachine := ibctesting.NewSolomachine(s.T(), s.chainA.Codec, "solomachine", "", 2)
-				msg, err = types.NewMsgCreateClient(soloMachine.ClientState(), soloMachine.ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
+				tendermintClient := ibctm.NewClientState(s.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
+				msg, err = types.NewMsgCreateClient(tendermintClient, s.chainA.CurrentTMClientHeader().ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
 				s.Require().NoError(err)
+				msg.ConsensusState = &codectypes.Any{}
 			},
-			nil,
+			ibcerrors.ErrUnpackAny,
+		},
+		{
+			"failure: invalid signer",
+			func() {
+				msg.Signer = ""
+			},
+			errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: empty address string is not allowed"),
 		},
 		{
-			"invalid solomachine client",
+			"failure: invalid solomachine client",
 			func() {
 				soloMachine := ibctesting.NewSolomachine(s.T(), s.chainA.Codec, "solomachine", "", 2)
 				msg, err = types.NewMsgCreateClient(&solomachine.ClientState{}, soloMachine.ConsensusState(), s.chainA.SenderAccount.GetAddress().String())
@@ -169,7 +189,7 @@ func (s *TypesTestSuite) TestMsgCreateClient_ValidateBasic() {
 			errorsmod.Wrap(types.ErrInvalidClient, "sequence cannot be 0"),
 		},
 		{
-			"invalid solomachine consensus state",
+			"failure: invalid solomachine consensus state",
 			func() {
 				soloMachine := ibctesting.NewSolomachine(s.T(), s.chainA.Codec, "solomachine", "", 2)
 				msg, err = types.NewMsgCreateClient(soloMachine.ClientState(), &solomachine.ConsensusState{}, s.chainA.SenderAccount.GetAddress().String())
@@ -178,7 +198,7 @@ func (s *TypesTestSuite) TestMsgCreateClient_ValidateBasic() {
 			errorsmod.Wrap(types.ErrInvalidConsensus, "timestamp cannot be 0"),
 		},
 		{
-			"invalid - client state and consensus state client types do not match",
+			"failure: invalid - client state and consensus state client types do not match",
 			func() {
 				tendermintClient := ibctm.NewClientState(s.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
 				soloMachine := ibctesting.NewSolomachine(s.T(), s.chainA.Codec, "solomachine", "", 2)

modules/core/02-client/v2/types/msgs.go

@@ -6,10 +6,14 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v10/modules/core/02-client/types"
+	conntypes "github.com/cosmos/ibc-go/v10/modules/core/03-connection/types"
 	host "github.com/cosmos/ibc-go/v10/modules/core/24-host"
 	ibcerrors "github.com/cosmos/ibc-go/v10/modules/core/errors"
 )
 
+// MaxCounterpartyMerklePrefixElements defines the maximum number of elements allowed in the counterparty merkle prefix. (This is an arbitrarily chosen value)
+const MaxCounterpartyMerklePrefixElements = 32
+
 var (
 	_ sdk.Msg = (*MsgRegisterCounterparty)(nil)
 	_ sdk.Msg = (*MsgUpdateClientConfig)(nil)
@@ -36,6 +40,14 @@ func (msg *MsgRegisterCounterparty) ValidateBasic() error {
 	if len(msg.CounterpartyMerklePrefix) == 0 {
 		return errorsmod.Wrap(ErrInvalidCounterparty, "counterparty messaging key cannot be empty")
 	}
+	if len(msg.CounterpartyMerklePrefix) > MaxCounterpartyMerklePrefixElements {
+		return errorsmod.Wrapf(ibcerrors.ErrTooLarge, "counterparty merkle prefix length cannot exceed %d elements", MaxCounterpartyMerklePrefixElements)
+	}
+	for i, key := range msg.CounterpartyMerklePrefix {
+		if len(key) > conntypes.MaxMerklePrefixLength {
+			return errorsmod.Wrapf(ibcerrors.ErrTooLarge, "counterparty merkle prefix key at index %d exceeds max length of %d bytes", i, conntypes.MaxMerklePrefixLength)
+		}
+	}
 	if err := host.ClientIdentifierValidator(msg.ClientId); err != nil {
 		return err
 	}

modules/core/02-client/v2/types/msgs_test.go

@@ -11,6 +11,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v10/modules/core/02-client/v2/types"
+	conntypes "github.com/cosmos/ibc-go/v10/modules/core/03-connection/types"
 	host "github.com/cosmos/ibc-go/v10/modules/core/24-host"
 	ibcerrors "github.com/cosmos/ibc-go/v10/modules/core/errors"
 	ibctesting "github.com/cosmos/ibc-go/v10/testing"
@@ -26,83 +27,88 @@ func TestMsgRegisterCounterpartyValidateBasic(t *testing.T) {
 	signer := ibctesting.TestAccAddress
 	testCases := []struct {
 		name     string
-		msg      *types.MsgRegisterCounterparty
+		malleate func(msg *types.MsgRegisterCounterparty)
 		expError error
 	}{
 		{
 			"success",
-			types.NewMsgRegisterCounterparty(
-				"testclientid-3",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"testclientid-2",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {},
 			nil,
 		},
 		{
 			"failure: client id does not match clientID format",
-			types.NewMsgRegisterCounterparty(
-				"testclientid1",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"testclientid-3",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.ClientId = "testclientid1"
+			},
 			host.ErrInvalidID,
 		},
 		{
 			"failure: counterparty client id does not match clientID format",
-			types.NewMsgRegisterCounterparty(
-				"testclientid-1",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"testclientid2",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.CounterpartyClientId = "testclientid2"
+			},
 			host.ErrInvalidID,
 		},
 		{
 			"failure: empty client id",
-			types.NewMsgRegisterCounterparty(
-				"",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"testclientid-3",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.ClientId = ""
+			},
 			host.ErrInvalidID,
 		},
 		{
 			"failure: empty counterparty client id",
-			types.NewMsgRegisterCounterparty(
-				"testclientid-1",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.CounterpartyClientId = ""
+			},
 			host.ErrInvalidID,
 		},
 		{
 			"failure: empty counterparty messaging key",
-			types.NewMsgRegisterCounterparty(
-				"testclientid-1",
-				[][]byte{},
-				"testclientid-3",
-				signer,
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.CounterpartyMerklePrefix = [][]byte{}
+			},
 			types.ErrInvalidCounterparty,
 		},
 		{
 			"failure: empty signer",
-			types.NewMsgRegisterCounterparty(
-				"testclientid-2",
-				[][]byte{[]byte("ibc"), []byte("channel-9")},
-				"testclientid-3",
-				"badsigner",
-			),
+			func(msg *types.MsgRegisterCounterparty) {
+				msg.Signer = "badsigner"
+			},
 			ibcerrors.ErrInvalidAddress,
 		},
+		{
+			"failure: counterparty merkle prefix length too large",
+			func(msg *types.MsgRegisterCounterparty) {
+				tooLargePrefix := make([][]byte, types.MaxCounterpartyMerklePrefixElements+1)
+				for i := range tooLargePrefix {
+					tooLargePrefix[i] = []byte("key")
+				}
+				msg.CounterpartyMerklePrefix = tooLargePrefix
+			},
+			ibcerrors.ErrTooLarge,
+		},
+		{
+			"failure: counterparty merkle prefix key too large",
+			func(msg *types.MsgRegisterCounterparty) {
+				largeKey := make([]byte, conntypes.MaxMerklePrefixLength+1)
+				msg.CounterpartyMerklePrefix = [][]byte{largeKey}
+			},
+			ibcerrors.ErrTooLarge,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := tc.msg.ValidateBasic()
+			msg := types.NewMsgRegisterCounterparty(
+				"testclientid-3",
+				[][]byte{[]byte("ibc"), []byte("channel-9")},
+				"testclientid-2",
+				signer,
+			)
+
+			tc.malleate(msg)
+
+			err := msg.ValidateBasic()
 			if tc.expError == nil {
 				require.NoError(t, err)
 			} else {

modules/core/03-connection/types/connection.go

@@ -8,6 +8,9 @@ import (
 	ibcerrors "github.com/cosmos/ibc-go/v10/modules/core/errors"
 )
 
+// MaxMerklePrefixLength defines the maximum length of the counterparty prefix in bytes. (This is an arbitrarily chosen value)
+const MaxMerklePrefixLength = 256
+
 // NewConnectionEnd creates a new ConnectionEnd instance.
 func NewConnectionEnd(state State, clientID string, counterparty Counterparty, versions []*Version, delayPeriod uint64) ConnectionEnd {
 	return ConnectionEnd{
@@ -59,6 +62,9 @@ func (c Counterparty) ValidateBasic() error {
 	if c.Prefix.Empty() {
 		return errorsmod.Wrap(ErrInvalidCounterparty, "counterparty prefix cannot be empty")
 	}
+	if len(c.Prefix.Bytes()) > MaxMerklePrefixLength {
+		return errorsmod.Wrapf(ErrInvalidCounterparty, "counterparty prefix length exceeds maximum length of %d bytes", MaxMerklePrefixLength)
+	}
 	return nil
 }
 

modules/core/03-connection/types/connection_test.go

@@ -53,6 +53,11 @@ func TestConnectionValidateBasic(t *testing.T) {
 			types.ConnectionEnd{clientID, []*types.Version{ibctesting.ConnectionVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, emptyPrefix}, 500},
 			types.ErrInvalidCounterparty,
 		},
+		{
+			"counterparty prefix too long",
+			types.ConnectionEnd{clientID, []*types.Version{ibctesting.ConnectionVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix(make([]byte, types.MaxMerklePrefixLength+1))}, 500},
+			types.ErrInvalidCounterparty,
+		},
 	}
 
 	for i, tc := range testCases {

modules/core/errors/errors.go

@@ -60,4 +60,7 @@ var (
 
 	// ErrNotFound defines an error when requested entity doesn't exist in the state.
 	ErrNotFound = errorsmod.Register(codespace, 16, "not found")
+
+	// ErrTooLarge defines an error when the entity is too large.
+	ErrTooLarge = errorsmod.Register(codespace, 17, "entity too large")
 )

modules/light-clients/07-tendermint/consensus_state.go

@@ -51,6 +51,11 @@ func (cs ConsensusState) ValidateBasic() error {
 	if cs.Root.Empty() {
 		return errorsmod.Wrap(clienttypes.ErrInvalidConsensus, "root cannot be empty")
 	}
+	if err := cmttypes.ValidateHash(cs.Root.GetHash()); err != nil {
+		if string(cs.Root.GetHash()) != SentinelRoot {
+			return errorsmod.Wrap(err, "root hash is invalid")
+		}
+	}
 	if err := cmttypes.ValidateHash(cs.NextValidatorsHash); err != nil {
 		return errorsmod.Wrap(err, "next validators hash is invalid")
 	}