Skip to content

Commit

Permalink
Adds checks for malformed data that overflows the buffer.
Browse files Browse the repository at this point in the history
  • Loading branch information
tgregg committed Jan 2, 2024
1 parent 1a09a29 commit 1899cdd
Show file tree
Hide file tree
Showing 6 changed files with 236 additions and 40 deletions.
17 changes: 10 additions & 7 deletions src/main/java/com/amazon/ion/impl/IonCursorBinary.java
Original file line number Diff line number Diff line change
Expand Up @@ -733,12 +733,9 @@ private boolean slowSeek(long numberOfBytes) {
private long uncheckedReadVarUInt_1_0(byte currentByte) {
long result = currentByte & LOWER_SEVEN_BITS_BITMASK;
do {
// Note: if the varUInt is malformed such that it extends beyond the declared length of the value *and*
// beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared
// value length has been filled. Preventing this is simple: if (peekIndex >= limit) throw
// new IonException(); However, we choose not to perform that check here because it is not worth sacrificing
// performance in this inner-loop code in order to throw one type of exception over another in case of
// malformed data.
if (peekIndex >= limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
currentByte = buffer[(int) (peekIndex++)];
result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
} while (currentByte >= 0);
Expand Down Expand Up @@ -778,6 +775,9 @@ private long slowReadVarUInt_1_0() {
private boolean uncheckedReadAnnotationWrapperHeader_1_0(IonTypeID valueTid) {
long endIndex;
if (valueTid.variableLength) {
if (peekIndex >= limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
byte b = buffer[(int) peekIndex++];
if (b < 0) {
endIndex = (b & LOWER_SEVEN_BITS_BITMASK);
Expand All @@ -790,7 +790,7 @@ private boolean uncheckedReadAnnotationWrapperHeader_1_0(IonTypeID valueTid) {
endIndex += peekIndex;
setMarker(endIndex, valueMarker);
if (endIndex > limit) {
return true;
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
byte b = buffer[(int) peekIndex++];
int annotationsLength;
Expand Down Expand Up @@ -886,6 +886,9 @@ private void prohibitEmptyOrderedStruct_1_0(Marker marker) {
private long calculateEndIndex_1_0(IonTypeID valueTid, boolean isAnnotated) {
long endIndex;
if (valueTid.variableLength) {
if (peekIndex >= limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
byte b = buffer[(int) peekIndex++];
if (b < 0) {
endIndex = (b & LOWER_SEVEN_BITS_BITMASK);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,34 +157,15 @@ int readVarUInt_1_0() {
int currentByte = 0;
int result = 0;
while ((currentByte & HIGHEST_BIT_BITMASK) == 0) {
if (peekIndex >= limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
currentByte = buffer[(int)(peekIndex++)];
result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
}
return result;
}

/**
* Reads a 2+ byte VarUInt, given the first byte. When called, `peekIndex` must point at the second byte in the
* VarUInt. When this method returns, `peekIndex` will point at the first byte that follows the VarUInt.
* NOTE: the VarUInt must fit in an `int`.
* @param currentByte the first byte in the VarUInt.
* @return the value.
*/
int readVarUInt_1_0(byte currentByte) {
int result = currentByte & LOWER_SEVEN_BITS_BITMASK;
do {
// Note: if the varUInt is malformed such that it extends beyond the declared length of the value *and*
// beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared
// value length has been filled. Preventing this is simple: if (peekIndex >= valueMarker.endIndex) throw
// new IonException(); However, we choose not to perform that check here because it is not worth sacrificing
// performance in this inner-loop code in order to throw one type of exception over another in case of
// malformed data.
currentByte = buffer[(int) (peekIndex++)];
result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
} while (currentByte >= 0);
return result;
}

/**
* Reads a 2+ byte VarInt, given the first byte. When called, `peekIndex` must point at the second byte in the
* VarInt. at `peekIndex`.When this method returns, `peekIndex` will point at the first byte that follows the
Expand All @@ -197,12 +178,9 @@ private int readVarInt_1_0(int firstByte) {
int sign = (currentByte & VAR_INT_SIGN_BITMASK) == 0 ? 1 : -1;
int result = currentByte & LOWER_SIX_BITS_BITMASK;
while ((currentByte & HIGHEST_BIT_BITMASK) == 0) {
// Note: if the varInt is malformed such that it extends beyond the declared length of the value *and*
// beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared
// value length has been filled. Preventing this is simple: if (peekIndex >= valueMarker.endIndex) throw
// new IonException(); However, we choose not to perform that check here because it is not worth sacrificing
// performance in this inner-loop code in order to throw one type of exception over another in case of
// malformed data.
if (peekIndex >= limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
currentByte = buffer[(int)(peekIndex++)];
result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
}
Expand Down Expand Up @@ -518,11 +496,13 @@ public boolean isNullValue() {
}

/**
* Performs any logic necessary to prepare a scalar value for parsing. In general nothing needs to be done, but
* subclasses may wish to provide different behavior, such as ensuring that the value is present in the buffer.
* Performs any logic necessary to prepare a scalar value for parsing. Subclasses may wish to provide additional
* logic, such as ensuring that the value is present in the buffer.
*/
void prepareScalar() {
// May be overridden.
if (valueMarker.endIndex > limit) {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,12 @@ public IonType getType() {
void prepareScalar() {
if (!isValueIncomplete) {
if (!isSlowMode || event == IonCursor.Event.VALUE_READY) {
// Nothing to do.
super.prepareScalar();
return;
}
if (isFillRequired) {
if (fillValue() == Event.VALUE_READY) {
super.prepareScalar();
return;
}
if (event == Event.NEEDS_INSTRUCTION) {
Expand Down
98 changes: 98 additions & 0 deletions src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package com.amazon.ion.impl;

import com.amazon.ion.IonCursor;
import com.amazon.ion.IonException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

Expand All @@ -26,6 +28,7 @@
import static com.amazon.ion.impl.IonCursorTestUtilities.scalar;
import static com.amazon.ion.impl.IonCursorTestUtilities.startContainer;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class IonCursorBinaryTest {

Expand Down Expand Up @@ -299,4 +302,99 @@ public void fillContainerThenSkip(boolean constructFromBytes) {
endStream()
);
}

@Test
public void expectMalformedListHeaderToFailCleanly() {
// The following test is expected to fail because the VarUInt length would extend beyond the end of the buffer.
// Since a byte array is provided as the input source, no additional input can be provided, so this must be
// treated as an error.
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xBE, // List with VarUInt length
0x01 // Non-terminal VarUInt byte
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMissingListLengthToFailCleanly() {
// The following test is expected to fail because the VarUInt length is missing.
// Since a byte array is provided as the input source, no additional input can be provided, so this must be
// treated as an error.
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xBE // List with VarUInt length, no length follows
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMalformedStructFieldNameToFailCleanly() {
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xD2, // Struct length 2
0x01, // Non-terminal VarUInt byte
0x01, // Non-terminal VarUInt byte
0x01 // Non-terminal VarUInt byte
);
cursor.nextValue();
cursor.stepIntoContainer();
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMissingAnnotationWrapperLengthToFailCleanly() {
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xEE // Annotation wrapper with VarUInt length, no length follows
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMalformedAnnotationWrapperHeaderToFailCleanly() {
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xEE, // Annotation wrapper with VarUInt length
0x01 // Non-terminal VarUInt byte
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMalformedAnnotationWrapperAnnotationLengthToFailCleanly() {
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xEE, // Annotation wrapper with VarUInt length
0x83, // VarUInt length 3
0x01, // Non-terminal VarUInt byte
0x01, // Non-terminal VarUInt byte
0x01 // Non-terminal VarUInt byte
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}

@Test
public void expectMissingAnnotationWrapperAnnotationLengthToFailCleanly() {
IonCursorBinary cursor = initializeCursor(
true,
0xE0, 0x01, 0x00, 0xEA,
0xEE, // Annotation wrapper with VarUInt length
0x83 // VarUInt length 3, no annotation length follows
);
assertThrows(IonException.class, cursor::nextValue);
cursor.close();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

package com.amazon.ion.impl;

import com.amazon.ion.IonException;
import com.amazon.ion.IonType;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

Expand All @@ -25,6 +27,7 @@
import static com.amazon.ion.impl.IonCursorTestUtilities.startContainer;
import static com.amazon.ion.impl.IonCursorTestUtilities.fillStringValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class IonReaderContinuableCoreBinaryTest {

Expand Down Expand Up @@ -290,4 +293,102 @@ public void fillContainerThenSkip(boolean constructFromBytes) {
endStream()
);
}

@Test
public void expectMalformedDecimalScaleToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x51, // Decimal length 1
0x01 // Non-terminal VarInt byte
);
reader.nextValue();
assertThrows(IonException.class, reader::decimalValue);
assertThrows(IonException.class, reader::bigDecimalValue);
reader.close();
}

@Test
public void expectMissingDecimalScaleToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x51 // Decimal length 1, missing scale
);
reader.nextValue();
assertThrows(IonException.class, reader::decimalValue);
assertThrows(IonException.class, reader::bigDecimalValue);
reader.close();
}

@Test
public void expectMalformedDecimalHeaderToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x53, // Decimal length 3, only 2 bytes follow
0xC1, // VarInt -1
0x01 // Non-terminal VarInt byte
);
reader.nextValue();
assertThrows(IonException.class, reader::decimalValue);
assertThrows(IonException.class, reader::bigDecimalValue);
reader.close();
}

@Test
public void expectMalformedIntHeaderToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x21 // Int length 1
);
reader.nextValue();
assertThrows(IonException.class, reader::intValue);
assertThrows(IonException.class, reader::bigIntegerValue);
assertThrows(IonException.class, reader::getIntegerSize);
reader.close();
}

@Test
public void expectMalformedTimestampOffsetToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x63, // Timestamp length 3
0x01, // Non-terminal VarInt byte
0x01, // Non-terminal VarInt byte
0x01 // Non-terminal VarInt byte
);
reader.nextValue();
assertThrows(IonException.class, reader::timestampValue);
reader.close();
}

@Test
public void expectMalformedTimestampYearToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x63, // Timestamp length 3
0xC0, // VarInt -0 (unknown offset)
0x01, // Non-terminal VarUInt byte
0x01 // Non-terminal VarUInt byte
);
reader.nextValue();
assertThrows(IonException.class, reader::timestampValue);
reader.close();
}

@Test
public void expectMissingTimestampBodyToFailCleanly() {
IonReaderContinuableCoreBinary reader = initializeReader(
true,
0xE0, 0x01, 0x00, 0xEA,
0x63 // Timestamp length 3, missing body
);
reader.nextValue();
assertThrows(IonException.class, reader::timestampValue);
reader.close();
}
}
Loading

0 comments on commit 1899cdd

Please sign in to comment.