search

airlift / aircompressor

A port of Snappy, LZO, LZ4, and Zstandard to Java
Apache License 2.0
562 stars 111 forks source link

Fix 24 bit out-of-bounds read #153

Closed Sineaggi closed 2 years ago

Sineaggi commented 2 years ago

Fixes an issue where the existing zstd decompressor can make an out-of-bounds unsafe read.

Currently the line

verify(input + SIZE_OF_BLOCK_HEADER <= inputLimit, input, "Not enough input bytes");

won't catch out-of-bounds reads because it only checks for SIZE_OF_BLOCK_HEADER, even tho the code reads a full int.

We fix this by reading only the bytes we care about. I've mirrored the existing put24BitLittleEndian function with a new get24BitLittleEndian and added a few tests.

Sineaggi commented 2 years ago

Can you add a test to ensure the decompressor will throw a MalformedInputException if there are not enough bytes to read from the input buffer under the conditions addressed by this fix?

I'm not sure we can add a test to this since the original code was unsafe, but worked. It read out of bounds (1 byte) then used a mask to get rid of any wrong bytes read. The call to verify() already handles the 3 byte read, however the code was reading 4 bytes.

martint commented 2 years ago

If you provide an input buffer that contains truncated input, it should be possible to assert that the code throws a MalformedInputException instead of silently reading that extra byte.

Sineaggi commented 2 years ago

Let me know if the new test shows off what you had in mind

martint commented 2 years ago

Yes, that's great. Can you squash that last commit into the one that fixes the bug?

Sineaggi commented 2 years ago

Yes, that's great. Can you squash that last commit into the one that fixes the bug?

Squashed and rewrote history a bit to get the correct changes into the correct commits.