From eddf66c3834e34f45e6276af2eb290f3a530b2c6 Mon Sep 17 00:00:00 2001 From: Arthur Chan Date: Thu, 18 Jan 2024 10:26:51 +0000 Subject: [PATCH 1/2] Fix issue 464: Add multiple checking condition to avoid IOOBE Signed-off-by: Arthur Chan --- .../jackson/dataformat/cbor/CBORParser.java | 14 +++++++++ .../fuzz/CBORFuzz464_65722_IOOBETest.java | 31 +++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java diff --git a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java index 43c19b18a..b6a05fcd2 100644 --- a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java +++ b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java @@ -2374,6 +2374,20 @@ private final String _finishShortText(int len) throws IOException // Let's actually do a tight loop for ASCII first: final int end = inPtr + len; + // If the provided len is malformed, the end value + // could be larger than the length of inputBuf. + // This could cause the following while loop + // not returning in the correct location, + // result in ArrayIndexOutOfBoundsException + // It could also throw AIOOBE if the inPtr + // already pointing to the end of inputBuf + // and no more data is left in inputBuf. + // It will also be a problem if end is + // negative when provided len is negative. + if ((end <= 0) || (end >= inputBuf.length) || (inPtr >= inputBuf.length)) { + throw _constructReadException("Requested length too long."); + } + int i; while ((i = inputBuf[inPtr]) >= 0) { outBuf[outPtr++] = (char) i; diff --git a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java new file mode 100644 index 000000000..98dfbc200 --- /dev/null +++ b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java @@ -0,0 +1,31 @@ +package com.fasterxml.jackson.dataformat.cbor.fuzz; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.core.exc.StreamReadException; + +import com.fasterxml.jackson.databind.ObjectMapper; + +import com.fasterxml.jackson.dataformat.cbor.CBORTestBase; + +public class CBORFuzz464_65722_IOOBETest extends CBORTestBase +{ + private final ObjectMapper MAPPER = cborMapper(); + + public void testInvalidText() throws Exception + { + final byte[] input = { + (byte)-60, (byte)-49, (byte)122, (byte)127, (byte)-1, + (byte)-1, (byte)-1, (byte)15, (byte)110 + }; + try (JsonParser p = MAPPER.createParser(input)) { + try { + p.nextToken(); + p.getTextLength(); + fail("Should not reach here (invalid input)"); + } catch (StreamReadException e) { + verifyException(e, "Requested length too long."); + } + } + } +} From 431c007547a97fbe499baf83ebfe34a168589b5d Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 18 Jan 2024 11:23:54 -0800 Subject: [PATCH 2/2] Rewrote fix, added release notes --- .../jackson/dataformat/cbor/CBORParser.java | 18 +++--------------- .../cbor/fuzz/CBORFuzz464_65722_IOOBETest.java | 5 +++-- release-notes/CREDITS-2.x | 3 +++ release-notes/VERSION-2.x | 3 +++ 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java index b6a05fcd2..a4f1a5f38 100644 --- a/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java +++ b/cbor/src/main/java/com/fasterxml/jackson/dataformat/cbor/CBORParser.java @@ -2289,7 +2289,9 @@ protected void _finishToken() throws IOException // 29-Jan-2021, tatu: as per [dataformats-binary#238] must keep in mind that // the longest individual unit is 4 bytes (surrogate pair) so we // actually need len+3 bytes to avoid bounds checks - final int needed = len + 3; + // 18-Jan-2024, tatu: For malicious input / Fuzzers, need to worry about overflow + // like Integer.MAX_VALUE + final int needed = Math.max(len, len + 3); final int available = _inputEnd - _inputPtr; if ((available >= needed) @@ -2374,20 +2376,6 @@ private final String _finishShortText(int len) throws IOException // Let's actually do a tight loop for ASCII first: final int end = inPtr + len; - // If the provided len is malformed, the end value - // could be larger than the length of inputBuf. - // This could cause the following while loop - // not returning in the correct location, - // result in ArrayIndexOutOfBoundsException - // It could also throw AIOOBE if the inPtr - // already pointing to the end of inputBuf - // and no more data is left in inputBuf. - // It will also be a problem if end is - // negative when provided len is negative. - if ((end <= 0) || (end >= inputBuf.length) || (inPtr >= inputBuf.length)) { - throw _constructReadException("Requested length too long."); - } - int i; while ((i = inputBuf[inPtr]) >= 0) { outBuf[outPtr++] = (char) i; diff --git a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java index 98dfbc200..1563eb91b 100644 --- a/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java +++ b/cbor/src/test/java/com/fasterxml/jackson/dataformat/cbor/fuzz/CBORFuzz464_65722_IOOBETest.java @@ -20,11 +20,12 @@ public void testInvalidText() throws Exception }; try (JsonParser p = MAPPER.createParser(input)) { try { - p.nextToken(); + assertToken(JsonToken.VALUE_STRING, p.nextToken()); + // oddly enough `getText()` didn't do it but this: p.getTextLength(); fail("Should not reach here (invalid input)"); } catch (StreamReadException e) { - verifyException(e, "Requested length too long."); + verifyException(e, "Unexpected end-of-input in VALUE_STRING"); } } } diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index 88f7ecb52..27533395f 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -307,3 +307,6 @@ Arthur Chan (@arthurscchan) (2.17.0) * Contributed #460: (protobuf) Unexpected `NullPointerException` in `ProtobufParser.currentName()` (2.17.0) + * Contributed #464: (cbor) Unexpected `ArrayIndexOutOfBoundsException` in `CBORParser` + for corrupt String value + (2.17.0) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 21696100d..390dff35b 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -40,6 +40,9 @@ Active maintainers: #460: (protobuf) Unexpected `NullPointerException` in `ProtobufParser.currentName()` (fix contributed by Arthur C) #462: (protobuf) `ProtobufParser.currentName()` returns wrong value at root level +#464: (cbor) Unexpected `ArrayIndexOutOfBoundsException` in `CBORParser` + for corrupt String value + (fix contributed by Arthur C) - (ion) Update `com.amazon.ion:ion-java` to 1.11.0 (from 1.10.5) 2.16.1 (24-Dec-2023)