Skip to content

Reject symbol IDs and VarInts that overflow int in the binary reader #1173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
radhika1314 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reject symbol IDs and VarInts that overflow int in the binary reader #1173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions src/main/java/com/amazon/ion/impl/IonCursorBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,19 @@ private long slowReadVarUInt_1_0() {
throw new IonException("Found a VarUInt that was too large to fit in a `long`");
}

/**
* Narrows the given symbol ID to an `int`, throwing if it does not fit. A symbol ID that does not fit in an `int`
* can never be mapped by a symbol table; wrapping it around would silently resolve it to a different symbol.
* @param sid the symbol ID.
* @return the symbol ID as an `int`.
*/
static int symbolIdToInt(long sid) {
if (sid < 0 || sid > Integer.MAX_VALUE) {
throw new IonException("Found a symbol ID that was too large to fit in an `int`.");
}
return (int) sid;
}

/**
* Reads the header of an annotation wrapper. This must only be called when it is known that the buffer already
* contains all the bytes in the header. Sets `valueMarker` with the start and end indices of the wrapped value.
Expand Down Expand Up @@ -976,8 +989,12 @@ private boolean slowReadFieldName_1_0() {
if (!fillAt(peekIndex, 2)) {
return true;
}
fieldSid = (int) slowReadVarUInt_1_0();
return fieldSid < 0;
long sid = slowReadVarUInt_1_0();
if (sid < 0) {
return true;
}
fieldSid = symbolIdToInt(sid);
return false;
}

/* ---- Ion 1.1 ---- */
Expand Down Expand Up @@ -1571,7 +1588,7 @@ private boolean uncheckedNextContainedToken() {
if (b < 0) {
fieldSid = (b & LOWER_SEVEN_BITS_BITMASK);
} else {
fieldSid = (int) uncheckedReadVarUInt_1_0(b);
fieldSid = symbolIdToInt(uncheckedReadVarUInt_1_0(b));
}
} else {
uncheckedReadFieldName_1_1();
Expand Down
50 changes: 41 additions & 9 deletions src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ class IonReaderContinuableCoreBinary extends IonCursorBinary implements IonReade
// The second-most significant bit in the most significant byte of a VarInt is the sign.
private static final int VAR_INT_SIGN_BITMASK = 0x40;

// The largest VarInt magnitude that can be decoded. This allows -2^31 (Integer.MIN_VALUE) as well as the
// exponent 2^31, which corresponds to a BigDecimal scale of Integer.MIN_VALUE.
private static final long MAXIMUM_VAR_INT_MAGNITUDE = -(long) Integer.MIN_VALUE;

// 32-bit floats must declare length 4.
private static final int FLOAT_32_BYTE_LENGTH = 4;

Expand Down Expand Up @@ -160,6 +164,9 @@ int readVarUInt_1_0() {
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream.");
}
currentByte = buffer[(int)(peekIndex++)];
if (result > (Integer.MAX_VALUE >>> VALUE_BITS_PER_VARUINT_BYTE)) {
throw new IonException("Found a VarUInt that was too large to fit in an `int`.");
}
result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
}
return result;
Expand All @@ -168,33 +175,51 @@ int readVarUInt_1_0() {
/**
* 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
* VarInt. NOTE: the VarInt must fit in an `int`.
* VarInt. NOTE: the magnitude of the VarInt must not exceed `MAXIMUM_VAR_INT_MAGNITUDE`; callers must check that
* the value fits in an `int` before narrowing it.
* @param firstByte the first byte of the VarInt representation, which has already been retrieved from the buffer.
* @return the value.
*/
private int readVarInt_1_0(int firstByte) {
private long readVarInt_1_0(int firstByte) {
int currentByte = firstByte;
int sign = (currentByte & VAR_INT_SIGN_BITMASK) == 0 ? 1 : -1;
int result = currentByte & LOWER_SIX_BITS_BITMASK;
long result = currentByte & LOWER_SIX_BITS_BITMASK;
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);
if (result > MAXIMUM_VAR_INT_MAGNITUDE) {
throw new IonException("Found a VarInt that was too large to fit in an `int`.");
}
}
return result * sign;
}

/**
* Reads the VarInt starting at `peekIndex`. When this method returns, `peekIndex` will point at the first byte
* that follows the VarUInt. NOTE: the VarInt must fit in an `int`.
* that follows the VarUInt. NOTE: the magnitude of the VarInt must not exceed `MAXIMUM_VAR_INT_MAGNITUDE`;
* callers must check that the value fits in an `int` before narrowing it.
* @return the value.
*/
private int readVarInt_1_0() {
private long readVarInt_1_0() {
return readVarInt_1_0(buffer[(int)(peekIndex++)]);
}

/**
* Reads the VarInt exponent of the decimal that begins at `peekIndex` and negates it to produce the scale of the
* decimal. When this method returns, `peekIndex` will point at the first byte of the decimal's coefficient.
* @return the scale.
*/
private int readDecimalScale_1_0() {
long scale = -readVarInt_1_0();
if (scale < Integer.MIN_VALUE || scale > Integer.MAX_VALUE) {
throw new IonException("Found a decimal exponent that was too large to fit in an `int`.");
}
return (int) scale;
}

/**
* Reads into a BigInteger the UInt value that begins at `valueMarker.startIndex` and ends at
* `valueMarker.endIndex`.
Expand Down Expand Up @@ -250,7 +275,7 @@ private BigInteger readIntAsBigInteger_1_0(int length) {
* @return the value.
*/
private BigDecimal readBigDecimal_1_0() {
int scale = -readVarInt_1_0();
int scale = readDecimalScale_1_0();
BigDecimal value;
int length = (int) (valueMarker.endIndex - peekIndex);
if (length < LONG_SIZE_IN_BYTES) {
Expand Down Expand Up @@ -278,7 +303,7 @@ private BigDecimal readBigDecimal_1_0() {
* @return the value.
*/
private Decimal readDecimal_1_0() {
int scale = -readVarInt_1_0();
int scale = readDecimalScale_1_0();
BigInteger coefficient;
int length = (int) (valueMarker.endIndex - peekIndex);
if (length > 0) {
Expand Down Expand Up @@ -338,7 +363,11 @@ private Timestamp readTimestamp_1_0() {
int firstByte = buffer[(int)(peekIndex++)] & SINGLE_BYTE_MASK;
Integer offset = null;
if (firstByte != VAR_INT_NEGATIVE_ZERO) {
offset = readVarInt_1_0(firstByte);
long offsetMinutes = readVarInt_1_0(firstByte);
if (offsetMinutes < Integer.MIN_VALUE || offsetMinutes > Integer.MAX_VALUE) {
throw new IonException("Found a timestamp offset that was too large to fit in an `int`.");
}
offset = (int) offsetMinutes;
}
int year = readVarUInt_1_0();
int month = 0;
Expand Down Expand Up @@ -796,7 +825,10 @@ public int symbolValueId() {
return -1;
}
prepareScalar();
return (int) readUInt(valueMarker.startIndex, valueMarker.endIndex);
if (valueMarker.endIndex - valueMarker.startIndex > Long.BYTES) {
throw new IonException("Found a symbol ID that was too large to fit in an `int`.");
}
return symbolIdToInt(readUInt(valueMarker.startIndex, valueMarker.endIndex));
}

/**
Expand Down
132 changes: 132 additions & 0 deletions src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,4 +664,136 @@ public void timestampLengthZeroFailsCleanly(boolean constructFromBytes) {
assertThrows(IonException.class, reader::timestampValue);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void fieldSidThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0xD7, // Struct, length 7
0x10, 0x00, 0x00, 0x00, 0x84, // Field name VarUInt 2^32 + 4, which wraps to 4 if narrowed to an int
0x21, 0x01 // Int 1
)
) {
assertEquals(IonCursor.Event.START_CONTAINER, reader.nextValue());
reader.stepIntoContainer();
assertThrows(IonException.class, reader::nextValue);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void annotationSidThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0xE8, // Annotation wrapper, length 8
0x85, // Annotations length 5
0x10, 0x00, 0x00, 0x00, 0x84, // Annotation VarUInt 2^32 + 4, which wraps to 4 if narrowed to an int
0x21, 0x01 // Int 1
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::getAnnotationIds);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void symbolValueIdThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0x75, // Symbol, length 5
0x01, 0x00, 0x00, 0x00, 0x04 // UInt 2^32 + 4, which wraps to 4 if narrowed to an int
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::symbolValueId);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void timestampYearThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0x66, // Timestamp, length 6
0x80, // VarInt 0 (UTC offset)
0x10, 0x00, 0x00, 0x0F, 0xD0 // Year VarUInt 2^32 + 2000, which wraps to 2000 if narrowed to an int
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::timestampValue);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void timestampOffsetThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0x67, // Timestamp, length 7
0x08, 0x00, 0x00, 0x00, 0x80, // Offset VarInt 2^31
0x0F, 0xD0 // Year VarUInt 2000
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::timestampValue);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void decimalExponentThatDoesNotFitInIntFailsCleanly(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0x56, // Decimal, length 6
0x08, 0x00, 0x00, 0x00, 0x81, // Exponent VarInt 2^31 + 1, which becomes -(2^31 - 1) if narrowed to an int
0x01, // Coefficient 1
0x56, // Decimal, length 6
0x48, 0x00, 0x00, 0x00, 0x80, // Exponent VarInt -2^31, whose scale 2^31 does not fit in an int
0x01 // Coefficient 1
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::decimalValue);
assertThrows(IonException.class, reader::bigDecimalValue);
assertEquals(START_SCALAR, reader.nextValue());
assertThrows(IonException.class, reader::decimalValue);
assertThrows(IonException.class, reader::bigDecimalValue);
}
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void decimalExponentAtIntBoundariesSucceeds(boolean constructFromBytes) {
try (
IonReaderContinuableCoreBinary reader = initializeReader(
constructFromBytes,
0xE0, 0x01, 0x00, 0xEA, // IVM
0x56, // Decimal, length 6
0x08, 0x00, 0x00, 0x00, 0x80, // Exponent VarInt 2^31, i.e. scale Integer.MIN_VALUE
0x01, // Coefficient 1
0x56, // Decimal, length 6
0x47, 0x7F, 0x7F, 0x7F, 0xFF, // Exponent VarInt -(2^31 - 1), i.e. scale Integer.MAX_VALUE
0x01 // Coefficient 1
)
) {
assertEquals(START_SCALAR, reader.nextValue());
assertEquals(Integer.MIN_VALUE, reader.bigDecimalValue().scale());
assertEquals(START_SCALAR, reader.nextValue());
assertEquals(Integer.MAX_VALUE, reader.bigDecimalValue().scale());
}
}
}
33 changes: 33 additions & 0 deletions src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,39 @@ public void unknownSymbolInAnnotation(boolean constructFromBytes) throws Excepti
reader.close();
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void fieldNameSidThatDoesNotFitInIntIsNotWrapped(boolean constructFromBytes) throws Exception {
// The field name SID is 2^32 + 4, which would resolve to $4 ('name') if it were narrowed to an int.
reader = readerFor(constructFromBytes, 0xD7, 0x10, 0x00, 0x00, 0x00, 0x84, 0x21, 0x01);
assertSequence(next(IonType.STRUCT), STEP_IN);
assertThrows(IonException.class, reader::next);
reader.close();
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void annotationSidThatDoesNotFitInIntIsNotWrapped(boolean constructFromBytes) throws Exception {
// The annotation SID is 2^32 + 4, which would resolve to $4 ('name') if it were narrowed to an int.
reader = readerFor(constructFromBytes, 0xE8, 0x85, 0x10, 0x00, 0x00, 0x00, 0x84, 0x21, 0x01);
assertThrows(IonException.class, () -> {
reader.next();
reader.getTypeAnnotations();
});
reader.close();
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void symbolValueSidThatDoesNotFitInIntIsNotWrapped(boolean constructFromBytes) throws Exception {
// The symbol value SID is 2^32 + 4, which would resolve to $4 ('name') if it were narrowed to an int.
reader = readerFor(constructFromBytes, 0x75, 0x01, 0x00, 0x00, 0x00, 0x04);
assertSequence(next(IonType.SYMBOL));
assertThrows(IonException.class, reader::symbolValue);
assertThrows(IonException.class, reader::stringValue);
reader.close();
}

@ParameterizedTest(name = "constructFromBytes={0}")
@ValueSource(booleans = {true, false})
public void unknownSymbolInValue(boolean constructFromBytes) throws Exception {
Expand Down