Fix heap buffer overflow in NDCELL decompression path#785
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens the NDCELL filter’s decompression (and compression) paths against attacker-controlled b2nd metalayer geometry by preventing 32-bit overflow/wraparound in block-size validation, and adds a regression test to prevent reintroducing the issue.
Changes:
- Compute and validate
blocksizeusing 64-bit arithmetic and reject invalidtypesize/blockshapevalues early. - Add a regression test that exercises wraparound and invalid blockshape dimensions.
- Wire the new regression test into the NDCELL plugin CMake/CTest targets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugins/filters/ndcell/ndcell.c | Adds 64-bit blocksize validation and stricter geometry checks in forward/backward paths. |
| plugins/filters/ndcell/test_ndcell_oversized_blockshape.c | Introduces a regression test targeting oversized/wrapping and invalid blockshape metadata. |
| plugins/filters/ndcell/CMakeLists.txt | Builds and registers the new regression test with CTest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int8_t cell_shape = (int8_t) meta; | ||
| const int cell_size = (int) pow(cell_shape, ndim); |
There was a problem hiding this comment.
Changed int8_t cell_shape = (int8_t) meta; to int cell_shape = (int) meta; and added a cell_shape <= 0 guard before pow()/division. meta is a uint8_t (0–255); the old int8_t cast made 0 a divide-by-zero in (blockshape[i] + cell_shape - 1) / cell_shape and turned 128–255 into negative geometry.
| @@ -188,12 +212,42 @@ int ndcell_backward(const uint8_t *input, uint8_t *output, int32_t length, uint8 | |||
| uint8_t *ip = (uint8_t *) input; | |||
| uint8_t *ip_limit = ip + length; | |||
There was a problem hiding this comment.
Same fix on the attacker-controlled decompression path, placed before cell_size/division use.
| uint8_t *input = calloc(1, (size_t) (length > 0 ? length : 1)); | ||
| uint8_t *output = malloc((size_t) (length > 0 ? length : 1)); | ||
|
|
||
| int rc = ndcell_backward(input, output, length, cell_shape, &dparams, 0); | ||
|
|
||
| free(input); | ||
| free(output); | ||
| blosc2_schunk_free(schunk); |
There was a problem hiding this comment.
"Length not equal to blocksize" now logs both values: "...%d %lld", length, (long long) blocksize, matching the forward path.
| if ((int64_t) length != blocksize) { | ||
| free(shape); | ||
| free(chunkshape); | ||
| free(blockshape); |
There was a problem hiding this comment.
Added a 64-byte 0xAC canary immediately after the output buffer and a post-call check, so a partial scatter-write that precedes the error return is caught — not just the return code. Added <string.h> for memset.
| // `meta` is the cell side length and comes from the chunk header (uint8_t, | ||
| // 0..255). Interpret it as unsigned and reject 0: an (int8_t) cast would turn | ||
| // 128..255 into negative values and 0 would later divide by zero in | ||
| // `(blockshape[i] + cell_shape - 1) / cell_shape`. | ||
| int cell_shape = (int) meta; | ||
| if (cell_shape <= 0) { | ||
| free(shape); | ||
| free(chunkshape); | ||
| free(blockshape); | ||
| BLOSC_TRACE_ERROR("Invalid cell_shape %d", cell_shape); | ||
| return BLOSC2_ERROR_FAILURE; | ||
| } | ||
| const int cell_size = (int) pow(cell_shape, ndim); |
There was a problem hiding this comment.
Fixed. cell_size is now int64_t, computed with checked multiplication that bails with BLOSC2_ERROR_FAILURE if the running product would exceed INT64_MAX — no more (int) pow() overflow/UB. The downstream guard is now cell_size > (int64_t) length / typesize, which can't overflow.
| // `meta` is attacker-controlled in a crafted frame. Interpret it as unsigned | ||
| // and reject 0: an (int8_t) cast would turn 128..255 into negative values and | ||
| // 0 would later divide by zero in `(blockshape[i] + cell_shape - 1) / cell_shape`. | ||
| int cell_shape = (int) meta; | ||
| if (cell_shape <= 0) { | ||
| free(shape); | ||
| free(chunkshape); | ||
| free(blockshape); | ||
| BLOSC_TRACE_ERROR("Invalid cell_shape %d", cell_shape); | ||
| return BLOSC2_ERROR_FAILURE; | ||
| } | ||
| int cell_size = (int) pow(cell_shape, ndim); | ||
| int32_t typesize = schunk->typesize; |
There was a problem hiding this comment.
Same fix applied here on the decompression path. Since meta is attacker-controlled, this matters more — checked 64-bit multiply rejects oversized cell_shape^ndim before it can bypass the buffer-size check, and the check itself is overflow-safe (length == blocksize guarantees the division is exact).
| int ndcell_backward(const uint8_t *input, uint8_t *output, int32_t length, uint8_t meta, | ||
| blosc2_dparams *dparams, uint8_t id); |
There was a problem hiding this comment.
Removed. ndcell.h is already included and declares ndcell_backward, so I dropped the local prototype to avoid signature drift.
|
LGTM. Thanks @saddamr3e ! |
Fix a heap buffer overflow in the NDCELL filter caused by insufficient validation of block geometry embedded in the attacker-controlled
b2ndmetalayer.Root Cause
ndcell_forward()andndcell_backward()computed:using 32-bit arithmetic. A crafted
blockshapecould cause the product to wrap around and match the supplied block length, allowing malformed metadata to bypass validation.Subsequent index calculations used the actual block dimensions, which could lead to out-of-bounds memory accesses during filter processing.
Fix
int64_t.typesizevalues.blockshapedimensions.Tests
Added a dedicated regression test covering: