Bound nesting depth of deserialized payloads - #2
Draft
claude-code-staging[bot] wants to merge 1 commit into
Draft
claude-code-staging[bot] wants to merge 1 commit into
claude-code-staging[bot] wants to merge 1 commit into
Conversation
Motivation: Serializers for wrapper messages deserialize the wrapped payload by re-entering the serialization infrastructure, and the nesting depth of such payloads is decided by the sender of the message. Nothing tracked the cumulative depth, so a single bounded message that nests payloads deep enough made deserialization recurse until the thread stack was exhausted. The resulting StackOverflowError is not NonFatal, so it escaped the deserialization stage instead of only failing that message. QuerySerializer is an example that is reachable from the remoting inbound port: the event and metadata of a QueryMessages.EventEnvelope are opaque Payloads, so each one can name serializer id 39 and manifest "a" again, and each nesting level costs only a few tens of bytes because the protobuf nested message limit does not apply to bytes fields. The same shape exists in the other wrapper serializers. Modification: Track the nested deserialization depth of the current thread in Serialization and abort with a NotSerializableException above pekko.serialization.max-nested-deserialization-depth, which defaults to 100. The counter is incremented in deserializeByteArray, in the ByteBufferSerializer branch of deserializeByteBuffer, in deserialize(bytes, clazz) and in the two branches of WrappedPayloadSupport.deserializePayload that call the payload serializer directly, so every re-entry is counted exactly once. Result: Deserializing one message recurses at most max-nested-deserialization- depth levels no matter how deeply the wire payload nests. Over-limit input fails with a NotSerializableException, which is NonFatal and is handled like any other deserialization failure, so the message is dropped and the inbound stream stays up. Tests: - Not run - sbt, scalafmt and a Scala toolchain are not installed in this environment and it has no network access to a Maven repository, so `sbt "actor-tests / Test / testOnly org.apache.pekko.serialization.NestedDeserializationDepthSpec"`, `sbt "persistence-query / Test / testOnly org.apache.pekko.persistence.query.internal.QuerySerializerSpec"`, `sbt +mimaReportBinaryIssues`, `sbt scalafmtAll` and `sbt headerCreateAll` were all skipped. No new files were added, so no license headers are needed. Changes are additive, so binary compatibility is preserved. - Added NestedDeserializationDepthSpec, covering the array and the ByteBuffer entry points at and above the limit, and that a rejected message leaves the counter clean for the next one. - Added QuerySerializerSpec cases for a nested EventEnvelope within the limit and for one nested well beyond it. - Added the default of the new setting to ConfigSpec. References: None - hardening of the deserialization paths that re-enter on attacker-controlled nested payloads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Serializers for wrapper messages deserialize the wrapped payload by re-entering the serialization infrastructure, and the nesting depth of such payloads is decided by the sender of the message. Nothing tracked the cumulative depth, so a single bounded message that nests payloads deep enough made deserialization recurse until the thread stack was exhausted. The resulting
StackOverflowErroris aVirtualMachineError, so it is not caught by theNonFatalhandlers around deserialization and escaped the deserialization stage instead of only failing that message.QuerySerializeris an example that is reachable from the remoting inbound port. It is registered as serializer id 39 wheneverpekko-persistence-queryis on the classpath, and theeventandmetadataof aQueryMessages.EventEnvelopeare opaqueContainerFormats.Payloads, so each one can name serializer id 39 and manifest"a"again:fromBinary->WrappedPayloadSupport.deserializePayload->fromBinary-> ...Protobuf's own nested-message recursion limit does not bound this, because each level lives in the
bytes enclosedMessagefield and is only parsed by the nextfromBinarycall. A level costs only a few tens of bytes, so thousands of levels fit inside the frame limits. The offset route (serialization.deserializefromfromStorageRepresentation) has the same shape, as do the other wrapper serializers (MiscMessageSerializer,StreamRefSerializer,ShardingSerializer,ReliableDeliverySerializer, distributed-data'sSerializationSupport, ...).Modification
Track the nested deserialization depth of the current thread in
Serializationand abort with aNotSerializableExceptionwhen it would exceedpekko.serialization.max-nested-deserialization-depth, which defaults to 100.The counter is incremented at the points where deserialization can be re-entered, once per nesting level:
Serialization.deserializeByteArray, which coversdeserialize(bytes, serializerId, manifest)and the array fallback ofdeserializeByteBufferByteBufferSerializerbranch ofSerialization.deserializeByteBufferSerialization.deserialize(bytes, clazz)WrappedPayloadSupport.deserializePayloadthat invoke the payload serializer directly, bypassingSerialization; its third branch goes throughSerialization.deserializeand is already countedThe counter is a thread local, deliberately shared by all
ActorSystems of the JVM, because the resource it protects, the thread stack, is shared too. The guard method is kept small so it stays inlinable on the artery hot path.Result
Deserializing one message recurses at most
max-nested-deserialization-depthlevels, no matter how deeply the wire payload nests envelopes or payloads. Over-limit input fails with aNotSerializableException, which isNonFataland is treated like any other deserialization failure, so artery (Codecs.Deserializer) and classic remoting (Endpoint) log it and drop the message, and the inbound lane stays up.The default is far above the nesting depth that Pekko itself produces, so legitimate traffic is unaffected.
Tests
sbt,scalafmtand a Scala toolchain are not installed in this environment and it has no network access to a Maven repository.sbt "actor-tests / Test / testOnly org.apache.pekko.serialization.NestedDeserializationDepthSpec",sbt "persistence-query / Test / testOnly org.apache.pekko.persistence.query.internal.QuerySerializerSpec",sbt "actor-tests / Test / testOnly org.apache.pekko.config.ConfigSpec",sbt +mimaReportBinaryIssues,sbt scalafmtAllandsbt headerCreateAllwere all skipped and need to be run in CI. No new files were added, so no license headers are needed, and the change only adds members, so binary compatibility is preserved.NestedDeserializationDepthSpecinactor-tests: a serializer that unwraps one nesting level per call, exercised at the limit and one above it, through bothdeserialize(bytes, id, manifest)anddeserializeByteBuffer, plus a case asserting that a rejected message leaves the thread's counter clean for the next one.QuerySerializerSpeccases that build a nestedQueryMessages.EventEnvelopeby hand: one within the limit that must deserialize to the expected nesting, and one nested well beyond it that must fail withNotSerializableException. Before this change the second one overflows the stack or returns a deeply nested envelope.ConfigSpec.References
None - hardening of the deserialization paths that re-enter on attacker-controlled nested payloads.
Generated by Claude Code