Harden ObjectArrayMessage deserialization with SerializationUtil.assertFiltered#4098
Harden ObjectArrayMessage deserialization with SerializationUtil.assertFiltered#4098SunWeb3Sec wants to merge 11 commits into
Conversation
…rtFiltered Adds a SerializationUtil.assertFiltered(in) call at the top of ObjectArrayMessage#readObject, bringing it in line with the defensive pattern already used by ObjectMessage and ParameterizedMessage. This is a defense-in-depth / consistency fix; the serialized wire format is unchanged so instances produced by older versions continue to round-trip. Signed-off-by: SunWeb3Sec <infosecpt@gmail.com>
|
@SunWeb3Sec, would you mind applying this DiD in all |
…tream) Extends the previous ObjectArrayMessage hardening to every remaining private void readObject(ObjectInputStream) implementation across log4j-api, log4j-core, log4j-1.2-api, log4j-slf4j-impl, log4j-slf4j2-impl, and log4j-perf-test, per reviewer request. Defense-in-depth only; serialized wire format is unchanged. Signed-off-by: SunWeb3Sec <infosecpt@gmail.com>
|
@vy Done, broadened to all private void readObject(ObjectInputStream) hits across the listed modules. Please review. thanks |
vy
left a comment
There was a problem hiding this comment.
@SunWeb3Sec, thanks so much for the contribution and prompt reaction. Changes LGTM.
|
@SunWeb3Sec, could you please fix the test failures? Make sure |
The PR's `assertFiltered(in)` calls reject plain `ObjectInputStream` on Java 8. Update the affected tests to deserialize via `SerialUtil` (or `FilteredObjectInputStream` for the JUnit 4 helper in `log4j-1.2-api`) so they work on both Java 8 and Java 9+.
Head branch was pushed to by a user without write access
|
Updated — routed the affected readObject tests through SerialUtil / FilteredObjectInputStream so they pass the new assertFiltered check on Java 8. |
|
@SunWeb3Sec, see the failing CI results. Have you made sure |
The Java 8 surefire run (`java8-tests` profile) goes through FilteredObjectInputStream, whose default allow-list covers `org.apache.logging.log4j.` but not the `org.apache.log4j.` 1.2-compatibility namespace. LevelTest#testDeserializeINFO and #testCustomLevelSerialization therefore failed with "Class is not allowed for deserialization" on Java 8. Pass the two 1.2 classes the tests in this module deserialize (`org.apache.log4j.Level`, `LevelTest$CustomLevel`) as the `allowedExtraClasses` set so the hardened `Level#readObject` check still passes. Signed-off-by: SunWeb3Sec <infosecpt@gmail.com>
Head branch was pushed to by a user without write access
|
Fixed — the Java 8 surefire run uses FilteredObjectInputStream, whose default allow-list covers org.apache.logging.log4j. but not the org.apache.log4j. 1.2 namespace, so LevelTest#testDeserializeINFO and #testCustomLevelSerialization failed with InvalidObjectException. Passed org.apache.log4j.Level and LevelTest$CustomLevel as allowedExtraClasses in SerializationTestHelper. |
| // FilteredObjectInputStream's default allow-list covers `org.apache.logging.log4j.` but not | ||
| // the `org.apache.log4j.` 1.2-compatibility namespace, so we have to enumerate the | ||
| // 1.2 classes that the tests in this module deserialize on Java 8. | ||
| private static final Collection<String> ALLOWED_LOG4J_1_2_CLASSES = |
There was a problem hiding this comment.
Could you inline this to newObjectInputStream as a local variable, please? You can do if (ver == 8) { your-var-and-code-goes-here; } else { ... }. Keep the comments above the local var.
|
@SunWeb3Sec, tests are still failing. Would you mind updating the PR and ensuring that |
Move the `org.apache.log4j.` allow-list into `newObjectInputStream` so the constant lives in the only place that uses it, per review feedback. The explanatory comment now sits directly above the local variable inside the `JAVA_MAJOR_VERSION == 8` branch. No behavior change. Signed-off-by: SunWeb3Sec <infosecpt@gmail.com>
Fixes two regressions surfaced when running `./mvnw verify` locally: * `org.apache.logging.slf4j.Log4jLogger` lives outside the `org.apache.logging.log4j.` namespace covered by `FilteredObjectInputStream`'s default allow-list, so the Java 8 surefire run of `SerializeTest` rejected it. Extend `SerialUtil` and `SerializableMatchers` with overloads that accept an additional allow-list and pass `Log4jLogger` through from the test. * Adding `import org.apache.logging.log4j.util.internal.SerializationUtil` to log4j-core created a cross-bundle OSGi `Import-Package` requirement on an unexported package, breaking `log4j-osgi-test`. Mirror the `core.util.internal.instant` package and ship a `package-info.java` that exports `util.internal` with javadoc warning external users away.
Harden
ObjectArrayMessage.readObject()withSerializationUtil.assertFiltered()Summary
Adds a single
SerializationUtil.assertFiltered(in)call at the top ofObjectArrayMessage.readObject(), bringing it in line with the defensive pattern already used byObjectMessageandParameterizedMessage.Change
log4j-api/.../message/ObjectArrayMessage.javaBefore
After
Plus the corresponding
import org.apache.logging.log4j.util.internal.SerializationUtil;.What this PR deliberately does not do
ObjectArrayMessageinstances produced by older versions still deserialize into a patched version, and vice versa.LocalizedMessageorFormattedMessage. They have similar gaps, but the Log4j security team scoped this request toObjectArrayMessageonly. Happy to submit follow-up PRs for those if desired.LocalizedMessageTest#testSerialization*,FormattedMessageTest#testSerialization) that use plainObjectInputStream, because those classes are untouched.Tests
Added a minimal round-trip test (
ObjectArrayMessageTest#testSerializableRoundTripThroughFilteredStream) that serializes and deserializes throughSerialUtil, which usesFilteredObjectInputStreamon Java 8 — verifying the newassertFiltered()call accepts filtered streams.Behavioral note
On Java 8, deserializing an
ObjectArrayMessagethrough a plainObjectInputStream(no JEP 290 filter) now throwsIllegalArgumentExceptioninstead of silently proceeding. This matches the existing behavior ofObjectMessageandParameterizedMessage. Callers that relied on unfiltered deserialization ofObjectArrayMessageshould wrap their streams inFilteredObjectInputStream, matching the project's guidance for the sibling message types.On Java 9+, the behavior is unchanged —
assertFiltered()is a no-op when a JVM-levelObjectInputFilteris active, and a warning otherwise.Checklist
ObjectArrayMessage)src/changelog/.2.x.x/harden_message_deserialization.xml(typechanged)References
ObjectMessage#readObjectParameterizedMessage#readObjectThanks to the Apache security team for the clear triage.