Fix infinite loop in zlib_mem_inflate() when uncomp_size=1 (CWE-835)

[lrycro]

Describe

In cram/cram_io.c, zlib_mem_inflate() (the HAVE_LIBDEFLATE branch), the buffer-growth logic on LIBDEFLATE_INSUFFICIENT_SPACE was:

(*size) *= 1.5;

guarded by if (!*size) *size = csize*2;, which only fires when *size == 0. It does nothing when *size == 1, and integer truncation of 1 * 1.5 leaves *size stuck at 1.

Expected behavior

On LIBDEFLATE_INSUFFICIENT_SPACE, the loop should always increase *size on every iteration, regardless of the starting value, so it eventually reaches a buffer large enough to hold the decompressed data and the loop terminates.

Actual behavior

*size is attacker-controlled end-to-end. cram_read_block() reads b->uncomp_size directly from a varint in the CRAM block header. The only validation on this value is == 0 and >= 0, both of which uncomp_size = 1 passes. cram_uncompress_block()'s GZIP case then passes this value straight through as the initial *size for zlib_mem_inflate().

A GZIP block declaring uncomp_size = 1 while actually holding more data makes the retry loop spin forever, so a single crafted file hangs the parser (CWE-835, DoS).

Added test/test_cram_io.c: calls zlib_mem_inflate() directly against a full local build, reproducing uncomp_size=1 under a 5-second alarm(). Existing unit tests and test/range.cram header decoding still pass.

-        if (!*size)
-            *size = csize*2;
-        new_data = realloc(data, *size);
+        new_data = realloc(data, *size);
...
         if (ret == LIBDEFLATE_INSUFFICIENT_SPACE) {
-            (*size) *= 1.5;
+            *size = (*size + 1) + (*size >> 1);
             continue;
         }

Thanks for reviewing.

Assisted-by: Claude:claude-sonnet-5

[jkbonfield]

Good catch.

Elsewhere we basically do something like this:

    size_t alloc = b->alloc+800;
    alloc = MAX(alloc + (alloc>>2), len);

That's essentially size = size*1.25 + 800, done in integer arithmetic.

Your code could be simpler expressed as

    *size = (*size + 1) + (*size >> 1);

Although a bigger increment than 1 is probably useful for small blocks given the malloc heap isn't going to allocate anything smaller than 16 or 32 bytes at a time anyway.

[lrycro]

Good point — (*size + 1) + (*size >> 1) is cleaner and handles the *size==0 case too, so I dropped the old guard.

Verified against the real build: the growth sequence is now 1 → 2 → 4 → 7 → 11 → 17 → 26 → 40 → 61 → 92 (9 iterations vs the previous 22). test_cram_io passes, and all other unit tests plus test/range.cram header decoding still pass with no regressions.

I'll leave the +800-style constant out for now since this path isn't as hot as the b->alloc case, but happy to add one for consistency if you'd prefer.

Force-pushed the updated diff.

[jkbonfield]

Generally *size is passed in as the uncompressed size as listed in the block. You noted this above.

It's called either from cram_uncompress_block where *size is (theoretically) already known, and cram cram_index_load where *size is 0 and the data is the whole file. That could be one single gzip stream, or a series of concatenated ones (eg BGZF).

Therefore I think a starting size when zero would be the ISIZE field listed in the GZIP data stream (see https://datatracker.ietf.org/doc/html/rfc1952). It should be correct for cram blocks, and may be correct for indices. Either way we need to protect against malformed data or too-small data (e.g. many BGZF blocks).

I think I'll do this myself as a follow up PR, and include your commit within it (and still authored by you), so I can experiment locally for a bit.

Note we also discussed the tests you added and concluded they don't warrant such a large amount of extra code. I think we can all agree that there are pieces of code which are sufficiently complicated that they do need a regression test as there's every chance we'll break it again, and other pieces of code that are simple enough to not need a regression test (such as a typo). Everything else is inbetween, but on balance we felt this came under the "didn't think about it at the time" scenario where having now seen and fixed it it's highly unlikely we'd break it again without doing a total restructuring.

Either way, thanks for the bug report and implementation.

[lrycro]

That makes sense, thanks for the context on cram_index_load — I hadn't considered the concatenated-stream case there. Using ISIZE with a guard against malformed/too-small values sounds like the right fix, happy for you to take that as a follow-up.

Agreed on the test — I've stripped test_cram_io.c out so the commit is just the growth-formula fix, making it easier to fold into your follow-up PR.

Thanks for digging into this and taking it forward.

[jkbonfield]

Actually it turns out using BGZF with concatenated blocks doesn't work anyway! Neither the original zlib API nor libdeflate's API work on concatenated gzip blocks. I guess we've never hit this given how long CRAM has been around, so it's not something we need to worry about.