Skip to content

ndmean: prevent blockshape overflow in block size validation#786

Open
metsw24-max wants to merge 6 commits into
Blosc:mainfrom
metsw24-max:ndmean-blockshape-overflow-validation
Open

ndmean: prevent blockshape overflow in block size validation#786
metsw24-max wants to merge 6 commits into
Blosc:mainfrom
metsw24-max:ndmean-blockshape-overflow-validation

Conversation

@metsw24-max

Copy link
Copy Markdown
Contributor

Harden the NDMEAN filter against malformed or attacker-controlled b2nd metadata by validating block geometry using 64-bit arithmetic and rejecting invalid block dimensions.

Previously, ndmean_forward() and ndmean_backward() computed:

blocksize = typesize * product(blockshape)

using 32-bit arithmetic. A crafted blockshape could cause the multiplication to wrap around and produce a value equal to length, allowing the size validation to succeed despite the true block geometry being much larger.

Subsequent indexing operations would then use the actual dimensions derived from the metadata, potentially resulting in out-of-bounds memory accesses.

Changes

  • Compute block size using int64_t instead of int32_t.

  • Reject non-positive block dimensions (blockshape[i] <= 0).

  • Abort when the running block size exceeds the expected block length.

  • Use 64-bit comparisons when validating length == blocksize.

  • Update diagnostic logging for 64-bit values.

  • Add a regression test covering:

    • 32-bit multiplication wraparound onto length
    • Zero block dimensions
    • Negative block dimensions

Security Impact

This change prevents malformed b2nd metadata from bypassing block-size validation through integer overflow and eliminates a path that could lead to heap out-of-bounds accesses during NDMEAN processing.

Testing

Added test_ndmean_oversized and verified rejection of:

  • Overflowing blockshape products
  • Zero-sized dimensions
  • Negative dimensions

@metsw24-max

Copy link
Copy Markdown
Contributor Author

@FrancescAlted any update on this?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the ndmean filter against malformed/attacker-controlled b2nd metadata by computing block size in 64-bit arithmetic, rejecting invalid block geometry early, and adding a regression test to prevent overflow-based validation bypasses that could lead to out-of-bounds writes during NDMEAN decompression.

Changes:

  • Switch ndmean_forward() / ndmean_backward() blocksize computation to int64_t, reject non-positive blockshape[], and abort when the running product exceeds length.
  • Update diagnostics to log 64-bit block sizes where relevant.
  • Add test_ndmean_oversized and wire it into the plugin test CMake config.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
plugins/filters/ndmean/ndmean.c Uses 64-bit arithmetic and adds validation to prevent blockshape overflow bypassing size checks.
plugins/filters/ndmean/test_ndmean_oversized.c Adds regression coverage for wraparound, zero, and negative block dimensions.
plugins/filters/ndmean/CMakeLists.txt Builds and registers the new regression test under BUILD_TESTS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +76 to 80
// See ndmean_backward(): validate the b2nd block geometry in 64-bit so a
// crafted blockshape cannot wrap a 32-bit product back onto `length` and make
// the index arithmetic below stray outside the `length`-sized buffers.
int64_t blocksize = (int64_t) typesize;
for (int i = 0; i < ndim; i++) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. meta is the attacker-controlled cell shape and is cast to int8_t before being used as the divisor in i_shape[i] = (blockshape[i] + cellshape[i] - 1) / cellshape[i], so meta == 0 divides by zero and meta > 127 goes negative. Added a (int8_t) meta <= 0 rejection right after the ndim check (before cellshape[] is built). Fixed.

Comment on lines 283 to +287
uint8_t *ip = (uint8_t *) input;
uint8_t *ip_limit = ip + length;
uint8_t *op = (uint8_t *) output;
int32_t blocksize = (int32_t) typesize;
if (typesize <= 0) {
free(shape);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed. Same root cause as the forward path: meta (cell shape) is cast to int8_t and used as the divisor for i_shape[]. Beyond the divide-by-zero, a negative cast (meta > 127) also produces a negative pad_shape[ndim-1], which would wrap to a huge size_t in the memcpy. Added a positive-cell-shape guard before cellshape[] is built, and covered both meta=0 and meta=200 in test_ndmean_oversized.c.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

plugins/filters/ndmean/ndmean.c:351

  • The new length/blocksize validation in ndmean_backward drops the diagnostic details. Including both values (and using a 64-bit format) makes it much easier to triage malformed frames and aligns with the forward path’s logging.
  if ((int64_t) length != blocksize) {
    free(shape);
    free(chunkshape);
    free(blockshape);
    BLOSC_TRACE_ERROR("Length not equal to blocksize");

Comment on lines +89 to +92
// See ndmean_backward(): validate the b2nd block geometry in 64-bit so a
// crafted blockshape cannot wrap a 32-bit product back onto `length` and make
// the index arithmetic below stray outside the `length`-sized buffers.
int64_t blocksize = (int64_t) typesize;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — cell_size is the product of up to ndim cellshape[i] values (each ≤127), so it can reach 127^ndim and overflow int before the geometry is rejected (signed-overflow UB). Changed cell_size to int64_t; the later cell_size * typesize is now also evaluated in 64-bit. Fixed.

Comment on lines 310 to +314
uint8_t *ip = (uint8_t *) input;
uint8_t *ip_limit = ip + length;
uint8_t *op = (uint8_t *) output;
int32_t blocksize = (int32_t) typesize;
if (typesize <= 0) {
free(shape);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix applied here — cell_size is now int64_t, so accumulating cellshape[i] over the attacker-controlled geometry can't overflow signed int even on the rejection paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants