linux, l4re: address soundness issues of CMSG_NXTHDR#4891
Closed
gibbz00 wants to merge 6 commits intorust-lang:mainfrom
Closed
linux, l4re: address soundness issues of CMSG_NXTHDR#4891gibbz00 wants to merge 6 commits intorust-lang:mainfrom
CMSG_NXTHDR#4891gibbz00 wants to merge 6 commits intorust-lang:mainfrom
Conversation
Contributor
Author
|
Wanted to add the stable-nominated rustbot label, but cherry-picking locally resulted in conflict. Presumably because of the large refactor done in 2fe1d91. Should I ping rustbot anyway? |
gibbz00
force-pushed
the
unsound_cmsg_nxthdr
branch
2 times, most recently
from
e79eaa6 to
f087d3c
Compare
This change makes sure that the header of `next` is within max, returning null if not. This is similar to how `glibc` does it. No checks were previously being done to assert that `next as usize + size_of::<cmsghdr>() < max`. Wrapping offset calculations could then lead to buffer over-reads in the following `(*next).cmsg_len`. [glibc ref](https://github.com/bminor/glibc/blob/b71d59074b98ad4abd23c136ec9ad4c26e29ee6d/sysdeps/unix/sysv/linux/cmsg_nxthdr.c#L49-L51)
gibbz00
force-pushed
the
unsound_cmsg_nxthdr
branch
from
f087d3c to
1d968f9
Compare
Likely written to make assertions in the unsound CMSG_NXTHDR implementations introduced in rust-lang#1235. CMSG_NXTHDR(mhdr, current_cmsghdr) should not be concerned with the value next_cmsghdr.cmsg_len, which the previous implementation did.
Setting `(*pcmsghdr).cmsg_len = cmsg_len as _;` when cmsg_len ranges from 0 to 64 is invalid as it must always be `>= size_of::<cmsghdr>()`, rounded up to the nearest alignment boundary. Some implementations (notably glbic) do check that `cmsg_len >= size_of::<cmsghdr>()` in `CMSG_NXTHDR`, returning null if so. But this is more so an extra precaution that is not mentioned in the POSIX 1003.1-2024. It can therefore not be relied on for tests executed on multiple platforms. The change also removes the ignoring of some testvalues when targeting AIX.
gibbz00
force-pushed
the
unsound_cmsg_nxthdr
branch
from
94c34e9 to
44ae998
Compare
Contributor
Author
|
Closing in favor of #4903 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unsoundness
This change makes sure that the header of
nextis within max, returning null if not. This is similar to howglibcdoes it.It is the fix that should have been made #1235, but instead it added pretty severe soundness issues. (Mentioning it for context, not to point fingers.) No checks were previously being done to assert that
next as usize + size_of::<cmsghdr>() < max. Wrapping offset calculations could then lead to buffer over-reads in the following(*next).cmsg_len.Testing
This PR also cleans up the CMSG_NXTHDR test in terms of:
Musl issues
CI for
muslbegan breaking since making sure that the test execute at the controllen boundary. I'm fairly certain it comes down to musl doing a>=boundary check when other implementations, including ours, just do a>check, and that it is musl that does it wrong.Say for example that
msghr_addr = 0,msghdr.controllen = 24somax_addr = 24. This means that byte 0-23 (inclusive) are allowed to be part of cmsg, but not byte 24.If
current_addr = 10,current_len = 6, andheader_size = 8, then sure;next_addr = 10 + 6 + 8 = 24, but the bytes in use are still 10 to 23 (inclusive).musl is most likely being too aggressive when returning null just because
next_addr == max_addr.Not sure how this should be dealt with.
SendWait for a patch to musl first? Do a separate implementation for musl which mimics this "incorrect" behaviour? Disable testing of cmsg_nxthdr for musl?Edit:
Patch to musl submitted: https://www.openwall.com/lists/musl/2025/12/27/1