Bounds-check decompression against corrupted input#132
Open
alexey-milovidov wants to merge 11 commits into
Open
Bounds-check decompression against corrupted input#132alexey-milovidov wants to merge 11 commits into
alexey-milovidov wants to merge 11 commits into
Conversation
SZ3's decompression path reads sizes, counts and dimensions from the compressed stream and uses them to drive reads and writes without validating them against the size of the buffer. On corrupted or adversarial input (e.g. when fuzzing the decompressor) this leads to out-of-bounds reads and writes. This adds bounds checks along the decompression path: - MemoryUtil: the length-checked read() overloads now throw std::out_of_range instead of assert() (a no-op in release builds). - SZ_decompress: validate that the buffer is at least as large as the 16-byte header and that the stored payload size fits in the buffer. - Config::load: take the size of the config blob, bound every read against it, validate the dimension count (1..4) and per-dimension bit width, and check that the product of the dimensions equals the element count. Also fixes the end pointer, which was one byte past the end of the config (confSize already includes the length-prefix byte). - HuffmanEncoder::load/decode: validate the node count, the serialized tree size and the encoded length against the available bytes, and guard against null nodes from a malformed tree. Valid data is unaffected: the checks only fire on input that would otherwise read or write out of bounds.
On decompression these walked vectors filled from the compressed data using running indices that were not bounded against the vector sizes, so crafted input could read past their ends: - LinearQuantizer::recover_unpred indexed unpred[index++] unbounded, and load resized unpred to an untrusted count before the bounded read could reject it; - RegressionPredictor consumed N+1 coefficients per block without checking the coefficient vector size; - ComposedPredictor read selection[current_index++] and indexed predictors[sid] with an unbounded index and an unchecked predictor id. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
unpad_tree followed child indices read from the compressed data without checking them against the node count, so a crafted tree could read the L/R/C/t arrays out of bounds or form a cycle. pad_tree always assigns a child a higher index than its parent, so enforce i < child < nodeCount. Also reject a stateNum too small to hold nodeCount nodes, otherwise new_node2 would write past the end of the node pool. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
…ClickHouse) Read quant_inds_size with the bounded overload so a truncated compressed buffer can not be read past its end. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
SZ_decompress_OMP read the thread count, per-thread configs and per-thread compressed sizes from the compressed data without validating them against the buffer, so corrupted input could over-read the buffer and point a thread's slice out of bounds: - bound the thread-count read and reject a thread count larger than the buffer; - parse each per-thread config with the bounded Config::load overload; - bound the per-thread size array read; and - build the per-thread offsets with an overflow-safe bound so each slice stays within the remaining buffer. Found while integrating SZ3 into ClickHouse and fuzzing the decompressor: ClickHouse/ClickHouse#108788
…ickHouse) On the ALGO_LOSSLESS decompress path the dispatcher passes the pre-allocated output buffer (conf.num elements) to Lossless_zstd::decompress, but the decompressed size was read from the (untrusted) payload and used as the zstd destination capacity without checking it against the buffer. A crafted block could keep conf.num correct while declaring a larger lossless size, so zstd would write past the end of the buffer. Pass the buffer capacity into the lossless decoder and reject a payload that declares a larger decompressed size before zstd runs. Found while integrating SZ3 into ClickHouse and fuzzing the decompressor: ClickHouse/ClickHouse#108788
The generic lossy decompressor (`SZGenericCompressor::decompress`, used by `ALGO_INTERP` / `ALGO_LORENZO_REG` / `ALGO_INTERP_LORENZO`) reads the size of its internal buffer from the untrusted compressed payload and passed it to `Lossless_zstd::decompress` with `dst == nullptr` and no capacity, so a corrupted block could force an arbitrary `malloc(dstLen)` before any validation. Bound that allocation by the largest internal buffer the configuration could have produced: during compression the buffer is zstd-compressed and `ZSTD_compressBound(B) >= B`, so a stored generic-lossy block satisfies `B < SZ_compress_size_bound = 4096 + conf.size_est() + ZSTD_compressBound(conf.num * sizeof(T))`. `Lossless_zstd::decompress` now treats a non-zero incoming `dstLen` as an upper bound on the size it may allocate when `dst == nullptr`, and rejects a payload that declares more before allocating it. `conf.num` is validated against the trusted output size by the caller, so the bound can not be inflated by corrupted input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e (ClickHouse) Lossless_zstd::decompress validated that the declared decompressed size fits the output buffer, but never required ZSTD_decompress to actually produce that many bytes. A crafted ALGO_LOSSLESS block could declare a size matching the trusted output size while the zstd frame expands to fewer bytes, leaving the tail of the output buffer uninitialized; the caller then copies it out as if it were decompressed data. Require the produced size to equal the declared size. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
…kHouse) Two memory-safety gaps remained in the generic lossy decompression path, both reachable from a crafted (untrusted) compressed payload: 1. Validate the inner interpolation dimensions. `ALGO_INTERP` stores its own dimensions array inside the compressed payload (`InterpolationDecomposition`), separately from the trusted `Config::dims`. A block could keep `config.num` equal to the trusted output size while declaring larger interpolation dimensions, so the decompressor would iterate past the end of the output buffer and the decoded quantization vector. `InterpolationDecomposition::decompress` now rejects a block whose stored dimensions do not match the trusted configuration before it uses them for anything. 2. Make the internal scratch buffer ownership exception-safe. The generic decompressor allocated the internal buffer with a raw `malloc` and freed it only on the success path, so any of the parsing steps that run on untrusted data (`decomposition.load`, `encoder.load`, the quantization-index count read, `encoder.decode`) leaked it on a corrupted block. `SZGenericCompressor::decompress` now owns the buffer with RAII, and `Lossless_zstd::decompress` frees a buffer it allocated itself when zstd decompression fails. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…read (ClickHouse) Two sanitizer findings from the SZ3 codec unit tests. `LorenzoPredictor` formed out-of-bounds pointers during compression. The neighbour helpers computed `d[-offset]` with an unsigned `offset`, which is `*(d + (size_t)(-offset))` and wraps the pointer below the buffer. The accessed element is in bounds (the predictor pads by 2), but the pointer computation is undefined behavior and was flagged by `-fsanitize=pointer-overflow`. Compute the address with pointer subtraction so the offset stays a small negative step. `SZGenericCompressor` let `HuffmanEncoder::decode` read past the end of the decompressed scratch buffer on a corrupted or truncated block. `load` records the bytes remaining right after the Huffman tree and uses that as the bound for the encoded stream, but the compressor wrote the `quant_inds` count between the tree and the stream, so the recorded bound was `sizeof(size_t)` bytes too large. Move that count before the encoder so the tree is immediately followed by its encoded stream, as the predictor-side encoders already do. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Problem
The decompression path reads sizes, counts and dimensions from the compressed stream and uses them to drive reads and writes without validating them against the size of the input buffer. On corrupted or adversarial input (for example when fuzzing the decompressor) this leads to out-of-bounds reads and writes — a
decode()loop bounded only by the target length, a Huffman tree reconstructed from an unvalidated node count, an unchecked payload-size field inSZ_decompress, etc.Changes
This adds bounds checks along the decompression path. Valid data is unaffected — the checks only fire on input that would otherwise read or write out of bounds.
MemoryUtil: the length-checkedread()overloads (the ones that takeremaining_length) now throwstd::out_of_rangeinstead ofassert(), which is a no-op in release builds.SZ_decompress(sz.hpp): validate that the buffer is at least the 16-byte header and that the stored compressed-payload size fits in the remaining buffer.Config::load: take the size of the config blob and bound every read against it; validate the dimension count (1..4) and per-dimension bit width before reading the dimensions; and check that the product of the dimensions equals the element count. (This also includes the end-pointer fix from Fix Config::load reading one byte past the end of the config #131.)HuffmanEncoder::load/decode: validate the node count, the serialized tree size and the encoded length against the available bytes, and guard against null nodes produced by a malformed tree.HuffmanEncodertree reconstruction:unpad_treenow rejects child indices that are out of range (a valid tree always assigns a child a higher index than its parent, soparent < child < nodeCount), which prevents out-of-bounds reads of theL/R/C/tarrays and cycles; and astateNumtoo small to holdnodeCountnodes is rejected so the node pool can not overflow.LinearQuantizer:recover_unpredbounds its index against the unpredictable-value vector, andloadvalidates the unpredictable count against the remaining bytes before resizing.RegressionPredictor/ComposedPredictor: bound the per-block coefficient/selection indices against the decoded vectors, and validate the selected predictor id.Lossless_zstd::decompress/SZ_decompress_dispatcher: on theALGO_LOSSLESSpath the pre-allocated output buffer is now passed to the lossless decoder with its capacity, and a payload that declares a larger decompressed size is rejected before zstd writes — previously the size came from the payload and was used as the zstd destination capacity unchecked (a heap buffer overflow).SZGenericCompressor::decompress: read the quantization-index count with the bounded overload so a truncated buffer is not read past its end.SZ_decompress_OMP(the OpenMP decompression path): bound the thread-count read, parse each per-thread config with the boundedConfig::load, bound the per-thread size array, and build the per-thread payload offsets with an overflow-safe bound so each thread's slice stays within the buffer.Generic lossy path (
SZGenericCompressor::decompress): the internal buffer size is read from the payload and was allocated unbounded forALGO_INTERP/ALGO_LORENZO_REG. It is now bounded by the largest buffer the configuration could have produced (4096 + conf.size_est() + ZSTD_compressBound(conf.num * sizeof(T)), valid becauseZSTD_compressBound(B) >= B), passed as a capacity thatLossless_zstd::decompressenforces before allocating.Lossless_zstd::decompressfail-closed: also requireZSTD_decompressto produce exactly the declared size, so a frame that expands to fewer bytes can not leave the tail of the (caller-provided) output buffer uninitialized.Inner interpolation dimensions + RAII scratch buffer:
InterpolationDecomposition::decompressrejects a block whose separately-stored dimensions do not match the trusted config (otherwise it would iterate past the output buffer), andSZGenericCompressor::decompressowns its scratch buffer with RAII so a corrupted block can not leak it.Pointer-overflow UB + Huffman over-read (sanitizer findings):
LorenzoPredictorneighbour helpers computedd[-offset]with an unsigned offset (*(d + (size_t)(-offset)), a wrapped/out-of-bounds pointer flagged by-fsanitize=pointer-overflow) — now use pointer subtraction; and thequant_indscount is written before the encoder so its serialized tree is immediately followed by its stream, so the decode bound recorded byload()is no longer one field too large (which let a truncated block over-read).Notes
Config::loadgains a second overloadload(const unsigned char*&, size_t cmpSize); the existing one-argument overload is kept for internal/trusted callers and delegates with an unbounded size.Config::load(it incorporates the same end-pointer fix). If Fix Config::load reading one byte past the end of the config #131 is merged first this rebases trivially.Context
Found while integrating SZ3 as an experimental compression codec in ClickHouse and fuzzing the decompressor: ClickHouse/ClickHouse#108788