Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revision and client identifier regex formats accept newlines #686

Closed
2 of 3 tasks
crodriguezvega opened this issue Jan 7, 2022 · 3 comments · Fixed by #724
Closed
2 of 3 tasks

Revision and client identifier regex formats accept newlines #686

crodriguezvega opened this issue Jan 7, 2022 · 3 comments · Fixed by #724
Assignees
Labels
audit Feedback from implementation audit core

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jan 7, 2022

This issue was found by Trail of Bits during the audit of ICS27 Interchain Accounts

Problem Definition

The IsRevisionFormat and IsClientIDFormat regular expressions (regexes) implemented in the 02-client module use the [^-] regex element to ensure that two components are delimited by only a single dash in the respective {chainID}-{revision} and {client-type}-{N} formats. However, this regex element also accepts newline characters, which could cause unwanted “revision” and “client identifier” formats to be accepted.

Proposal

  • Fix the IsRevisionFormat and IsClientIDFormat regexes by changing the [^-] element to [^\n-] so that newline characters are not accepted before the - delimiter.
  • Extend the TestParseClientIdentifier and TestParseChainID test cases to check for client and chain IDs that contain newline characters before the - delimiter.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega added 27-interchain-accounts audit Feedback from implementation audit labels Jan 7, 2022
@crodriguezvega crodriguezvega added this to the Interchain Accounts milestone Jan 7, 2022
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Jan 7, 2022
@colin-axner
Copy link
Contributor

I think newlines should be allowed in IsRevisionFormat. We shouldn't impose chainID naming restrictions more than appending the revision number

@crodriguezvega
Copy link
Contributor Author

We will fix only the client ID regex.

@crodriguezvega crodriguezvega moved this from Backlog to Todo in ibc-go Jan 10, 2022
@colin-axner colin-axner self-assigned this Jan 13, 2022
@crodriguezvega crodriguezvega moved this from Todo to In progress in ibc-go Jan 13, 2022
@colin-axner
Copy link
Contributor

On second thought the IsRevisionFormat should be updated. I didn't realize we already enforce chainIDs not to have newlines. The proposed change is just ensuring newlines aren't used before the -<revision number>. Since we are already imposing the naming restriction in of no newlines in the chainID to be a valid revision number, we can leave this as so until we receive requests otherwise

@crodriguezvega crodriguezvega moved this from In progress to In review in ibc-go Jan 14, 2022
Repository owner moved this from In review to Done in ibc-go Jan 17, 2022
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
Since `SignedHeaders` are gossiped, there is no use for `Commit`
gossiping. Entire related logic should be removed from `p2p.Client` and
node(s).

Closes: cosmos#686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit Feedback from implementation audit core
Projects
Archived in project
2 participants