From d9c0cbbe62df20322894cd4b6f8059caf4b3ab35 Mon Sep 17 00:00:00 2001 From: Ross Lawley Date: Mon, 1 Jun 2026 14:18:20 +0100 Subject: [PATCH 1/2] Harden BsonBinaryReader.skipValue() size validation The skip path in doSkipValue() was more permissive than the canonical decode path, accepting malformed BSON sizes that full decoding rejects. This allowed RawBsonDocument metadata accessors (size(), containsKey(), get()) to accept attacker-controlled malformed data as valid fields. Add type-specific validation helpers to doSkipValue(): - readStringSize(): rejects size 0 (STRING, SYMBOL, JAVASCRIPT, DB_POINTER) - readContainerSize(min): rejects undersized containers (DOCUMENT, ARRAY, JAVASCRIPT_WITH_SCOPE) that would produce negative skips - readJavaScriptWithScopeSize(): validates nested code/scope sizes and total-size consistency - addTrailingBytes(): guards against int overflow for BINARY and DB_POINTER BINARY size 0 remains valid (empty binary is spec-compliant). JAVA-6220 --- bson/src/main/org/bson/BsonBinaryReader.java | 63 +++- .../unit/org/bson/BsonBinaryReaderTest.java | 298 +++++++++++++++++- .../unit/org/bson/RawBsonDocumentTest.java | 64 ++++ 3 files changed, 416 insertions(+), 9 deletions(-) diff --git a/bson/src/main/org/bson/BsonBinaryReader.java b/bson/src/main/org/bson/BsonBinaryReader.java index 5fff43beefe..1337b4b8ab4 100644 --- a/bson/src/main/org/bson/BsonBinaryReader.java +++ b/bson/src/main/org/bson/BsonBinaryReader.java @@ -304,10 +304,10 @@ protected void doSkipValue() { int skip; switch (getCurrentBsonType()) { case ARRAY: - skip = readSize() - 4; + skip = readContainerSize(5) - 4; break; case BINARY: - skip = readSize() + 1; + skip = addSkipBytes(readSize(), 1); break; case BOOLEAN: skip = 1; @@ -316,7 +316,7 @@ protected void doSkipValue() { skip = 8; break; case DOCUMENT: - skip = readSize() - 4; + skip = readContainerSize(5) - 4; break; case DOUBLE: skip = 8; @@ -331,10 +331,10 @@ protected void doSkipValue() { skip = 16; break; case JAVASCRIPT: - skip = readSize(); + skip = readStringSize(); break; case JAVASCRIPT_WITH_SCOPE: - skip = readSize() - 4; + skip = readJavaScriptWithScopeSize(); break; case MAX_KEY: skip = 0; @@ -354,10 +354,10 @@ protected void doSkipValue() { skip = 0; break; case STRING: - skip = readSize(); + skip = readStringSize(); break; case SYMBOL: - skip = readSize(); + skip = readStringSize(); break; case TIMESTAMP: skip = 8; @@ -366,7 +366,7 @@ protected void doSkipValue() { skip = 0; break; case DB_POINTER: - skip = readSize() + 12; // String followed by ObjectId + skip = addSkipBytes(readStringSize(), 12); // String followed by ObjectId break; default: throw new BSONException("Unexpected BSON type: " + getCurrentBsonType()); @@ -385,6 +385,53 @@ private int readSize() { return size; } + private int readStringSize() { + int size = readSize(); + if (size == 0) { + throw new BsonSerializationException( + "While reading a BSON string found a size of 0; expected at least 1 for the null terminator."); + } + return size; + } + + private int readContainerSize(final int minSize) { + int size = readSize(); + if (size < minSize) { + throw new BsonSerializationException(format( + "While reading a BSON container found a size of %d, which is less than the minimum of %d.", + size, minSize)); + } + return size; + } + + private int readJavaScriptWithScopeSize() { + // int32 totalSize + minimal string (int32 size + '\x00') + minimal document (int32 size + '\x00') = 14 + int size = readContainerSize(14); + int codeSize = readStringSize(); + // 13 = 4 codeSize prefix + 4 scopeSize prefix + 5 minimum scope document + if (codeSize > size - 13) { + throw new BsonSerializationException(format( + "While reading a BSON javascript_with_scope found code size %d is too large for total size %d.", + codeSize, size)); + } + bsonInput.skip(codeSize); + int scopeSize = readContainerSize(5); + long expectedSize = 4L + 4 + codeSize + scopeSize; + if (expectedSize != size) { + throw new BsonSerializationException(format("Expected size to be %d, not %d.", size, expectedSize)); + } + return scopeSize - 4; + } + + private int addSkipBytes(final int size, final int trailingBytes) { + if (size > Integer.MAX_VALUE - trailingBytes) { + throw new BsonSerializationException(format( + "While skipping a BSON value found a size of %d, which is too large to skip %d trailing bytes.", + size, trailingBytes)); + } + return size + trailingBytes; + } + protected Context getContext() { return (Context) super.getContext(); } diff --git a/bson/src/test/unit/org/bson/BsonBinaryReaderTest.java b/bson/src/test/unit/org/bson/BsonBinaryReaderTest.java index bffda74ecaa..dd822660931 100644 --- a/bson/src/test/unit/org/bson/BsonBinaryReaderTest.java +++ b/bson/src/test/unit/org/bson/BsonBinaryReaderTest.java @@ -25,6 +25,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; public class BsonBinaryReaderTest { @@ -70,7 +71,302 @@ public void testInvalidBsonTypeFollowedByInvalidCString() { } } - private BsonBinaryReader createReaderForBytes(final byte[] bytes) { + // String rejection tests - skipValue() rejects size=0 + + @Test + public void testSkipValueRejectsZeroLengthString() { + // {"a": } + // doc size=12: 4(size) + 1(type 0x02) + 2(name "a\0") + 4(string size=0) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.STRING), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // string size: 0 (invalid) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsZeroLengthSymbol() { + // {"a": } + // doc size=12: 4(size) + 1(type 0x0E) + 2(name "a\0") + 4(symbol size=0) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.SYMBOL), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // symbol size: 0 (invalid) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsZeroLengthJavaScript() { + // {"a": } + // doc size=12: 4(size) + 1(type 0x0D) + 2(name "a\0") + 4(js size=0) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // javascript size: 0 (invalid) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsZeroLengthDbPointer() { + // {"a": } + // doc size=24: 4(size) + 1(type 0x0C) + 2(name "a\0") + 4(string size=0) + 12(objectid) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 24, 0, 0, 0, // document size + typeByte(BsonType.DB_POINTER), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // string size: 0 (invalid) + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // objectid (12 bytes) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsOverflowingBinarySize() { + // {"a": } + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.BINARY), + 0x61, 0x00, // name: "a\0" + -1, -1, -1, 0x7F, // binary size: Integer.MAX_VALUE + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsOverflowingDbPointerSize() { + // {"a": } + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.DB_POINTER), + 0x61, 0x00, // name: "a\0" + -1, -1, -1, 0x7F, // string size: Integer.MAX_VALUE + 0x00 // document terminator + }); + } + + // Container rejection tests - skipValue() rejects undersized containers + + @Test + public void testSkipValueRejectsUndersizedDocument() { + // {"a": } + // doc size=12: 4(size) + 1(type 0x03) + 2(name "a\0") + 4(embedded doc size=3) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.DOCUMENT), + 0x61, 0x00, // name: "a\0" + 3, 0, 0, 0, // embedded document size: 3 (invalid, minimum is 5) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsUndersizedArray() { + // {"a": } + // doc size=12: 4(size) + 1(type 0x04) + 2(name "a\0") + 4(array size=0) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 12, 0, 0, 0, // document size + typeByte(BsonType.ARRAY), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // array size: 0 (invalid, minimum is 5) + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsUndersizedJavaScriptWithScope() { + // {"a": } + // doc size=24: 4(size) + 1(type 0x0F) + 2(name "a\0") + 4(js_ws size=13) + 12(filler) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 24, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 13, 0, 0, 0, // javascript_with_scope size: 13 (invalid, minimum is 14) + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // filler + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsJavaScriptWithScopeWithZeroLengthCode() { + // {"a": } + // doc size=22: 4(size) + 1(type 0x0F) + 2(name "a\0") + 14(js_ws value) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 22, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 14, 0, 0, 0, // javascript_with_scope total size + 0, 0, 0, 0, // code string size: 0 (invalid) + 0, 0, 0, 0, 0, 0, // filler + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsJavaScriptWithScopeWithOversizedCode() { + // {"a": } + // totalSize=15, codeSize=3: codeSize (3) > totalSize - 13 (2), so the intermediate guard fires. + // doc size=23: 4(size) + 1(type 0x0F) + 2(name "a\0") + 15(js_ws value) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 23, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 15, 0, 0, 0, // javascript_with_scope total size: 15 + 3, 0, 0, 0, // code string size: 3 (invalid - too large; max is totalSize-13=2) + 0, 0, 0, 0, 0, 0, 0, 0, // filler + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueRejectsJavaScriptWithScopeWithMismatchedTotalSize() { + // {"a": } + // totalSize=20, codeSize=1, scopeSize=5: parts sum to 4+4+1+5=14, which != totalSize 20. + // Exercises the final expectedSize != size consistency check. + // doc size=28: 4(size) + 1(type 0x0F) + 2(name "a\0") + 20(js_ws value) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 28, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 20, 0, 0, 0, // javascript_with_scope total size: 20 (inconsistent with parts) + 1, 0, 0, 0, // code string size: 1 + 0x00, // code string content: "\0" + 5, 0, 0, 0, // scope document size: 5 (valid minimum) + 0x00, // scope document terminator + 0, 0, 0, 0, 0, 0, // filler so outer doc reaches declared total + 0x00 // outer document terminator + }); + } + + @Test + public void testSkipValueRejectsJavaScriptWithScopeWithUndersizedScopeDocument() { + // {"a": } + // doc size=22: 4(size) + 1(type 0x0F) + 2(name "a\0") + 14(js_ws value) + 1(doc term) + assertSkipValueThrows(new byte[]{ + 22, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 14, 0, 0, 0, // javascript_with_scope total size + 1, 0, 0, 0, // code string size: 1 + 0x00, // code string content: "\0" + 4, 0, 0, 0, // scope document size: 4 (invalid, minimum is 5) + 0x00, // filler + 0x00 // document terminator + }); + } + + // Regression guards - valid minimal values still work + + @Test + public void testSkipValueAcceptsMinimalString() { + // {"a": ""} - string with size=1 (just the null terminator) + // doc size=13: 4(size) + 1(type 0x02) + 2(name "a\0") + 4(string size=1) + 1('\0') + 1(doc term) + assertSkipValueSucceeds(new byte[]{ + 13, 0, 0, 0, // document size + typeByte(BsonType.STRING), + 0x61, 0x00, // name: "a\0" + 1, 0, 0, 0, // string size: 1 (valid minimum - just null terminator) + 0x00, // string content: "\0" + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueAcceptsEmptyBinary() { + // {"a": } + // doc size=13: 4(size) + 1(type 0x05) + 2(name "a\0") + 4(binary size=0) + 1(subtype) + 1(doc term) + assertSkipValueSucceeds(new byte[]{ + 13, 0, 0, 0, // document size + typeByte(BsonType.BINARY), + 0x61, 0x00, // name: "a\0" + 0, 0, 0, 0, // binary size: 0 (valid - empty binary) + 0x00, // subtype: generic + 0x00 // document terminator + }); + } + + @Test + public void testSkipValueAcceptsMinimalDocument() { + // {"a": {}} - embedded document with size=5 (minimum valid: int32 size + '\x00') + // doc size=13: 4(docsize) + 1(type 0x03) + 2(name "a\0") + 5(embedded {}) + 1(doc term) + assertSkipValueSucceeds(new byte[]{ + 13, 0, 0, 0, // document size + typeByte(BsonType.DOCUMENT), + 0x61, 0x00, // name: "a\0" + 5, 0, 0, 0, // embedded document size: 5 (valid minimum) + 0x00, // embedded document terminator + 0x00 // outer document terminator + }); + } + + @Test + public void testSkipValueAcceptsMinimalArray() { + // {"a": []} - embedded array with size=5 (minimum valid: int32 size + '\x00') + // doc size=13: 4(docsize) + 1(type 0x04) + 2(name "a\0") + 5(embedded []) + 1(doc term) + assertSkipValueSucceeds(new byte[]{ + 13, 0, 0, 0, // document size + typeByte(BsonType.ARRAY), + 0x61, 0x00, // name: "a\0" + 5, 0, 0, 0, // embedded array size: 5 (valid minimum) + 0x00, // embedded array terminator + 0x00 // outer document terminator + }); + } + + @Test + public void testSkipValueAcceptsMinimalJavaScriptWithScope() { + // {"a": } + // doc size=22: 4(size) + 1(type 0x0F) + 2(name "a\0") + 14(js_ws value) + 1(doc term) + assertSkipValueSucceeds(new byte[]{ + 22, 0, 0, 0, // document size + typeByte(BsonType.JAVASCRIPT_WITH_SCOPE), + 0x61, 0x00, // name: "a\0" + 14, 0, 0, 0, // javascript_with_scope total size + 1, 0, 0, 0, // code string size: 1 + 0x00, // code string content: "\0" + 5, 0, 0, 0, // scope document size: 5 (valid minimum) + 0x00, // scope document terminator + 0x00 // outer document terminator + }); + } + + /** + * Wraps the given bytes in a BSON reader, advances past the document start, type, and field name, + * and asserts that {@code skipValue()} throws {@link BsonSerializationException}. + */ + private static void assertSkipValueThrows(final byte[] bytes) { + try (BsonBinaryReader reader = createReaderForBytes(bytes)) { + reader.readStartDocument(); + reader.readBsonType(); + reader.readName(); + assertThrows(BsonSerializationException.class, reader::skipValue); + } + } + + /** + * Wraps the given bytes in a BSON reader, advances past the document start, type, and field name, + * skips the value, and asserts the document terminates cleanly. + */ + private static void assertSkipValueSucceeds(final byte[] bytes) { + try (BsonBinaryReader reader = createReaderForBytes(bytes)) { + reader.readStartDocument(); + reader.readBsonType(); + reader.readName(); + reader.skipValue(); + reader.readEndDocument(); + } + } + + private static BsonBinaryReader createReaderForBytes(final byte[] bytes) { return new BsonBinaryReader(new ByteBufferBsonInput(new ByteBufNIO(ByteBuffer.wrap(bytes)))); } + + private static byte typeByte(final BsonType type) { + return (byte) type.getValue(); + } } diff --git a/bson/src/test/unit/org/bson/RawBsonDocumentTest.java b/bson/src/test/unit/org/bson/RawBsonDocumentTest.java index 866dc101799..1ad24bc02ff 100644 --- a/bson/src/test/unit/org/bson/RawBsonDocumentTest.java +++ b/bson/src/test/unit/org/bson/RawBsonDocumentTest.java @@ -21,6 +21,7 @@ import org.bson.io.BasicOutputBuffer; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.api.Named; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -29,6 +30,8 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; class RawBsonDocumentTest { @@ -72,6 +75,67 @@ void shouldExposeBackingArrayOffsetAndLength(final RawBsonDocument rawDocument, rawDocument.getByteOffset() + rawDocument.getByteLength())); } + @Test + void sizeShouldRejectMalformedZeroLengthStringValue() { + RawBsonDocument malformed = createDocumentWithZeroLengthStringValue(); + assertThrows(BsonSerializationException.class, malformed::size); + } + + @Test + void containsKeyShouldRejectMalformedZeroLengthStringWhenKeyNotFound() { + // "nonExistent" forces iteration past the malformed "x" field via skipValue(). + RawBsonDocument malformed = createDocumentWithZeroLengthStringValue(); + assertThrows(BsonSerializationException.class, () -> malformed.containsKey("nonExistent")); + } + + @Test + void containsKeyShouldReturnTrueForMalformedFieldKey() { + // "x" is the malformed field itself - containsKey matches the key before skipValue() is called. + RawBsonDocument malformed = createDocumentWithZeroLengthStringValue(); + assertTrue(malformed.containsKey("x")); + } + + @Test + void getShouldRejectMalformedZeroLengthStringWhenKeyNotFound() { + // "nonExistent" forces iteration past the malformed "x" field via skipValue(). + RawBsonDocument malformed = createDocumentWithZeroLengthStringValue(); + assertThrows(BsonSerializationException.class, () -> malformed.get("nonExistent")); + } + + @Test + void getFirstKeyShouldSucceedForMalformedDocument() { + // getFirstKey() reads the key name but never calls skipValue(), so it succeeds + // even when the first field's value is malformed. + RawBsonDocument malformed = createDocumentWithZeroLengthStringValue(); + assertEquals("x", malformed.getFirstKey()); + } + + /** + * Creates a {@link RawBsonDocument} containing a string field whose declared size is zero. + * Encodes {"x": "hello", "a": "world"} on a cloned byte array, then corrupts the string size + * of "x" to 0. "x" is deliberately the first field so that {@code getFirstKey()} and + * {@code containsKey("x")} can short-circuit before the malformed value is reached. + */ + private static RawBsonDocument createDocumentWithZeroLengthStringValue() { + BsonDocument doc = new BsonDocument() + .append("x", new BsonString("hello")) + .append("a", new BsonString("world")); + RawBsonDocument raw = new RawBsonDocument(doc, new BsonDocumentCodec()); + // Clone so the malformed document does not share state with `raw`. + byte[] bytes = raw.getBackingArray().clone(); + int offset = raw.getByteOffset(); + // BSON layout: [4 docSize] [0x02 type] [x \0 name] [int32 stringSize] [string bytes] ... + // The string size field for "x" starts at offset + 4(docSize) + 1(type) + 2(name "x\0") = offset + 7. + int stringSizeOffset = offset + 7; + assertEquals(6, (bytes[stringSizeOffset] & 0xFF) + | ((bytes[stringSizeOffset + 1] & 0xFF) << 8) + | ((bytes[stringSizeOffset + 2] & 0xFF) << 16) + | ((bytes[stringSizeOffset + 3] & 0xFF) << 24), + "Expected test fixture string length 6 (\"hello\\0\") at offset+7; encoding assumptions may have changed"); + Arrays.fill(bytes, stringSizeOffset, stringSizeOffset + 4, (byte) 0); + return new RawBsonDocument(bytes, offset, raw.getByteLength()); + } + private static Named createFromDocument() { return Named.of("from document", new RawBsonDocument(DOCUMENT, new BsonDocumentCodec())); } From ff3fff23f234ba3a1b147146d0aeb35001dcd457 Mon Sep 17 00:00:00 2001 From: Ross Lawley Date: Tue, 2 Jun 2026 12:27:40 +0100 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- bson/src/main/org/bson/BsonBinaryReader.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bson/src/main/org/bson/BsonBinaryReader.java b/bson/src/main/org/bson/BsonBinaryReader.java index 1337b4b8ab4..06d2d70656e 100644 --- a/bson/src/main/org/bson/BsonBinaryReader.java +++ b/bson/src/main/org/bson/BsonBinaryReader.java @@ -416,9 +416,9 @@ private int readJavaScriptWithScopeSize() { } bsonInput.skip(codeSize); int scopeSize = readContainerSize(5); - long expectedSize = 4L + 4 + codeSize + scopeSize; - if (expectedSize != size) { - throw new BsonSerializationException(format("Expected size to be %d, not %d.", size, expectedSize)); + long actualSize = 4L + 4 + codeSize + scopeSize; + if (actualSize != size) { + throw new BsonSerializationException(format("Expected size to be %d, not %d.", size, actualSize)); } return scopeSize - 4; }