Improve bounds checks

[Marcono1234]

As discussed @cowtowncoder, these are the changes for final review. If it is easier, you can review the commits individually.

If possible on merge please preserve the individual commits and don't squash them.

(CC @yawkat)

[cowtowncoder]

Hmmmh. Not a big fan of defensive coding; plus, doing this will cancel much of performance benefits of buffer reuse.

Or, maybe worse, add overhead for case where there is no reuse.

[cowtowncoder]

Actually, let's just remove these. If we really wanted, could add opt-in configuration for clearing but for now I don't think we should clear the buffers due to overhead.

[yawkat]

@cowtowncoder it can lead to serious issues, see GHSA-cmp6-m4wj-q63q

[cowtowncoder]

Yes, in case of bug(s) that allow copy from outside legal area.

I will remove clearing from this PR: if there's reproduction of actual vulnerability can address that separately.

I would also be +1 for adding setting that simply disables recycling for most security conscious; that would avoid the issue even more reliably.

[cowtowncoder]

Removed buffer clearing from this PR; as I said can be addressed in a follow-up as necessary.

[Marcono1234]

doing this will cancel much of performance benefits of buffer reuse

Fair point. But unlike GHSA-cmp6-m4wj-q63q, if there was a similar issue here in ning/compress, then it would always be an issue due to the automatic buffer reuse.

Though it seems LZF is not prone to the same offset == 0 bug because for LZF the offset is always + 1, see https://github.com/ning/compress/wiki/LZFFormat. So the risk here would be rather some other implementation bug or overflow.
Though at least during fuzzing I did not find any issues.

[cowtowncoder]

Just realized I messed up beautifully orchestrated individual commits. Sorry. Will still merge as separate.

[cowtowncoder]

Ok this is merged with changes I discussed and I'd be ready for 1.2.0 release.

But before doing that wanted to check anyone thinks further work is needed for before doing that (f.ex to address concerns about buffer recycling).

@Marcono1234 @yawkat WDYT?

[Marcono1234]

But before doing that wanted to check anyone thinks further work is needed for before doing that (f.ex to address concerns about buffer recycling).

I am not sure, but I think not. It seems most or all cases where BufferRecycler is used already allow the user to provide their own BufferRecycler so they could do new BufferRecycler(). Maybe it could still be improved somehow in the future, but for now it might suffice?

Also a small note regarding 9265ebb, specifically this check:

compress/src/main/java/com/ning/compress/lzf/ChunkDecoder.java

Lines 110 to 113 in c9acc13

	// Fail if more input than expected was consumed, respectively if `inLength` does not include full block
	if (inPtr > endMinusOne + 1) {
	throw new LZFException("Corrupt input data, block #" + blockNr + " is incomplete");
	}

That happens after too much data was already consumed, but I think it is safe nonetheless because there are already bounds checks for the Unsafe access (so Unsafe out-of-bounds access is not a concern), and it throws an exception here so even if targetBuffer contains data which should not have been decompressed due to being out-of-bounds, it is not inspected by the user anyway.

The only (?) risk would be some side channel attack, that a malicious user can deduce information from different exceptions which occur before this check here. Or that despite the exception being thrown here, user-code still somehow uses the content from targetBuffer.
But I think the chances for both of this are rather low.
And preventing that would probably require refactoring the chunk decoder implementations and adding bounds checks (the code currently contains little to no bounds checks, and only fails implicitly due to out-of-bounds array access).

[cowtowncoder]

Ok. For now I consider this potentially releasable; but will wait a day or two just in case.
(not that it's a big deal; releasing is easy and versions free).