Skip to content

fix: resolve, check and parse manifests more carefully in the Jackson serializers - #3503

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:jackson-fixes
Open

fix: resolve, check and parse manifests more carefully in the Jackson serializers#3503
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:jackson-fixes

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Motivation

Four issues in JacksonSerializer, all reachable from the manifest string on the wire.

The case object branch resolved the class in a way that initializes it, before checking
the allow list.
When the manifest ends in $, fromBinary called
system.dynamicAccess.getObjectFor(className), which reads the MODULE$ field through a
VarHandle and so runs the class's initializer; checkAllowedClass ran only afterwards.
The only gate before that point is checkAllowedClassName, which consults Jackson's gadget
deny list rather than serialization-bindings or allowed-class-prefix. So a manifest
naming any Scala object on the classpath ran that object's body on the way to being
rejected. The sibling branch already does the right thing — getClassFor is
Class.forName(fqcn, false, cl) and does not initialize. Same shape as #3495.

LZ4Meta.get checked for four remaining bytes and then read eight — the magic plus the
declared length. A 4-to-7 byte payload beginning with 0x87D96DF6 raised
BufferUnderflowException out of decompress.

parseManifest called toInt on whatever followed the last #, so Foo#abc or a
trailing # raised NumberFormatException rather than a serialization error.

isInAllowList evaluated the throwing operand first. isBoundToJacksonSerializer calls
serializerFor, which raises NotSerializableException — stack trace and all — for a class
that is not bound, and that is exactly the case for a class allowed only by
allowed-class-prefix. checkAllowedClass runs on every fromBinary, so this built and
discarded one exception per message on that path.

Modification

  • Resolve the case object's class with getClassFor, run checkAllowedClass on it, and read
    the module field only after that.
  • Require 8 remaining bytes before reading an LZ4 header.
  • Parse the manifest version with toIntOption and report a non-numeric one as
    NotSerializableException naming the manifest.
  • Test hasAllowedClassPrefix before isBoundToJacksonSerializer.

Applied identically to serialization-jackson and serialization-jackson3.

The reordering in isInAllowList is the only change with no behaviour difference — both
operands are pure predicates and isBoundToJacksonSerializer already swallows its exception —
so it is covered by the existing allowed-class-prefix tests rather than a new one.

Result

A rejected manifest no longer initializes the class it names; two malformed manifests are
reported as serialization failures instead of unrelated runtime exceptions; the allow list
check no longer constructs an exception per message on the prefix path. No change for
manifests that were accepted before.

Tests

New tests in JacksonSerializerSpec in both modules (so each runs under both the JSON and
CBOR serializers):

  • not allow deserialization of a case object that is not in serialization-bindings — the
    allow list still rejects it
  • not initialize a case object class it goes on to reject — a test-only object whose body
    sets a flag; the flag must still be false after the rejection
  • reject a manifest whose version is not a number
  • not underflow on a payload that is only as long as the LZ4 magic

I checked these discriminate by reverting the production change and re-running: the last
three fail without it (true did not equal false, NumberFormatException was thrown, and
the underflow respectively). The first passes either way — it documents that the allow list
decision itself is unchanged, which is the point.

  • sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" — 128 passed
  • sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" — 126 passed
  • sbt "serialization-jackson/mimaReportBinaryIssues" — no issues (serialization-jackson3
    disables MimaPlugin)
  • sbt scalafmtAll headerCreateAll — no changes

References

The case object issue is the same shape as #3495, which stopped the serializer resolving a
wire-supplied manifest class it would not use.

ProtobufSerializer.isInAllowList has the same throwing-operand-first ordering, but it
caches its method handle after the check so it pays the cost once per class rather than once
per message; it is in pekko-remote and is left for a separate change.

… serializers

Motivation:
Four issues in JacksonSerializer, all reachable from a wire manifest:

- The case object branch called getObjectFor before checkAllowedClass.
  getObjectFor reads the MODULE$ field, which initializes the class, so a
  manifest naming a class the allow list would reject ran that class's
  initializer on the way to being rejected.
- LZ4Meta.get checked for 4 remaining bytes and then read 8, so a 4 to 7
  byte payload beginning with the LZ4 magic raised BufferUnderflowException.
- parseManifest called toInt on whatever followed the last '#', so a
  non-numeric version raised NumberFormatException.
- isInAllowList evaluated isBoundToJacksonSerializer first, which calls
  serializerFor and raises (filling in a stack trace) for a class that is
  allowed only by prefix. checkAllowedClass runs on every fromBinary.

Modification:
Resolve the case object's class with getClassFor, which does not initialize,
run checkAllowedClass on it, and only then read the module field. Require 8
remaining bytes for an LZ4 header. Parse the manifest version with
toIntOption and report a bad one as NotSerializableException. Test the
prefix before the binding, which cannot throw. Applied to both
serialization-jackson and serialization-jackson3.

Result:
A rejected manifest no longer initializes the class it names, two malformed
manifests are reported as serialization failures rather than as unrelated
runtime exceptions, and the allow list check no longer builds an exception
per message on the prefix path. No change for manifests that were accepted
before.
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.

1 participant