Skip to content

Serialize ObjectArrayMessage elements through the filtered wrapper - #4274

Open
ppkarwasz wants to merge 1 commit into
feat/2.x/string-map-deserialization-boundsfrom
feat/2.x/object-array-message-wrapped
Open

Serialize ObjectArrayMessage elements through the filtered wrapper#4274
ppkarwasz wants to merge 1 commit into
feat/2.x/string-map-deserialization-boundsfrom
feat/2.x/object-array-message-wrapped

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

Important

This PR is part of the deserialization hardening work tracked in #4168. The Logging Services PMC does not use nor recommend Java serialization/deserialization, and our security FAQ has long documented this position. This work is submitted solely to reduce the false-positive "vulnerability" reports that keep being filed regardless of that FAQ. Its utility for end users is close to zero.

ObjectArrayMessage.readObject read its Object[] with a plain readObject() call — the last remaining direct object read in a shipped readObject method. It now uses the same per-element writeWrappedObject/readWrappedObject mechanism as ParameterizedMessage, which re-applies the deserialization allowlist to each element inside its own nested stream, degrades a rejected or unreadable element to null instead of losing the whole array, and replaces non-Serializable elements with their String.valueOf representation on the writing side.

A single wrapped blob of the whole array (the literal ObjectMessage shape) was considered and rejected: the Object[] would then be allocated inside the JDK's nested stream, where a forged array header still forces an unbounded eager allocation, and the resulting OutOfMemoryError escapes readWrappedObject's catch (Exception | LinkageError).

The shared loop moves to SerializationUtil.writeWrappedObjects/readWrappedObjects, which also bounds the allocation during deserialization: at most 256 elements are pre-allocated and the array grows as elements are actually read, so a forged length fails on the missing data instead of committing the reader to a large allocation. ParameterizedMessage — whose serialized form is byte-for-byte unchanged — picks up the same bound on its previously eager new Object[argCount].

Compatibility: the serialized form of ObjectArrayMessage changes; streams written by earlier versions are rejected by newer readers and vice versa. ParameterizedMessage and ObjectMessage are unaffected.

Stacked on #4272.

`ObjectArrayMessage.readObject` read its `Object[]` with a plain
`readObject()` call, the last remaining direct object read in a
shipped `readObject` method. It now uses the same per-element
`writeWrappedObject`/`readWrappedObject` mechanism as
`ParameterizedMessage`, which re-applies the deserialization allowlist
to each element and replaces non-`Serializable` elements with their
`String.valueOf` representation on the writing side.

The shared loop moves to `SerializationUtil.writeWrappedObjects`/
`readWrappedObjects`, which also bounds the array allocation during
deserialization: at most 256 elements are pre-allocated and the array
grows as elements are actually read, so a forged length cannot force a
large allocation. `ParameterizedMessage`, whose serialized form is
unchanged, picks up the same bound on its previously eager
`new Object[argCount]`.

Compatibility: the serialized form of `ObjectArrayMessage` changes;
streams written by earlier versions are rejected by newer readers and
vice versa.

Part of the hardening series from #4168.

Assisted-By: Claude Fable 5 <[email protected]>
@ramanathan1504

Copy link
Copy Markdown
Contributor

ParameterizedMessage is byte for byte the same, I checked. ObjectArrayMessage also degrades a non-serializable element now instead of throwing.

But the reason for rejecting the single blob applies here too. Each element wrapper is a byte[] read by in.readObject() on the outer stream, at a length the JDK trusts. I
patched that length in a 103 byte ObjectMessage from 12 to 400 million and both filter paths give OutOfMemoryError under -Xmx64m. Can we bound
filterInfo.arrayLength() in DefaultObjectInputFilter instead?

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

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants