Skip to content

fix: bound jsonb array item count to remaining input - #7737

Open
Support-itecz wants to merge 3 commits into
alibaba:mainfrom
Support-itecz:jsonb-array-item-count
Open

fix: bound jsonb array item count to remaining input#7737
Support-itecz wants to merge 3 commits into
alibaba:mainfrom
Support-itecz:jsonb-array-item-count

Conversation

@Support-itecz

Copy link
Copy Markdown

What this PR does / why we need it?

JSONReaderJSONB sizes array allocations from the item count on the wire without checking it against the input. startArray() returns the BC_ARRAY/BC_BINARY count from readInt32Value() unbounded, and readObject(), readAny() and readArray() each size a JSONArray/ArrayList from a BC_ARRAY length. The 6 byte payload A4 48 7F FF FF FF declares Integer.MAX_VALUE items, so JSONB.parseObject(payload, long[].class) reaches new long[Integer.MAX_VALUE] and dies with OutOfMemoryError; every primitive, boxed and object array reader behaves the same, and 0x0FFFFFFF stays under the 256MB cap in readLength() while doing the same through JSONB.parse and List/Map targets.

The count is also what keeps element reads inside the message. Under JSONB.parseObject(byte[], off, len, type) the reader's end is the frame boundary, not the end of the buffer, so the two byte frame A4 08 declaring eight items runs the item loop past end and returns the eight bytes that follow it. For a framed protocol decoding messages out of a shared or pooled buffer, that hands the caller data belonging to the neighboring frame.

Summary of your change

A JSONB item always costs at least one byte, so a count larger than the bytes left in the input is malformed by construction. checkItemCnt() rejects counts that are negative or larger than end - offset, applied at the four places a count comes off the wire, which also covers the array readers that reach it through startArray().

The bound sits in the reader rather than in each ObjectReaderImpl*Array because that is where end is known, and it follows the existing checkBigintLen guard. JSONBArrayItemCountTest covers the four paths, the read past end, and valid arrays including a 100k element one; without the fix it exhausts the forked test JVM, with it the core suite stays green.

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

A declared BC_ARRAY/BC_BINARY item count larger than the bytes left in the input sized allocations directly and let element reads run past end.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +46 to +48
@Test
public void testInt32ValueArrayItemCountOverflow() {
assertThrows(JSONException.class, () -> JSONB.parseObject(ARRAY_MAX_CNT, int[].class));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The BC_BINARY branch in startArray() (line 1438 of JSONReaderJSONB.java) has no test coverage. All 12 tests use BC_ARRAY (0xA4) inputs; none exercise BC_BINARY (0x91). The mutation "delete checkItemCnt from the BC_BINARY line" survives every test in this file. — Concrete cost: a future refactor that accidentally drops the guard on the BC_BINARY path would reintroduce the OOM vector for payloads like {0x91, 0x48, 0x7F, 0xFF, 0xFF, 0xFF} with no test to catch it.

Suggested change
@Test
public void testInt32ValueArrayItemCountOverflow() {
assertThrows(JSONException.class, () -> JSONB.parseObject(ARRAY_MAX_CNT, int[].class));
@Test
public void testInt32ValueArrayItemCountOverflow() {
assertThrows(JSONException.class, () -> JSONB.parseObject(ARRAY_MAX_CNT, int[].class));
}
@Test
public void testBinaryArrayItemCountOverflow() {
// 0x91 = BC_BINARY, 0x48 = BC_INT32, 0x7FFFFFFF = Integer.MAX_VALUE
byte[] binaryMaxCnt = {
(byte) 0x91,
(byte) 0x48,
(byte) 0x7F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF
};
assertThrows(JSONException.class, () -> JSONB.parseObject(binaryMaxCnt, int[].class));
}

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Added testBinaryArrayItemCountOverflow with the 0x91 payload; int[].class routes through the BC_BINARY branch in startArray(), so dropping that guard now fails the test instead of surviving.

Comment on lines +6316 to +6318
private int checkItemCnt(int itemCnt) {
if (itemCnt < 0 || itemCnt > end - offset) {
throw new JSONException("array item count out of range: " + itemCnt + ", available: " + (end - offset));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test exercises the boundary itemCnt == end - offset. The off-by-one mutation >>= survives all 12 tests and would silently reject valid arrays whose declared count exactly equals the remaining bytes. — Concrete cost: a future "simplification" of this condition would break parsing of tightly-packed single-byte-element arrays (e.g. {0xA4, 0x03, 0x01, 0x02, 0x03}) with no test to catch it.

Consider adding a boundary test:

@Test
public void testItemCountExactlyMatchesRemainingBytes() {
    // BC_ARRAY, count=3 (single-byte int 0x03), three 1-byte items
    byte[] buffer = {
            (byte) 0xA4, 0x03,
            0x01, 0x02, 0x03
    };
    assertArrayEquals(new int[]{1, 2, 3}, JSONB.parseObject(buffer, int[].class));
}

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as testItemCountExactlyMatchesRemainingBytes, so the > vs >= boundary is pinned and a tightly packed array still parses. All 14 tests pass locally.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-1 skipValue()'s BC_ARRAY branch (line 2559) reads array item count via bare readInt32Value() without checkItemCnt, leaving the exact vulnerability class this PR fixes (OOM/OOB read from malformed count) reachable through any bean deserialization path that skips an unrecognized field containing an array. Probe confirmed: AIOOBE on unmodified code, clean JSONException with the one-line fix. Suggested fix: itemCnt = checkItemCnt(readInt32Value());

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Comment on lines +1098 to 1099
? checkItemCnt(readLength())
: valueType - BC_ARRAY_FIX_MIN;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No error-path test covers this checkItemCnt call site (the nested-array element path inside readArray()). All other four call sites have dedicated overflow tests. — Concrete cost: if a future refactor accidentally drops checkItemCnt from this site only, new JSONArray(0x0FFFFFFF) at line 1107 would attempt a ~268M-element allocation (OOM), and no test in this PR would fail.

Suggested test: outer BC_ARRAY with count 2, first element a small int (0x01), second element BC_ARRAY + BC_INT32 + 0x0FFFFFFF. Assert JSONException.

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as testNestedArrayItemCountOverflow. One note: with a List.class target the elements route through readAny(), which has its own guard, so the test goes through JSONB.parseArray to actually land on this branch. Confirmed via stack trace that the exception comes from the readArray() site.

len = itemType;
} else {
len = readLength();
len = checkItemCnt(readLength());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No error-path test covers this checkItemCnt call site (the readObject() field-value branch). testReadObjectItemCountOverflow uses Map.class, which routes through ObjectReaderImplMapstartArray() (line 1445) — it does not reach this line. — Concrete cost: if a future refactor drops checkItemCnt here, JSONB.parseObject(bytes) (the no-type overload) with a malformed object reaches new JSONArray(0x0FFFFFFF) → OOM, and no test catches it. Probe confirmed: removing the guard flips from pass to OutOfMemoryError.

Suggested test: assertThrows(JSONException.class, () -> JSONB.parseObject(OBJECT_ARRAY_LARGE_CNT));

— qwen3.8-max-preview via Qwen Code /review (v0.21.3)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as testReadObjectNoTypeItemCountOverflow using the no-type overload. Stack trace confirms it lands on the field-value branch here rather than startArray().

@Support-itecz

Copy link
Copy Markdown
Author

Good catch on skipValue(). Its BC_ARRAY branch now runs the count through checkItemCnt() as well, and testSkipValueItemCountOverflow covers it through a bean with an unknown field holding a truncated array. Without the guard that path reproduces the AIOOBE you describe, with it a clean JSONException. Core suite (7964 tests) and checkstyle both pass.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants