Skip to content

Commit

Permalink
Merge pull request from GHSA-qcwq-55hx-v3vh
Browse files Browse the repository at this point in the history
* asserted chunksize should be in the bounds of 0-java.outofmmeoryexception

* asserted chunksize should be in the bounds of 0-java.outofmmeoryexception

* https://github.com/xerial/snappy-java-ghsa-qcwq-55hx-v3vh/pull/2

* advisory-fix-3

* added and changed method name for happy and sad cases in SnappyTest.java

* removed expected error for happy case in unit testing

* added another unit test case in SnappyTest.java and fixed comments in SnappyInputStream.java

* switched SnappyError to INVALID_CHUNK_SIZE

* Updated unit tests

* Resolved conflicts with another PR merge
  • Loading branch information
aidancch authored Jun 14, 2023
1 parent 820e2e0 commit 3bf6785
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 2 deletions.
13 changes: 12 additions & 1 deletion src/main/java/org/xerial/snappy/SnappyInputStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -417,9 +417,20 @@ protected boolean hasNextChunk()
}
}

// chunkSize is negative
if (chunkSize < 0) {
throw new SnappyError(SnappyErrorCode.INVALID_CHUNK_SIZE, "chunkSize is too big or negative : " + chunkSize);
}

// extend the compressed data buffer size
if (compressed == null || chunkSize > compressed.length) {
compressed = new byte[chunkSize];
// chunkSize exceeds limit
try {
compressed = new byte[chunkSize];
}
catch (java.lang.OutOfMemoryError e) {
throw new SnappyError(SnappyErrorCode.INVALID_CHUNK_SIZE, e.getMessage());
}
}
readBytes = 0;
while (readBytes < chunkSize) {
Expand Down
51 changes: 50 additions & 1 deletion src/test/java/org/xerial/snappy/SnappyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import static org.junit.Assert.*;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.nio.ByteBuffer;

Expand Down Expand Up @@ -330,14 +331,61 @@ public void isValidCompressedData()
}
}

/*
Tests happy cases for SnappyInputStream.read method
- {0}
*/
@Test
public void isValidChunkLengthForSnappyInputStreamIn()
throws Exception {
byte[] data = {0};
SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data));
byte[] out = new byte[50];
in.read(out);
}

/*
Tests sad cases for SnappyInputStream.read method
- Expects a java.lang.NegativeArraySizeException catched into a SnappyError
- {-126, 'S', 'N', 'A', 'P', 'P', 'Y', 0, 0, 0, 0, 0, 0, 0, 0, 0,(byte) 0x7f, (byte) 0xff, (byte) 0xff, (byte) 0xff}
*/
@Test(expected = SnappyError.class)
public void isInvalidChunkLengthForSnappyInputStreamInNegative()
throws Exception {
byte[] data = {-126, 'S', 'N', 'A', 'P', 'P', 'Y', 0, 0, 0, 0, 0, 0, 0, 0, 0,(byte) 0x7f, (byte) 0xff, (byte) 0xff, (byte) 0xff};
SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data));
byte[] out = new byte[50];
in.read(out);
}

/*
Tests sad cases for SnappyInputStream.read method
- Expects a java.lang.OutOfMemoryError
- {-126, 'S', 'N', 'A', 'P', 'P', 'Y', 0, 0, 0, 0, 0, 0, 0, 0, 0,(byte) 0x7f, (byte) 0xff, (byte) 0xff, (byte) 0xff}
*/
@Test(expected = SnappyError.class)
public void isInvalidChunkLengthForSnappyInputStreamOutOfMemory()
throws Exception {
byte[] data = {-126, 'S', 'N', 'A', 'P', 'P', 'Y', 0, 0, 0, 0, 0, 0, 0, 0, 0, (byte) 0x7f, (byte) 0xff, (byte) 0xff, (byte) 0xff};
SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data));
byte[] out = new byte[50];
try {
in.read(out);
} catch (Exception ignored) {
// Exception here will be catched
// But OutOfMemoryError will not be caught, and will still be thrown
}
}

/*
Tests happy cases for BitShuffle.shuffle method
- double: 0, 10
- float: 0, 10
- int: 0, 10
- long: 0, 10
- short: 0, 10
*/
*/
@Test
public void isValidArrayInputLengthForBitShuffleShuffle()
throws Exception
Expand Down Expand Up @@ -386,5 +434,6 @@ public void isTooLargeLongArrayInputLengthForBitShuffleShuffle() throws Exceptio
@Test(expected = SnappyError.class)
public void isTooLargeShortArrayInputLengthForBitShuffleShuffle() throws Exception {
BitShuffle.shuffle(new short[Integer.MAX_VALUE / 2 + 1]);

}
}

1 comment on commit 3bf6785

@josephearl
Copy link

@josephearl josephearl commented on 3bf6785 Jul 21, 2023

Choose a reason for hiding this comment

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

I don't see how this addresses the issue of DoS.

If the chunkSize is too large an OutOfMemoryError still occurs, it is just wrapped. This does mean that technically the error is now caught by catch block in the program and thus the test case in the advisory passes (most of the time - it may not, since allocating a new SnappyError after catching OutOfMemoryError could fail), but catching Errors is very rarely a good idea, and libraries downgrading them to Exceptions and hiding them from applications is definitely not a good idea. IMO this change should be reverted - either the chunkSize should be validated to be within some bounds (perhaps specified by the application) and an Exception thrown without an OutOfMemoryError occurring, or if a (unbounded) chunkSize is supported then the OutOfMemoryError should simply be allowed to occur and applications may catch that if they wish.

If the chunkSize is negative the program still crashes, just with a different error - the availability is in no senses improved. However this is a good change of validating the inputs and should be kept regardless.

All that said I think the advisory is probably the one wrong here rather than the code, since throwing an exception for a negative chunkSize is the right thing to do if recovering is not possible, as is a library not limiting the chunkSize. So this definitely shouldn't be a high.

Please sign in to comment.