Skip to content

[BUG]ArrayIndexOutOfBoundsException in skipComment() on a truncated multi-line comment #7825

Description

@Namron2000

Version: 2.0.64 (latest release, reproduced against the published jar)
Classes: com.alibaba.fastjson2.JSONReaderUTF8, com.alibaba.fastjson2.JSONReaderUTF16

Parsing a document that ends inside a multi-line comment, on the comment's *, throws
ArrayIndexOutOfBoundsException instead of JSONException.

JSON.parse("{/**");
java.lang.ArrayIndexOutOfBoundsException: Index 4 out of bounds for length 4
    at com.alibaba.fastjson2.JSONReaderUTF8.skipComment(JSONReaderUTF8.java:5126)
    at com.alibaba.fastjson2.JSONReader.readObject(JSONReader.java:3788)
    at com.alibaba.fastjson2.JSONReader.read(JSONReader.java:3281)
    at com.alibaba.fastjson2.JSON.parse(JSON.java:142)

Four bytes are enough. JSON.parse("{/*") — one byte shorter — already raises
JSONException correctly, which is the behaviour expected here.

Cause

JSONReaderUTF8.skipComment() scans for the */ that ends a multi-line comment
(JSONReaderUTF8.java:5123-5128):

ch = bytes[offset++];

while (true) {
    boolean endOfComment = false;
    if (multi) {
        if (ch == '*'
                && offset <= end && bytes[offset] == '/') {     // <-- line 5127
            offset++;
            endOfComment = true;
        }
    } else {
        endOfComment = ch == '\n';
    }

The bound is offset <= end. end is the exclusive limit, so offset == end passes the
check and bytes[end] reads one element past the array. It is reached whenever the last
character of the document is the * of a would-be */.

The guard is present — it is simply one off. The same method uses the correct form three
times within twenty lines:

if (endOfComment) {
    if (offset >= this.end) { ch = EOI; break; }        // correct

    ch = bytes[offset];
    while (ch <= ' ' && ((1L << ch) & SPACE) != 0) {
        offset++;
        if (offset >= this.end) { ch = EOI; break; }    // correct
        ch = bytes[offset];
    }

and the method's entry guard is if (offset + 1 >= this.end) throw new JSONException(...).

Which inputs reach it

Exactly those where a multi-line comment's final * is the last character:

input result
{/** AIOOBE
[/** AIOOBE
{"a":1,/** AIOOBE
{/*x* AIOOBE
{/*** AIOOBE
{/* JSONException — no trailing *, caught by the entry guard
{/**/ JSONException — comment closed, document truncated
{/**x JSONException* is not the last character
{//, {//x JSONException — the single-line branch tests ch == '\n' and never indexes ahead

Only the multi branch looks ahead, which is why single-line comments are unaffected.

Comments are a fastjson2 extension rather than standard JSON, but the parser accepts them
by default, so no feature flag is needed to reach this from untrusted input.

The same comparison is in JSONReaderUTF16

file line code
JSONReaderUTF8.java 5127 && offset <= end && bytes[offset] == '/'
JSONReaderUTF16.java 3833 && offset <= end && chars[offset] == '/'

The UTF16 one is live as well — it just needs a UTF16-backed String, which is why a
byte-oriented fuzzer does not find it:

JSON.parse("{\"\":1,/**");
// java.lang.ArrayIndexOutOfBoundsException
//     at com.alibaba.fastjson2.JSONReaderUTF16.skipComment(JSONReaderUTF16.java:3832)

JSONReaderASCII extends JSONReaderUTF8 and does not override skipComment(), so it
inherits whatever is done in the base class.

Fix

Two hunks, one comparison each:

     while (true) {
         boolean endOfComment = false;
         if (multi) {
             if (ch == '*'
-                    && offset <= end && bytes[offset] == '/') {
+                    && offset < end && bytes[offset] == '/') {
                 offset++;
                 endOfComment = true;
             }

and the identical change over chars[] in JSONReaderUTF16.skipComment().

With <, a document ending on * never matches the terminator, falls out of the loop the
way any other unterminated comment does, and is reported as JSONException — matching
what {/* does today. Nothing else in the method changes.

Verification

Both readers patched from the 2.0.64 sources and compiled ahead of the published jar.

case stock 2.0.64 patched
{/**, [/**, {"a":1,/**, {/*x*, {/*** AIOOBE at JSONReaderUTF8.skipComment:5126 JSONException
{"中":1,/**, ["中"/** AIOOBE at JSONReaderUTF16.skipComment:3832 JSONException
the original 41-byte fuzzer input AIOOBE JSONException
{/*, {//, {/**/, {/**x, {//x JSONException JSONException
{/*c*/"a":1} {"a":1} {"a":1}
[1,/*c*/2] [1,2] [1,2]
{"a":1}//x {"a":1} {"a":1}
{/**/} {} {}
{//\n} {} {}
{"中":1} {"中":1} {"中":1}

No behaviour change for any valid input, and every already-clean rejection stays clean.

Not a security issue

The out-of-range access is a read of bytes[end], and the JVM's bounds check fires
before it — nothing is read or disclosed, and the exception message carries only the
input length, which the sender already knows.

The impact is the API-contract one: JSON.parse() on untrusted input throws an unchecked
ArrayIndexOutOfBoundsException, so code following the documented pattern

try { JSON.parse(untrusted); } catch (JSONException e) { /* reject */ }

does not catch it, and a four-byte input propagates a RuntimeException past the intended
error handling.


Found by the CISPA Fandango-Team

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions