Unbounded field lengths

[faddat]

Summary of Bug

In ibc-go, there are many unbounded field lengths, and in production, this can result in ibc messages that are as large as several megabytes, which are seen as valid and relayed.

Expected Behaviour

With the exception of the total size of client updates, I'm not aware of any ibc transaction that benefits from a field length of over 5kb for any field. Even if this is incorrect and we need to exceed 5kb in some places, we need to introduce bounds to fields in ibc for safety.

Version

all

Steps to Reproduce

Contact Jacob Gadikian or another Notional team member via slack for information on reproduction

fix

We should ensure that every field in IBC has a maximum length. Marko from the SDK team says that length limitations are done on a per module basis in the SDK, and so IBC should comply. We are checking internally to determine if we have time / financial resources to complete this task.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[crodriguezvega]

Thank you for opening the PR, @faddat.

On a first pass, I think we have the following string fields whose maximum length is not checked:

• receiver in MsgTransfer (fix: limit receive addresses to 2kb #2983)
• memo in MsgTransfer (fix(statemachine)!: add check for length of memo field in MsgTransfer #4915)
• counterparty_payee in MsgRegisterCounterpartyPayee (fix(statemachine)!: add check for length of counterparty payee in MsgRegisterCounterpartyPayee #4870)
• owner in MsgRegisterInterchainAccount (fix(statemachine)!: add check for length of owner in MsgRegisterInterchainAccount #4874)
• version in MsgRegisterInterchainAccount - I think I will skip this one, since the version is passed to MsgChannelOpenInit and its validate basic should catch if the field is too long.
• owner in MsgSendTx (fix(statemachine)!: add check for length of owner in MsgSendTx  #4875)
• Channel handshake messages (fix(statemachine)!: add check for length of version and counterparty version in channel handshake messages #4877):
  • version of channel in MsgChannelOpenInit
  • version of channel and counterparty_version in MsgChannelOpenTry
  • counterparty_version in MsgChannelOpenAck
• Connection handshake messages (fix(statemachine)!: add check for length of version fields in connection handshake messages #4878):
  • The string fields of version in MsgConnectionOpenInit
  • The string fields of counterparty_versions in MsgConnectionOpenTry
  • The string field of version in MsgConnectionOpenAck

PRs should be opened against this feature branch.

[faddat]

Thank you!

[tac0turtle]

technically every module in existence may have this issue, it would be good to understand why longer length cause larger amounts of computation on certain fields over others. I dont think this is limited to ibc.

[faddat]

It is unfortunate that this was not addressed in a serious and rigorous manner when it was reported in October 2022, and I mention this because the reason we are scrambling does not relate to the software changes needed, but instead to a set of process-driven failures caused by the opaque ways of working and general unaccountability of the Interchain foundation, coupled with their legal threats against security researchers, and their security team's lack of good practices.

cosmos/security#19

Later, a chain-comber found the osmosis banana kings:

https://twitter.com/web3_analyst/status/1635687287962112000

Given that Amulet has released information about a related matter direct to the public on the cosmos hub forum, I will be releasing a set of targeted mitigations that cite this issue today at midnight Bali time, which is 4PM GMT.

The mitigations are available in the p2p-storms channel, as well as the validator working group on spam, and feedback from all teams is welcome, but timeline cannot be extended beyond then due to amulet's release of related information against my explicit and direct wishes.

• https://forum.cosmos.network/t/amulet-security-advisory-for-cometbft-asa-2023-002/11604/14

@tac0turtle can you think of a rapid way to check modules for unbounded field length, or should the suggested mitigation be to instruct authors of modules to ensure that every field has a length limitation?

[tac0turtle]

almost all fields in existence, you would need check each by hand, but this doesnt answer the question at hand, why does this tx take a long time to confirm? is it the bech32 decoding? Can we get a test case in ibc for a single message type to test what is going on here before making a bunch of length checks.

the fix overall is to reduce total block sizes, there is no need to have 10mb+ blocks. That would prevent the tweet issue. I havent seen the submitted report so dont know if a test case was added, if so can you point me to a gist i can run locally to test?

[faddat]

@tac0turtle can you check HackerOne?

https://twitter.com/ctrl_Felix/status/1714714816625934383?t=0TTLNJYmd07vt3G52Lj2Bg&s=19

Felix mentioned that he reported this -- even before contacting me. I thought the order was the other way around though it doesn't really matter what the order is. Issues slip by and that's bad.