Skip to content

Commit 31be276

Browse files
mergify[bot]srdtrk
mergify[bot]
and
srdtrk
authored
imp: add extra validation to ica msgs (backport #8734) (#8738)
* imp: add extra validation to ica msgs (#8734) * imp: add extra validation * chore: lint * imp: error msg * test: fix tests * docs: document the validation change --------- Co-authored-by: Alex | Cosmos Labs <alex@cosmoslabs.io> (cherry picked from commit 7574333) # Conflicts: # docs/docs/02-apps/02-interchain-accounts/07-tx-encoding.md # modules/apps/27-interchain-accounts/host/keeper/relay_test.go * fix: conflicts * fix: use ErrUnknownDataType instead of ibcerrors.ErrInvalidType * test: fix * lint: gci * test: fix * chore: remove unused diff * docs: update changelog --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com>
1 parent 4872019 commit 31be276

File tree

3 files changed

+164
-20
lines changed

3 files changed

+164
-20
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
3434

3535
# Changelog
3636

37+
## [v8.8.0](https://github.com/cosmos/ibc-go/releases/tag/v8.7.0) - 2025-12-18
38+
39+
### Improvements
40+
41+
* [\#8734](https://github.com/cosmos/ibc-go/pull/8734) Add extra validation for ProtoJSON unmarshalling in ICS-27 ICA.
42+
3743
## [v8.7.0](https://github.com/cosmos/ibc-go/releases/tag/v8.7.0) - 2025-03-12
3844

3945
- [ISA-2025-001](https://github.com/cosmos/ibc-go/security/advisories/GHSA-4wf3-5qj9-368v) Fix ISA-2025-001

‎modules/apps/27-interchain-accounts/host/keeper/relay_test.go‎

Lines changed: 135 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ import (
99
sdkmath "cosmossdk.io/math"
1010

1111
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
12+
"github.com/cosmos/cosmos-sdk/testutil/testdata"
1213
sdk "github.com/cosmos/cosmos-sdk/types"
14+
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
1315
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
1416
disttypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
1517
govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
@@ -555,7 +557,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
555557
expPass bool
556558
}{
557559
{
558-
"interchain account successfully executes an arbitrary message type using the * (allow all message types) param",
560+
"success: interchain account successfully executes an arbitrary message type using the * (allow all message types) param",
559561
func(icaAddress string) {
560562
// Populate the gov keeper in advance with an active proposal
561563
testProposal := &govtypes.TextProposal{
@@ -579,8 +581,8 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
579581
{
580582
"@type": "/cosmos.gov.v1beta1.MsgVote",
581583
"voter": "` + icaAddress + `",
582-
"proposal_id": 1,
583-
"option": 1
584+
"proposal_id": "1",
585+
"option": "VOTE_OPTION_YES"
584586
}
585587
]
586588
}`)
@@ -599,7 +601,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
599601
true,
600602
},
601603
{
602-
"interchain account successfully executes banktypes.MsgSend",
604+
"success: interchain account successfully executes banktypes.MsgSend",
603605
func(icaAddress string) {
604606
msgBytes := []byte(`{
605607
"messages": [
@@ -624,7 +626,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
624626
true,
625627
},
626628
{
627-
"interchain account successfully executes govtypes.MsgSubmitProposal",
629+
"success: interchain account successfully executes govtypesv1.MsgSubmitProposal",
628630
func(icaAddress string) {
629631
msgBytes := []byte(`{
630632
"messages": [
@@ -653,7 +655,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
653655
true,
654656
},
655657
{
656-
"interchain account successfully executes govtypes.MsgVote",
658+
"success: interchain account successfully executes govtypesv1.MsgVote",
657659
func(icaAddress string) {
658660
// Populate the gov keeper in advance with an active proposal
659661
testProposal := &govtypes.TextProposal{
@@ -677,8 +679,8 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
677679
{
678680
"@type": "/cosmos.gov.v1beta1.MsgVote",
679681
"voter": "` + icaAddress + `",
680-
"proposal_id": 1,
681-
"option": 1
682+
"proposal_id": "1",
683+
"option": "VOTE_OPTION_YES"
682684
}
683685
]
684686
}`)
@@ -695,7 +697,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
695697
true,
696698
},
697699
{
698-
"interchain account successfully executes govtypes.MsgSubmitProposal, govtypes.MsgDeposit, and then govtypes.MsgVote sequentially",
700+
"success: interchain account successfully executes govtypesv1.MsgSubmitProposal, govtypesv1.MsgDeposit, and then govtypesv1.MsgVote sequentially",
699701
func(icaAddress string) {
700702
msgBytes := []byte(`{
701703
"messages": [
@@ -710,16 +712,16 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
710712
"proposer": "` + icaAddress + `"
711713
},
712714
{
713-
"@type": "/cosmos.gov.v1beta1.MsgDeposit",
714-
"proposal_id": 1,
715+
"@type": "/cosmos.gov.v1.MsgDeposit",
716+
"proposal_id": "1",
715717
"depositor": "` + icaAddress + `",
716718
"amount": [{ "denom": "stake", "amount": "10000000" }]
717719
},
718720
{
719721
"@type": "/cosmos.gov.v1beta1.MsgVote",
720722
"voter": "` + icaAddress + `",
721-
"proposal_id": 1,
722-
"option": 1
723+
"proposal_id": "1",
724+
"option": "VOTE_OPTION_YES"
723725
}
724726
]
725727
}`)
@@ -730,13 +732,13 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
730732
"data":` + byteArrayString + `
731733
}`)
732734

733-
params := types.NewParams(true, []string{sdk.MsgTypeURL((*govtypes.MsgSubmitProposal)(nil)), sdk.MsgTypeURL((*govtypes.MsgDeposit)(nil)), sdk.MsgTypeURL((*govtypes.MsgVote)(nil))})
735+
params := types.NewParams(true, []string{sdk.MsgTypeURL((*govtypes.MsgSubmitProposal)(nil)), sdk.MsgTypeURL((*govv1.MsgDeposit)(nil)), sdk.MsgTypeURL((*govtypes.MsgVote)(nil))})
734736
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
735737
},
736738
true,
737739
},
738740
{
739-
"interchain account successfully executes transfertypes.MsgTransfer",
741+
"success: interchain account successfully executes transfertypes.MsgTransfer",
740742
func(icaAddress string) {
741743
transferPath := ibctesting.NewTransferPath(suite.chainB, suite.chainC)
742744

@@ -751,8 +753,9 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
751753
"token": { "denom": "stake", "amount": "100" },
752754
"sender": "` + icaAddress + `",
753755
"receiver": "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk",
754-
"timeout_height": { "revision_number": 1, "revision_height": 100 },
755-
"timeout_timestamp": 0
756+
"timeout_height": { "revision_number": "1", "revision_height": "100" },
757+
"timeout_timestamp": "0",
758+
"memo": ""
756759
}
757760
]
758761
}`)
@@ -769,7 +772,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
769772
true,
770773
},
771774
{
772-
"unregistered sdk.Msg",
775+
"failure: unregistered sdk.Msg",
773776
func(icaAddress string) {
774777
msgBytes := []byte(`{"messages":[{}]}`)
775778
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",")
@@ -785,7 +788,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
785788
false,
786789
},
787790
{
788-
"message type not allowed banktypes.MsgSend",
791+
"failure: message type not allowed banktypes.MsgSend",
789792
func(icaAddress string) {
790793
msgBytes := []byte(`{
791794
"messages": [
@@ -810,7 +813,7 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
810813
false,
811814
},
812815
{
813-
"unauthorised: signer address is not the interchain account associated with the controller portID",
816+
"failure: signer address is not the interchain account associated with the controller portID",
814817
func(icaAddress string) {
815818
msgBytes := []byte(`{
816819
"messages": [
@@ -834,6 +837,113 @@ func (suite *KeeperTestSuite) TestJSONOnRecvPacket() {
834837
},
835838
false,
836839
},
840+
{
841+
"failure: missing optional metadata field",
842+
func(icaAddress string) {
843+
proposal, err := govv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govv1.DefaultStartingProposalID, suite.chainA.GetContext().BlockTime(), suite.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
844+
suite.Require().NoError(err)
845+
846+
err = suite.chainB.GetSimApp().GovKeeper.SetProposal(suite.chainB.GetContext(), proposal)
847+
suite.Require().NoError(err)
848+
err = suite.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(suite.chainB.GetContext(), proposal)
849+
suite.Require().NoError(err)
850+
851+
msgBytes := []byte(`{
852+
"messages": [
853+
{
854+
"@type": "/cosmos.gov.v1.MsgVote",
855+
"voter": "` + icaAddress + `",
856+
"proposal_id": "1",
857+
"option": "VOTE_OPTION_YES"
858+
}
859+
]
860+
}`)
861+
// this is the way cosmwasm encodes byte arrays by default
862+
// golang doesn't use this encoding by default, but it can still deserialize:
863+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
864+
865+
packetData = []byte(`{
866+
"type": 1,
867+
"data":` + byteArrayString + `
868+
}`)
869+
870+
params := types.NewParams(true, []string{"*"})
871+
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
872+
},
873+
false,
874+
},
875+
{
876+
"failure: alternative int representation of enum field",
877+
func(icaAddress string) {
878+
proposal, err := govv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govv1.DefaultStartingProposalID, suite.chainA.GetContext().BlockTime(), suite.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
879+
suite.Require().NoError(err)
880+
881+
err = suite.chainB.GetSimApp().GovKeeper.SetProposal(suite.chainB.GetContext(), proposal)
882+
suite.Require().NoError(err)
883+
err = suite.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(suite.chainB.GetContext(), proposal)
884+
suite.Require().NoError(err)
885+
886+
msgBytes := []byte(`{
887+
"messages": [
888+
{
889+
"@type": "/cosmos.gov.v1.MsgVote",
890+
"voter": "` + icaAddress + `",
891+
"proposal_id": "1",
892+
"option": 1,
893+
"metadata": ""
894+
}
895+
]
896+
}`)
897+
// this is the way cosmwasm encodes byte arrays by default
898+
// golang doesn't use this encoding by default, but it can still deserialize:
899+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
900+
901+
packetData = []byte(`{
902+
"type": 1,
903+
"data":` + byteArrayString + `
904+
}`)
905+
906+
params := types.NewParams(true, []string{"*"})
907+
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
908+
},
909+
false,
910+
},
911+
{
912+
"failure: alternative integer field representation",
913+
func(icaAddress string) {
914+
proposal, err := govv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govv1.DefaultStartingProposalID, suite.chainA.GetContext().BlockTime(), suite.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
915+
suite.Require().NoError(err)
916+
917+
err = suite.chainB.GetSimApp().GovKeeper.SetProposal(suite.chainB.GetContext(), proposal)
918+
suite.Require().NoError(err)
919+
err = suite.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(suite.chainB.GetContext(), proposal)
920+
suite.Require().NoError(err)
921+
922+
msgBytes := []byte(`{
923+
"messages": [
924+
{
925+
"@type": "/cosmos.gov.v1.MsgVote",
926+
"voter": "` + icaAddress + `",
927+
"proposal_id": 1,
928+
"option": "VOTE_OPTION_YES",
929+
"metadata": ""
930+
}
931+
]
932+
}`)
933+
// this is the way cosmwasm encodes byte arrays by default
934+
// golang doesn't use this encoding by default, but it can still deserialize:
935+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
936+
937+
packetData = []byte(`{
938+
"type": 1,
939+
"data":` + byteArrayString + `
940+
}`)
941+
942+
params := types.NewParams(true, []string{"*"})
943+
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
944+
},
945+
false,
946+
},
837947
}
838948

839949
for _, tc := range testCases {
@@ -897,3 +1007,8 @@ func (suite *KeeperTestSuite) fundICAWallet(ctx sdk.Context, portID string, amou
8971007
suite.Require().NotEmpty(res)
8981008
suite.Require().NoError(err)
8991009
}
1010+
1011+
func getTestProposalMessage() sdk.Msg {
1012+
_, _, addr := testdata.KeyTestPubAddr()
1013+
return banktypes.NewMsgSend(authtypes.NewModuleAddress("gov"), addr, sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(1000))))
1014+
}

‎modules/apps/27-interchain-accounts/types/codec.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package types
22

33
import (
4+
"encoding/json"
5+
"reflect"
6+
47
"github.com/cosmos/gogoproto/proto"
58

69
errorsmod "cosmossdk.io/errors"
@@ -90,6 +93,15 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M
9093
if err := cdc.UnmarshalJSON(data, &cosmosTx); err != nil {
9194
return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot unmarshal CosmosTx with proto3 json")
9295
}
96+
reconstructedData, err := cdc.MarshalJSON(&cosmosTx)
97+
if err != nil {
98+
return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot remarshal CosmosTx with proto3 json: %v", err)
99+
}
100+
if isEqual, err := equalJSON(data, reconstructedData); err != nil {
101+
return nil, errorsmod.Wrapf(ErrUnknownDataType, "cannot compare original and reconstructed JSON: %v", err)
102+
} else if !isEqual {
103+
return nil, errorsmod.Wrapf(ErrUnknownDataType, "original and reconstructed JSON objects do not match, original: %s, reconstructed: %s", string(data), string(reconstructedData))
104+
}
93105
default:
94106
return nil, errorsmod.Wrapf(ErrInvalidCodec, "unsupported encoding format %s", encoding)
95107
}
@@ -107,3 +119,14 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M
107119

108120
return msgs, nil
109121
}
122+
123+
func equalJSON(a, b []byte) (bool, error) {
124+
var x, y any
125+
if err := json.Unmarshal(a, &x); err != nil {
126+
return false, err
127+
}
128+
if err := json.Unmarshal(b, &y); err != nil {
129+
return false, err
130+
}
131+
return reflect.DeepEqual(x, y), nil
132+
}

0 commit comments

Comments
 (0)