inflate: add opt-in limit on Huffman table rebuilds (fixes #198)#199
Open
scadastrangelove wants to merge 1 commit into
Open
inflate: add opt-in limit on Huffman table rebuilds (fixes #198)#199scadastrangelove wants to merge 1 commit into
scadastrangelove wants to merge 1 commit into
Conversation
…lexity DoS) Every BTYPE=1/BTYPE=2 deflate block header forces a full O(FAST_LOOKUP_SIZE + MAX_HUFF_TREE_SIZE) rebuild of the decoder's Huffman tables in init_tree(), regardless of how many symbols the block actually encodes. Since DEFLATE allows chaining an unbounded number of legal, minimal, near-empty blocks (a static block containing only an end-of-block code is 10 bits), a ~1.25MB crafted input made of a million such blocks forces over a second of CPU time while producing zero bytes of output. Because output never grows, the existing output-size limit (decompress_to_vec_with_limit) never triggers and provides no protection against this. This is legal (if degenerate) DEFLATE, not malformed input, so it can't be rejected outright without risking legitimate decompression. Add a second, independent, opt-in limit instead: - DecompressorOxide::set_max_huffman_table_rebuilds() bounds the number of table rebuilds a single stream is allowed to trigger before decompression aborts with the new TINFLStatus::HuffmanTableRebuildLimitExceeded status. Defaults to u32::MAX (unlimited), so existing callers are unaffected unless they opt in. - decompress_to_vec_with_limits() / decompress_to_vec_zlib_with_limits() expose the same knob at the to-Vec convenience layer, alongside the existing *_with_limit() output-size-only variants. - The budget is checked in init_tree() before doing any of the expensive table-clearing/construction work, so an exceeded budget is cheap to detect. The counter lives on DecompressorOxide itself (reset per-stream in the Start state, like the other transient decode fields) rather than as a new decompress()/decompress_with_limit() parameter, so no existing function signature changes and streaming users (via InflateState::decompressor()) can opt in too. Adds regression tests reproducing the attack (a from-scratch DEFLATE bit writer emitting N minimal static-Huffman blocks) at the core and to-Vec API layers, confirming: unbounded decompression of such a stream remains legal and unaffected (no existing behavior regressed), a configured limit stops decompression after a tiny fraction of a large attack payload, and the limit has no effect on legitimate round-trip decompression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Fixes #198.
Summary
Adds an opt-in
max_huffman_table_rebuildsbudget that bounds the number of timesinit_tree()(the O(1024+576) Huffman decode-table rebuild) can run during a single decompression call, closing the algorithmic-complexity DoS described in #198: a chain of minimal, degenerate DEFLATE blocks that each produce 0 bytes of output can force the expensive table rebuild arbitrarily many times, with the existing_with_limitoutput-size guard never engaging since nothing is ever written.Design
DecompressorOxideas opt-in state, defaulting tou32::MAX(unlimited) — fully backward compatible forflate2and every other consumer.DecompressorOxide::set_max_huffman_table_rebuilds(&mut self, u32)InflateState::decompressor()decompress_to_vec_with_limits/decompress_to_vec_zlib_with_limits, bounding both output size and rebuild count alongside the existing_with_limitfunctions.TINFLStatus::HuffmanTableRebuildLimitExceededvariant (the enum is already#[non_exhaustive]) rather than reusingFailed, so callers can distinguish "hit your own budget" from "corrupt data."Alternatives considered and rejected
Testing
--features block-boundary, and--features serde,std.cargo clippy --all-targetsshows the same pre-existing warnings before/after, zero new ones.miniz_oxide/tests/test.rs:DecompressorOxide) stops early once the rebuild budget is exceeded.decompress_to_vec_with_limitsrejects the attack payload.max_huffman_table_rebuilds = 100. A 9 MB legitimate round-trip (levels 1/6/9) is byte-identical and timing-indistinguishable with a generous 200,000-rebuild budget engaged.Open questions for maintainer judgment
MAXvalue is set, deliberately — a "safe" value depends entirely on caller workload/hardware. Open to discussion if you think a conservative default makes sense here (though it would be a behavior change for existing callers).from_block_boundary_state()does not currently carry over a previously configured limit when resuming from a serializedBlockBoundaryState— noted in a doc comment, but flagging explicitly in case that gap matters for the block-boundary/streaming-resume use case.Discovered by the rust-in-peace security pipeline.
🤖 Generated with Claude Code