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

ibc: Chain ID parsing incorrectly accepts newlines #4547

Open
avahowell opened this issue Jun 4, 2024 · 1 comment
Open

ibc: Chain ID parsing incorrectly accepts newlines #4547

avahowell opened this issue Jun 4, 2024 · 1 comment
Labels
consensus-breaking breaking change to execution of on-chain data _P-V1 Priority: slated for V1 release zellic-ibc-needs-remediation Finding by Zellic reviewing the IBC component, needs remediation

Comments

@avahowell
Copy link
Contributor

Describe the bug
When parsing chain IDs, ibc-types uses a more lenient regex than ibc-go's regex.

For example, ibc-go will consider the chain ID \n-foo-1 to have revision number 0, whereas penumbra-ibc will consider it to have revision number 1.

This likely does not have any security impact, but it is a deviation from the specification and can potentially lead to an issue in the future.

Expected behavior
Include a newline (and possibly anchors) in ibc-types's version of the regex.

pub fn is_epoch_format(chain_id: &str) -> bool {
  - let re = safe_regex::regex!(br".*[^-]-[1-9][0-9]*");
  + let re = safe_regex::regex!(br"^.*[^\n-]-[1-9][0-9]*$");
       re.is_match(chain_id.as_bytes())
   }

@avahowell avahowell added the zellic-ibc-needs-remediation Finding by Zellic reviewing the IBC component, needs remediation label Jun 4, 2024
@avahowell avahowell self-assigned this Jun 4, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra Jun 4, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Jun 4, 2024
@aubrika aubrika added _P-V1 Priority: slated for V1 release and removed needs-refinement unclear, incomplete, or stub issue that needs work labels Jun 4, 2024
@aubrika aubrika moved this from Backlog to Todo in Penumbra Jun 5, 2024
@aubrika aubrika added this to the Sprint 8 milestone Jun 5, 2024
@aubrika aubrika modified the milestones: Sprint 8, Sprint 9 Jul 15, 2024
@aubrika aubrika modified the milestones: Sprint 9, Sprint 10 Aug 12, 2024
@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Aug 26, 2024
@conorsch
Copy link
Contributor

We didn't fix this as an immediate result of the audit. It'd be nice to clean it up, but we should inspect where the chain id is used, particularly in IBC contexts like handshakes. Marking this issue as consensus-breaking so that a fix doesn't get shipped without upgrade/migration plans.

@aubrika aubrika removed this from the Sprint 10 milestone Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data _P-V1 Priority: slated for V1 release zellic-ibc-needs-remediation Finding by Zellic reviewing the IBC component, needs remediation
Projects
Status: Backlog
Development

No branches or pull requests

3 participants