fix(buffer): bound alignment exponents read from untrusted input - #9155
Open
lorenzhs wants to merge 2 commits into
Open
fix(buffer): bound alignment exponents read from untrusted input#9155lorenzhs wants to merge 2 commits into
lorenzhs wants to merge 2 commits into
Conversation
## Rationale for this change Alignments in the file format are a power-of-two exponent in a `u8`, and `try_from_exponent` only rejects exponents that would overflow a `usize` shift. Satisfying an alignment can require copying the buffer into an allocation of `len + alignment` bytes, so an exponent of 40 on a 4-byte buffer asks the reader for 1 TiB. This cannot be contained by the caller: Rust aborts via `handle_alloc_error` when an allocation fails rather than unwinding, so it is a process abort rather than an error. Fuzzing the reader finds it from two directions, array-tree buffer descriptors and footer segment specs, with exponents from 32 to 61. - Related: vortex-data#8819 ## What changes are included in this PR? - `Alignment::MAX_UNTRUSTED` (4096) and `Alignment::try_from_untrusted_exponent`, which rejects anything above it. - Use it wherever an exponent comes from a file or message: array-tree buffer descriptors, footer segment specs, postscript segments, and IPC buffer messages. The IPC path used the panicking `from_exponent`. - Regression tests for the array-tree and footer-segment paths, and for the constructor's bounds. ## What APIs are changed? Are there any user-facing changes? `Alignment` gains a public const and constructor, both additive. Reading a file that declares a buffer or segment alignment above 4096 now errors where it previously succeeded. No writer in this repository records an alignment above `DEFAULT_ALIGNMENT` (256), so this only rejects files a writer could not have produced without a caller explicitly over-aligning a buffer first. Signed-off-by: Lorenz Hübschle <[email protected]>
Merging this PR will degrade performance by 11.17%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
lorenzhs
commented
Aug 3, 2026
| vortex_bail!("excessive buffer alignment must be rejected"); | ||
| }; | ||
| assert!( | ||
| err.to_string().contains("exceeds"), |
Contributor
Author
There was a problem hiding this comment.
before the fix, this died with memory allocation of 1099511627780 bytes failed
AdamGS
reviewed
Aug 4, 2026
| //github.com/ | ||
| //github.com/ A copy to satisfy an alignment allocates `len + alignment`, once per buffer, so this bounds | ||
| //github.com/ the amplification. 16x the [`Self::DEFAULT_ALIGNMENT`] every writer here records. | ||
| pub const MAX_UNTRUSTED: Self = Alignment::new(4096); |
Contributor
There was a problem hiding this comment.
I think we do want to support larger alignments, see #8599
Contributor
Author
There was a problem hiding this comment.
OK sure will bump to 64K
| //github.com/ | ||
| //github.com/ This admits 64KiB page alignment, as used on some ARM systems, while bounding the extra | ||
| //github.com/ allocation required to satisfy an alignment from untrusted input. | ||
| pub const MAX_UNTRUSTED: Self = Alignment::new(64 * 1024); |
Contributor
There was a problem hiding this comment.
see the discussion above, as far as I know this is the biggest alignment we ran into so far.
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
3 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.
Rationale for this change
Found a few places where untrusted alignments from input files are not properly validated and can lead to excessive allocation sizes / OOMs.
Alignments in the file format are a power-of-two exponent in a
u8, andtry_from_exponentonly rejects exponents that would overflow ausizeshift. Satisfying an alignment can require copying the buffer into an allocation oflen + alignmentbytes, so an exponent of 40 on a 4-byte buffer asks the reader for 1 TiB.This cannot be contained by the caller: Rust aborts via
handle_alloc_errorwhen an allocation fails rather than unwinding, so it is a process abort rather than an error. Fuzzing the reader found it from two directions, array-tree buffer descriptors and footer segment specs, with exponents from 32 to 61.SerializedArray::from_flatbuffer_and_segment(uncheckedBufferHandle::slice) #8819What changes are included in this PR?
Alignment::MAX_UNTRUSTED(64K) andAlignment::try_from_untrusted_exponent, which rejects anything above it.from_exponent.What APIs are changed? Are there any user-facing changes?
Alignmentgains a public const and constructor. Reading a file that declares a buffer or segment alignment above 65536 now errors where it previously succeeded. 64K is what some ARM chips use as page alignment, but larger alignments aren't useful here.I used Claude Code to investigate this and draw up the initial fix and write the tests. The issue itself was found using a libFuzzer-based harness.