diff --git a/CHANGELOG.md b/CHANGELOG.md index a42245c..33d933b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,14 @@ +## v0.1.13 +- Improved robustness to StructPointer integer wrapping problems. + +## v0.1.12 +- Fix bug in StructPointer.wordSize() found by Martin Dindoffer. + +## v0.1.11 +- Fix memory and cpu amplification vulnerabilities found by Martin Dindoffer. + +## v0.1.10 +- Make even more compatible with Java 8. + ## v0.1.9 - Add `8` to pom.xml, to increase compability with Java 8. diff --git a/benchmark/pom.xml b/benchmark/pom.xml index 3914d48..89446e2 100644 --- a/benchmark/pom.xml +++ b/benchmark/pom.xml @@ -5,7 +5,7 @@ benchmark jar capnproto-java benchmark - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT capnproto-java benchmark org.capnproto @@ -39,7 +39,7 @@ org.capnproto runtime - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT diff --git a/compiler/pom.xml b/compiler/pom.xml index 5bd6837..cf1ff77 100644 --- a/compiler/pom.xml +++ b/compiler/pom.xml @@ -5,7 +5,7 @@ compiler jar schema compiler plugin for java - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT capnpc-java org.capnproto @@ -44,7 +44,7 @@ org.capnproto runtime - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT diff --git a/compiler/src/test/java/org/capnproto/test/EncodingTest.java b/compiler/src/test/java/org/capnproto/test/EncodingTest.java index 60c35eb..e927f7f 100644 --- a/compiler/src/test/java/org/capnproto/test/EncodingTest.java +++ b/compiler/src/test/java/org/capnproto/test/EncodingTest.java @@ -615,6 +615,24 @@ public class EncodingTest { root.getAnyPointerField().getAs(StructList.newFactory(Test.TestAllTypes.factory)); } + // Test that we throw an exception on out-of-bounds list pointers. + // Before v0.1.11, we were vulnerable to a cpu amplification attack: + // reading an out-of-bounds pointer to list a huge number of elements of size BIT, + // when read as a struct list, would return without error. + @org.junit.Test(expected=DecodeException.class) + public void testListPointerOutOfBounds() throws DecodeException { + byte[] bytes = new byte[] + {0,0,0,0, 0,0,1,0, // struct, one pointer + 1, 0x2f, 0, 0, 1, 0, -127, -128}; // list, points out of bounds. + ByteBuffer segment = ByteBuffer.wrap(bytes); + segment.order(ByteOrder.LITTLE_ENDIAN); + MessageReader message = new MessageReader(new ByteBuffer[]{segment}, + ReaderOptions.DEFAULT_READER_OPTIONS); + + Test.TestAnyPointer.Reader root = message.getRoot(Test.TestAnyPointer.factory); + root.getAnyPointerField().getAs(StructList.newFactory(Test.TestAllTypes.factory)); + } + @org.junit.Test public void testLongUint8List() { MessageBuilder message = new MessageBuilder(); @@ -864,5 +882,41 @@ public class EncodingTest { root2.setAs(AnyPointer.factory, message1.getRoot(AnyPointer.factory).asReader()); TestUtil.checkTestMessage(root2.getAs(Test.TestAllTypes.factory)); + } + + @org.junit.Test + public void testZeroPointerUnderflow() throws DecodeException { + byte[] bytes = new byte[8 + 8 * 65535]; + bytes[4] = -1; + bytes[5] = -1; // struct pointer with 65535 words of data section. + for (int ii = 0; ii < 8 * 65535; ++ii) { + bytes[8 + ii] = 101; // populate the data section with sentinel data + } + ByteBuffer segment = ByteBuffer.wrap(bytes); + segment.order(ByteOrder.LITTLE_ENDIAN); + MessageReader message1 = new MessageReader(new ByteBuffer[]{segment}, + ReaderOptions.DEFAULT_READER_OPTIONS); + Test.TestAnyPointer.Reader message1RootReader = message1.getRoot(Test.TestAnyPointer.factory); + + MessageBuilder message2Builder = + new MessageBuilder(3 * 65535); // ample space to avoid far pointers + Test.TestAnyPointer.Builder message2RootBuilder = + message2Builder.getRoot(Test.TestAnyPointer.factory); + + // Copy the struct that has the sentinel data. + message2RootBuilder.getAnyPointerField().setAs(Test.TestAnyPointer.factory, message1RootReader); + + // Now clear the struct pointer. + message2RootBuilder.getAnyPointerField().clear(); + + java.nio.ByteBuffer[] outputSegments = message2Builder.getSegmentsForOutput(); + Assert.assertEquals(1, outputSegments.length); + Assert.assertEquals(0L, outputSegments[0].getLong(8)); // null because cleared + + Assert.assertEquals(16 + 8 * 65535, outputSegments[0].limit()); + for (int ii = 0; ii < 65535; ++ii) { + // All of the data should have been cleared. + Assert.assertEquals(0L, outputSegments[0].getLong((2 + ii) * 8)); + } } } diff --git a/examples/pom.xml b/examples/pom.xml index 793edc7..f733219 100644 --- a/examples/pom.xml +++ b/examples/pom.xml @@ -5,7 +5,7 @@ examples jar capnproto-java examples - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT capnproto-java examples org.capnproto @@ -33,18 +33,19 @@ UTF-8 14 14 + 14 org.capnproto runtime - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT org.capnproto runtime-rpc - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT diff --git a/pom.xml b/pom.xml index f33578d..e51ca3e 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ org.capnproto capnproto-java pom - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT Cap'n Proto for Java https://capnproto.org/ diff --git a/runtime/pom.xml b/runtime/pom.xml index b2a38d5..18d2c63 100644 --- a/runtime/pom.xml +++ b/runtime/pom.xml @@ -5,7 +5,7 @@ runtime jar runtime - 0.1.10-SNAPSHOT + 0.1.14-SNAPSHOT Cap'n Proto runtime library org.capnproto diff --git a/runtime/src/main/java/org/capnproto/SegmentReader.java b/runtime/src/main/java/org/capnproto/SegmentReader.java index 82f8330..b407d21 100644 --- a/runtime/src/main/java/org/capnproto/SegmentReader.java +++ b/runtime/src/main/java/org/capnproto/SegmentReader.java @@ -38,4 +38,12 @@ public class SegmentReader { public final long get(int index) { return buffer.getLong(index * Constants.BYTES_PER_WORD); } + + // Verify that the `size`-long (in words) range starting at word index + // `start` is within bounds. + public final boolean in_bounds(int start, int size) { + if (start < 0 || size < 0) return false; + long sizeInWords = size * Constants.BYTES_PER_WORD; + return (long) start + sizeInWords < (long) this.buffer.capacity(); + } } diff --git a/runtime/src/main/java/org/capnproto/Serialize.java b/runtime/src/main/java/org/capnproto/Serialize.java index 9a8406c..6dcccad 100644 --- a/runtime/src/main/java/org/capnproto/Serialize.java +++ b/runtime/src/main/java/org/capnproto/Serialize.java @@ -84,6 +84,10 @@ public final class Serialize { segment0Size = firstWord.getInt(4); } + if (segment0Size < 0) { + throw new DecodeException("segment 0 has more than 2^31 words, which is unsupported"); + } + long totalWords = segment0Size; // in words @@ -94,6 +98,11 @@ public final class Serialize { fillBuffer(moreSizesRaw, bc); for (int ii = 0; ii < segmentCount - 1; ++ii) { int size = moreSizesRaw.getInt(ii * 4); + if (size < 0) { + throw new DecodeException("segment " + (ii + 1) + + " has more than 2^31 words, which is unsupported"); + } + moreSizes.add(size); totalWords += size; } diff --git a/runtime/src/main/java/org/capnproto/StructPointer.java b/runtime/src/main/java/org/capnproto/StructPointer.java index 582e6eb..855ea74 100644 --- a/runtime/src/main/java/org/capnproto/StructPointer.java +++ b/runtime/src/main/java/org/capnproto/StructPointer.java @@ -24,17 +24,17 @@ package org.capnproto; import java.nio.ByteBuffer; final class StructPointer{ - public static short dataSize(long ref) { + public static int dataSize(long ref) { // in words. - return (short)(WirePointer.upper32Bits(ref) & 0xffff); + return WirePointer.upper32Bits(ref) & 0xffff; } - public static short ptrCount(long ref) { - return (short)(WirePointer.upper32Bits(ref) >>> 16); + public static int ptrCount(long ref) { + return WirePointer.upper32Bits(ref) >>> 16; } public static int wordSize(long ref) { - return (int)dataSize(ref) + (int)ptrCount(ref); + return dataSize(ref) + ptrCount(ref); } public static void setFromStructSize(ByteBuffer buffer, int offset, StructSize size) { diff --git a/runtime/src/main/java/org/capnproto/WireHelpers.java b/runtime/src/main/java/org/capnproto/WireHelpers.java index 6ca38a2..275f9b4 100644 --- a/runtime/src/main/java/org/capnproto/WireHelpers.java +++ b/runtime/src/main/java/org/capnproto/WireHelpers.java @@ -38,6 +38,14 @@ final class WireHelpers { return (int)((bits + 63) / ((long) Constants.BITS_PER_WORD)); } + // ByteBuffer already does bounds checking, but we still want + // to check bounds in some places to avoid cpu amplification attacks. + static boolean bounds_check(SegmentReader segment, + int start, + int size) { + return segment == null || segment.in_bounds(start, size); + } + static class AllocateResult { public final int ptr; public final int refOffset; @@ -434,8 +442,8 @@ final class WireHelpers { } FollowBuilderFarsResult resolved = followBuilderFars(ref, target, segment); - short oldDataSize = StructPointer.dataSize(resolved.ref); - short oldPointerCount = StructPointer.ptrCount(resolved.ref); + int oldDataSize = StructPointer.dataSize(resolved.ref); + int oldPointerCount = StructPointer.ptrCount(resolved.ref); int oldPointerSection = resolved.ptr + oldDataSize; if (oldDataSize < size.data || oldPointerCount < size.pointers) { @@ -482,7 +490,7 @@ final class WireHelpers { } else { return factory.constructBuilder(resolved.segment, capTable, resolved.ptr * Constants.BYTES_PER_WORD, oldPointerSection, oldDataSize * Constants.BITS_PER_WORD, - oldPointerCount); + (short)oldPointerCount); } } @@ -623,7 +631,7 @@ final class WireHelpers { throw new DecodeException("INLINE_COMPOSITE list with non-STRUCT elements not supported."); } int oldDataSize = StructPointer.dataSize(oldTag); - short oldPointerCount = StructPointer.ptrCount(oldTag); + int oldPointerCount = StructPointer.ptrCount(oldTag); int oldStep = (oldDataSize + oldPointerCount * Constants.POINTER_SIZE_IN_WORDS); int elementCount = WirePointer.inlineCompositeListElementCount(oldTag); @@ -632,7 +640,8 @@ final class WireHelpers { return factory.constructBuilder(resolved.segment, capTable, oldPtr * Constants.BYTES_PER_WORD, elementCount, oldStep * Constants.BITS_PER_WORD, - oldDataSize * Constants.BITS_PER_WORD, oldPointerCount); + oldDataSize * Constants.BITS_PER_WORD, + (short)oldPointerCount); } //# The structs in this list are smaller than expected, probably written using an older @@ -951,7 +960,7 @@ final class WireHelpers { resolved.ptr * Constants.BYTES_PER_WORD, (resolved.ptr + dataSizeWords), dataSizeWords * Constants.BITS_PER_WORD, - StructPointer.ptrCount(resolved.ref), + (short)StructPointer.ptrCount(resolved.ref), nestingLimit - 1); } @@ -993,7 +1002,7 @@ final class WireHelpers { resolved.ptr * Constants.BYTES_PER_WORD, (resolved.ptr + dataSizeWords), dataSizeWords * Constants.BITS_PER_WORD, - StructPointer.ptrCount(resolved.ref), + (short)StructPointer.ptrCount(resolved.ref), nestingLimit - 1); } @@ -1004,7 +1013,7 @@ final class WireHelpers { AllocateResult allocation = allocate(refOffset, segment, capTable, totalSize, WirePointer.STRUCT); StructPointer.set(allocation.segment.buffer, allocation.refOffset, - dataSize, value.pointerCount); + (short)dataSize, value.pointerCount); if (value.dataSize == 1) { throw new RuntimeException("single bit case not handled"); @@ -1131,7 +1140,7 @@ final class WireHelpers { resolved.ptr * Constants.BYTES_PER_WORD, resolved.ptr + StructPointer.dataSize(resolved.ref), StructPointer.dataSize(resolved.ref) * Constants.BITS_PER_WORD, - StructPointer.ptrCount(resolved.ref), + (short)StructPointer.ptrCount(resolved.ref), nestingLimit - 1)); case WirePointer.LIST : byte elementSize = ListPointer.elementSize(resolved.ref); @@ -1167,7 +1176,7 @@ final class WireHelpers { elementCount, wordsPerElement * Constants.BITS_PER_WORD, StructPointer.dataSize(tag) * Constants.BITS_PER_WORD, - StructPointer.ptrCount(tag), + (short)StructPointer.ptrCount(tag), nestingLimit - 1)); } else { int dataSize = ElementSize.dataBitsPerElement(elementSize); @@ -1238,6 +1247,10 @@ final class WireHelpers { FollowFarsResult resolved = followFars(ref, refTarget, segment); + if (WirePointer.kind(resolved.ref) != WirePointer.LIST) { + throw new DecodeException("Message contains non-list pointer where list was expected."); + } + byte elementSize = ListPointer.elementSize(resolved.ref); switch (elementSize) { case ElementSize.INLINE_COMPOSITE : { @@ -1247,6 +1260,9 @@ final class WireHelpers { int ptr = resolved.ptr + 1; resolved.segment.arena.checkReadLimit(wordCount + 1); + if (!bounds_check(resolved.segment, resolved.ptr, wordCount + 1)) { + throw new DecodeException("Message contains out-of-bounds list pointer"); + } int size = WirePointer.inlineCompositeListElementCount(tag); @@ -1269,7 +1285,7 @@ final class WireHelpers { size, wordsPerElement * Constants.BITS_PER_WORD, StructPointer.dataSize(tag) * Constants.BITS_PER_WORD, - StructPointer.ptrCount(tag), + (short)StructPointer.ptrCount(tag), nestingLimit - 1); } default : { @@ -1282,8 +1298,12 @@ final class WireHelpers { int elementCount = ListPointer.elementCount(resolved.ref); int step = dataSize + pointerCount * Constants.BITS_PER_POINTER; - resolved.segment.arena.checkReadLimit( - roundBitsUpToWords(elementCount * step)); + int wordCount = roundBitsUpToWords((long)elementCount * step); + resolved.segment.arena.checkReadLimit(wordCount); + + if (!bounds_check(resolved.segment, resolved.ptr, wordCount)) { + throw new DecodeException("Message contains out-of-bounds list pointer"); + } if (elementSize == ElementSize.VOID) { // Watch out for lists of void, which can claim to be arbitrarily large without diff --git a/runtime/src/test/java/org/capnproto/SerializeTest.java b/runtime/src/test/java/org/capnproto/SerializeTest.java index 9ee4db9..5ad9abe 100644 --- a/runtime/src/test/java/org/capnproto/SerializeTest.java +++ b/runtime/src/test/java/org/capnproto/SerializeTest.java @@ -176,4 +176,22 @@ public class SerializeTest { 3, 0, 0, 0, 0, 0, 0, 0 }); } + + @Test(expected=DecodeException.class) + public void testSegment0SizeOverflow() throws java.io.IOException { + byte[] input = {0, 0, 0, 0, -1, -1, -1, -113}; + java.nio.channels.ReadableByteChannel channel = + java.nio.channels.Channels.newChannel(new java.io.ByteArrayInputStream(input)); + MessageReader message = Serialize.read(channel); + } + + @Test(expected=DecodeException.class) + public void testSegment1SizeOverflow() throws java.io.IOException { + byte[] input = { + 1, 0, 0, 0, 1, 0, 0, 0, + -1, -1, -1, -113, 0, 0, 0, 0}; + java.nio.channels.ReadableByteChannel channel = + java.nio.channels.Channels.newChannel(new java.io.ByteArrayInputStream(input)); + MessageReader message = Serialize.read(channel); + } } diff --git a/runtime/src/test/java/org/capnproto/StructPointerTest.java b/runtime/src/test/java/org/capnproto/StructPointerTest.java new file mode 100644 index 0000000..f39ae0e --- /dev/null +++ b/runtime/src/test/java/org/capnproto/StructPointerTest.java @@ -0,0 +1,48 @@ +package org.capnproto; + +import org.junit.Assert; +import org.junit.Test; + +public class StructPointerTest { + @Test + public void testDataSize() { + Assert.assertEquals( + 2, + StructPointer.dataSize(0x0001000200000000L)); + } + + @Test + public void testDataSizeUnderflow() { + Assert.assertEquals( + 0xffff, + StructPointer.dataSize(0x0001ffff00000000L)); + } + + @Test + public void testPtrCount() { + Assert.assertEquals( + 1, + StructPointer.ptrCount(0x0001000200000000L)); + } + + @Test + public void testPtrCountUnderflow() { + Assert.assertEquals( + 0xffff, + StructPointer.ptrCount(0xffff000200000000L)); + } + + @Test + public void testWordSize() { + Assert.assertEquals( + 3, + StructPointer.wordSize(0x0001000200000000L)); + } + + @Test + public void testWordSizeUnderflow() { + Assert.assertEquals( + 0x1fffe, + StructPointer.wordSize(0xffffffff00000000L)); + } +} diff --git a/security-advisories/2021-09-30-0-cpu-amplification.md b/security-advisories/2021-09-30-0-cpu-amplification.md new file mode 100644 index 0000000..d0be8a6 --- /dev/null +++ b/security-advisories/2021-09-30-0-cpu-amplification.md @@ -0,0 +1,57 @@ +Problem +======= + +CPU usage amplification attack + +Discovered by +============= + +Martin Dindoffer <contact@dindoffer.eu> + +Announced +========= + +2021-09-30 + +Impact +====== + +It is possible for an attacker to craft a small malicious message that will contain large lists with no data, leading to +artificial load on the system and possibly a Denial of Service attack. + +CVSS score +========== + +5.9 (Medium) CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H + +Fixed in +======== + +Release 0.1.11 + +Details +======= + +Cap'n'Proto List pointer encodes the size of the list in the header as a 29 bit value. This value is read as a counter +for the list Iterator. Setting a large value for the elementCount may produce empty loops in the code that don't operate +on any data but still consume CPU time. + +List Amplification is a well known posibility in the Cap'n'Proto protocol, as can be seen in the Encoding +spec (https://capnproto.org/encoding.html): +> A list of Void values or zero-size structs can have a very large element count while taking constant space on the wire. +> If the receiving application expects a list of structs, it will see these zero-sized elements as valid structs set to their default values. +> If it iterates through the list processing each element, it could spend a large amount of CPU time or other resources despite the message being small. +> To defend against this, the “traversal limit” should count a list of zero-sized elements as if each element were one word instead. +> This rule was introduced in the C++ implementation in commit 1048706. + +A form of this traversal limit countermeasure is present in capnproto-java. However, the message may contain a huge list +with 1-bit elements that are read as a struct list. This combined with the fact that the data may simply be stripped +from the message makes it is possible to create problematically huge Iterators with default traversal limits. + +Remediation +=========== + +The iteration should take into account the actual message length. As a rule of thumb the parser should not enter loops +with an attacker-controlled iteration count that is not bounded by the input length. + +Therefore additional bounds checking was introduced to detect out-of-bounds list pointers. \ No newline at end of file diff --git a/security-advisories/2021-09-30-1-excessive-memory-allocation.md b/security-advisories/2021-09-30-1-excessive-memory-allocation.md new file mode 100644 index 0000000..2f54aca --- /dev/null +++ b/security-advisories/2021-09-30-1-excessive-memory-allocation.md @@ -0,0 +1,54 @@ +Problem +======= + +Insufficient validation of message metadata with negative segment sizes may lead to excessive memory allocation and +subsequent DoS. + +Discovered by +============= + +Martin Dindoffer <contact@dindoffer.eu> + +Announced +========= + +2021-09-30 + +Impact +====== + +It is possible for an attacker to craft a malicious message that may thanks to memory amplification lead to a Denial of +Service attack against the consumer of CapnProto messages. In practical terms a message of only 8 bytes may cause +allocation of 2GB of memory. + +CVSS score +========== + +7.5 (High) CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H + +Fixed in +======== + +Release 0.1.11 + +Details +======= + +A read of 4 bytes from the message is performed, the result of which is read as the size of a segment in a variable. +This size is stored in memory as a signed integer. If the segment count is 1, the size of the first segment is also +considered to be the total size of the message stored in a totalWords variable. Since both the segment0Size and the +totalWords are signed integers, they may hold a negative number which will pass the traversal limit check. + +Subsequently capnproto-java tries to allocate enough memory for the segment. Because the encoded size is in words, the +value is multiplied by the number 8. The resulting number is used as the size of a ByteBuffer allocated for the first +segment. A sufficiently large negative number when multiplied by 8 will net a large positive integer, thus causing large +memory allocation in the form of a ByteBuffer. + +A segment size of `-1 879 048 193` multiplied by 8 results in `2 147 483 640` bytes of allocated memory, which is close +to `Integer.MAX_VALUE` and around the size of the maximum length of a Java array. Therefore the maximum size a message +may allocate this way is roughly 2GB. + +Remediation +=========== + +Stricter validation refusing negative segment sizes was implemented. \ No newline at end of file diff --git a/security-advisories/2021-10-04-0-cpu-amplification.md b/security-advisories/2021-10-04-0-cpu-amplification.md new file mode 100644 index 0000000..cd23609 --- /dev/null +++ b/security-advisories/2021-10-04-0-cpu-amplification.md @@ -0,0 +1,47 @@ +Problem +======= + +Incorrect parsing of large list element sizes may lead to CPU usage amplification attack + +Discovered by +============= + +Martin Dindoffer <contact@dindoffer.eu> + +Announced +========= + +2021-10-04 + +Impact +====== + +It is possible for an attacker to craft a small malicious message that will contain large lists with no data, leading to +artificial load on the system and possibly a Denial of Service attack. + +CVSS score +========== + +5.9 (Medium) CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H + +Fixed in +======== + +Release 0.1.12 + +Details +======= + +A tag pointer encodes the size of the elements in words and the total number of elements. Validation of these values is +performed in the code, but only as an overflow check of their product (against the total wordcount encoded in the list +pointer). The element size is read as a `short` value with improper casting to signed integers. Therefore, it is +possible to craft a message that will encode a large amount of elements (up to `2^30`) and offset this bloated size by +specifying a negative size of the elements (in words). There is no need to encode the actual payload for the large list, +thus capnproto-java will generate Iterators with large iteration counts over empty payload with messages of well under +100 bytes, that may put CPU under loads heavy enough to easily perform DoS attacks. + +Remediation +=========== + +The `short` values of the list element sizes are now parsed as unsigned values, making sure that the list overflow check +will catch the huge list size. \ No newline at end of file