From d33087d28bf3c688d29758954de1b96666d3fd86 Mon Sep 17 00:00:00 2001
From: Tyler Gregg <greggt@amazon.com>
Date: Thu, 14 Mar 2024 15:26:04 -0700
Subject: [PATCH] Adds a check for overflow when reading VarUInts.

---
 .../com/amazon/ion/impl/IonCursorBinary.java  |  5 +-
 .../IonReaderContinuableCoreBinaryTest.java   | 64 +++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
index 3b5f94ca73..7e06b6db4e 100644
--- a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
+++ b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java
@@ -746,6 +746,9 @@ private long uncheckedReadVarUInt_1_0(byte currentByte) {
             currentByte = buffer[(int) (peekIndex++)];
             result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
         } while (currentByte >= 0);
+        if (result < 0) {
+            throw new IonException("Found a VarUInt that was too large to fit in a `long`");
+        }
         return result;
     }
 
@@ -796,7 +799,7 @@ private boolean uncheckedReadAnnotationWrapperHeader_1_0(IonTypeID valueTid) {
         }
         endIndex += peekIndex;
         setMarker(endIndex, valueMarker);
-        if (endIndex > limit) {
+        if (endIndex > limit || endIndex < 0) {
             throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
         }
         byte b = buffer[(int) peekIndex++];
diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java
index 92091c599f..2d81d09974 100644
--- a/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java
+++ b/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java
@@ -498,6 +498,70 @@ public void expectLobWithOverflowingEndIndexToFailCleanly(boolean constructFromB
         reader.close();
     }
 
+    @Test
+    public void expectIncompleteAnnotationWrapperWithOverflowingEndIndexToFailCleanly() {
+        // This test exercises the case where an annotation wrapper's length itself does not overflow a java long,
+        // but its end index (calculated by adding the reader's current index in the buffer to the value's length) does.
+        // Note: this is only tested for fixed input sources because non-fixed sources will wait for more input
+        // to determine if the annotation wrapper is well-formed. The next test tests a similar failure case for
+        // both types of inputs.
+        IonReaderContinuableCoreBinary reader = initializeReader(
+            true,
+            0xE0, 0x01, 0x00, 0xEA,
+            0xEE,
+            0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFE // 9-byte VarUInt with value just below Long.MAX_VALUE
+        );
+        assertThrows(IonException.class, reader::nextValue);
+        reader.close();
+    }
+
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void expectCompleteAnnotationWrapperWithOverflowingEndIndexToFailCleanly(boolean constructFromBytes) {
+        // This test exercises the case where an annotation wrapper's length itself does not overflow a java long,
+        // but its end index (calculated by adding the reader's current index in the buffer to the value's length) does.
+        // Unlike the previous test, this test contains the remaining bytes of the annotation wrapper, allowing the
+        // cursor to advance to a failure condition even when the input is not fixed.
+        IonReaderContinuableCoreBinary reader = initializeReader(
+            constructFromBytes,
+            0xE0, 0x01, 0x00, 0xEA,
+            0xEE,
+            0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFE, // 9-byte VarUInt with value just below Long.MAX_VALUE
+            0x81, 0x83
+        );
+        assertThrows(IonException.class, reader::nextValue);
+        reader.close();
+    }
+
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void expectLobWithOverflowingLengthToFailCleanly(boolean constructFromBytes) {
+        // This test exercises the case where a value's length overflows a java long.
+        IonReaderContinuableCoreBinary reader = initializeReader(
+            constructFromBytes,
+            0xE0, 0x01, 0x00, 0xEA, // IVM
+            0x9E, // clob with length VarUInt
+            0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF, // 10-byte VarUInt with value that exceeds Long.MAX_VALUE
+            0x00 // The first byte of the clob
+        );
+        assertThrows(IonException.class, reader::nextValue);
+        reader.close();
+    }
+
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void expectAnnotationWrapperWithOverflowingLengthToFailCleanly(boolean constructFromBytes) {
+        // This test exercises the case where an annotation wrapper's length overflows a java long.
+        IonReaderContinuableCoreBinary reader = initializeReader(
+            constructFromBytes,
+            0xE0, 0x01, 0x00, 0xEA,
+            0xEE,
+            0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF // 10-byte VarUInt with value that exceeds Long.MAX_VALUE
+        );
+        assertThrows(IonException.class, reader::nextValue);
+        reader.close();
+    }
+
     @Test
     public void expectIncompleteContainerToFailCleanlyAfterFieldSid() {
         IonReaderContinuableCoreBinary reader = initializeReader(