Skip to content

decode/ipv6: bound hop-by-hop option to the option area - #15695

Closed
uwezkhan wants to merge 1 commit into
OISF:mainfrom
uwezkhan:ipv6-hopopts-bound
Closed

decode/ipv6: bound hop-by-hop option to the option area#15695
uwezkhan wants to merge 1 commit into
OISF:mainfrom
uwezkhan:ipv6-hopopts-bound

Conversation

@uwezkhan

@uwezkhan uwezkhan commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Link to ticket: https://redmine.openinfosecfoundation.org/issues/

Describe changes:

The TLV loop for IPv6 hop-by-hop and destination option headers gates each option on (offset + 1 + ip6_optlen) > optslen. An option is two header bytes (type, length) plus ip6_optlen data bytes, so the value ends at offset + 1 + ip6_optlen and the bound has to be offset + 2 + ip6_optlen. Before: an option that declares its data running one byte past the option area is accepted, and the Router Alert / Jumbo / Home Address memcpy then copies sizeof(value) bytes ending one byte past the area; when the header sits at the tail of the IPv6 payload that read goes past the payload.

After: the option is rejected with IPV6_EXTHDR_INVALID_OPTLEN, matching how the IPv4 option loop already bounds a full option against the remaining length. The check stays next to the read so each option type does not need its own guard. Added DecodeIPV6HopOptLenTest01, which fails on the current tree and passes with this change.

@uwezkhan
uwezkhan requested a review from victorjulien as a code owner June 22, 2026 11:19
@victorjulien

Copy link
Copy Markdown
Member

@uwezkhan please have a look at the CLA. If you're not able or willing to sign it, that is fine. In that case just file bug reports. But please no more PRs until this is cleared up.

@uwezkhan

Copy link
Copy Markdown
Contributor Author

Hey @victorjulien i have signed CLA , looking forward for a review

@catenacyber catenacyber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This work will need green CI, a redmine ticket, and we would prefer the test as a suricata-verify test

@uwezkhan
uwezkhan force-pushed the ipv6-hopopts-bound branch from 696a3b1 to 36e56dd Compare June 23, 2026 16:49
@github-actions

Copy link
Copy Markdown

Closing this pull request: its branch was updated after the pull
request was opened.

Per our workflow, a new pull request is required when changes are made
to an existing one. Please open a new pull request with the updated
changes.

If you wish to create an in progress pull request that you can push to,
please create a draft pull request.

Please see our GitHub Pull Request Workflow.

@uwezkhan

Copy link
Copy Markdown
Contributor Author

@catenacyber pushed an update.

  • Moved the test out of the C unit tests into suricata-verify: tests: ipv6 hop-by-hop option length past option area suricata-verify#3188 (same branch name so the verify run pairs with this PR). This PR is now just the one-line bounds fix.
  • Rebased onto current main. The earlier red was the shared "Prepare cbindgen" step failing to pull a crate from crates.io (HTTP2 framing error), not this change, so the rebase should get a clean run.
  • I'll file a redmine ticket and add the reference.

Before: the option was bounded on offset + 1 + ip6_optlen, so one whose data ends a byte past the option area was accepted and the Router Alert memcpy read that byte. After: bounded on offset + 2 + ip6_optlen (the type/len header bytes), so it's rejected as exthdr_invalid_optlen.

@catenacyber

Copy link
Copy Markdown
Contributor

Could you please open a new PR as per our workflow ?

@uwezkhan

uwezkhan commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Opened a fresh one as per the workflow: #15796. Rebased onto current main, single one-line bounds fix, and the regression test moved to suricata-verify (OISF/suricata-verify#3215). Let's continue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants