decode/ipv6: bound hop-by-hop option to the option area - #15796
Conversation
The TLV loop for the 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 its data 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.
|
The red CI wasn't from the decode change. The linked suricata-verify branch was based on a master that predates the FTP test updates (8.0.6 and the nlst change), so ruletype-firewall-113/114-ftp-upload and output-eve-ftp-data failed, and the unittests coverage job hit a container without rustc, which looks transient. I've rebased OISF/suricata-verify#3215 onto current master and closed/reopened here to retrigger, since a branch push would auto-close the PR per the workflow. The new runs are queued pending workflow approval; the one-line fix itself is unchanged. |
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
I think this is a good fix but we need a redmine ticket, the commit mentioning it (and I think we will want to backport it)
|
Makes sense, will file the redmine ticket and put the reference in the commit message. Since pushing to this branch would auto-close the PR, the updated commit will come as a new PR once the ticket exists. On the backport: the same bound is in main-7.0.x and main-8.0.x, so the one-liner applies to both. |
|
Ticket covering this issue from another 3rd party report https://redmine.openinfosecfoundation.org/issues/8817 |
|
Replaced by #15989 |
3 participants
Reopens #15695 as a fresh PR per the GitHub workflow (the previous one was auto-closed after its branch was updated). Rebased onto current main and now just the one-line bounds fix; the unit test lives in suricata-verify instead.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: filing on redmine and will add the reference here.
Describe changes:
The TLV loop for the 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) plusip6_optlendata bytes, so the value ends atoffset + 1 + ip6_optlenand the bound has to beoffset + 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
memcpythen copiessizeof(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.Addressing the earlier review: rebased for green CI, and the regression test is a suricata-verify test (OISF/suricata-verify#3215) rather than a C unit test.
SV_REPO=
SV_BRANCH=OISF/suricata-verify#3215
SU_REPO=
SU_BRANCH=