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

The IsValidAddr regex accepts 0-length and excessively long addresses #687

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

The IsValidAddr regex accepts 0-length and excessively long addresses #687

crodriguezvega opened this issue Jan 7, 2022 · 0 comments · Fixed by #723
Assignees
Labels
27-interchain-accounts audit Feedback from implementation audit type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

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 IsValidAddr regex accepts 0-length addresses and addresses longer than the DefaultMaxAddrLength, which are not valid. The ValidateAccountAddress function mitigates this issue by erroring out if the address string length is 0 or if it is longer than the DefaultMaxAddrLength; however, because the regex is a public variable, other developers could use this regex directly instead of calling the ValidateAccountAddress function, introducing data validation bugs into the software.

Proposal

Change the IsValidAddr regex to use a + quantifier instead of * so that it will match 1 or more characters, rather than 0 or more characters, from the [a-zA-Z0-9] range. Additionally, consider either modifying the regex so that it takes DefaultMaxAddrLength into account or lowercasing the regex’s name to make it a private variable. This will help prevent bugs in the future if someone uses the regex directly instead of through the ValidateAccountAddress function.


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
@crodriguezvega crodriguezvega added the type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. label Jan 10, 2022
@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
Repository owner moved this from In progress to Done in ibc-go Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-interchain-accounts audit Feedback from implementation audit type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants