fix: bound Jackson payload decompression size, unlimited by default (#3491, #3515) - #3523
Open
pjfanning wants to merge 2 commits into
Open
fix: bound Jackson payload decompression size, unlimited by default (#3491, #3515)#3523pjfanning wants to merge 2 commits into
pjfanning wants to merge 2 commits into
Conversation
Motivation: JacksonSerializer.decompress inflated gzip payloads with an unbounded transferTo, and passed the lz4 decompressed length declared on the wire straight to the decompressor as the allocation size. A small, well-formed message could therefore declare (or expand to) an arbitrarily large size and drive an OutOfMemoryError on deserialization. Modification: Add a `compression.max-decompressed-size` setting (default 256 MiB) to the jackson and jackson3 modules. On deserialization the gzip path copies through a bounded loop and the lz4 path rejects a declared length that is negative or over the cap, before allocating. Applies regardless of the `algorithm` setting, since decompression is chosen by the payload's magic bytes. Result: A payload that would decompress beyond the cap is rejected with an IllegalArgumentException instead of exhausting the heap. Tests: - sbt "serialization-jackson/testOnly *JacksonJsonSerializerSpec" "serialization-jackson3/testOnly *JacksonJsonSerializerSpec" - 71 passed each, incl. new gzip/lz4 cap tests - sbt "serialization-jackson/mimaReportBinaryIssues" - no issues (changed symbols are @InternalApi/private) - sbt scalafmt for changed main and test sources References: None - found while reviewing the draft threat model in apache#3478
) * fix: default the Jackson max-decompressed-size to unlimited Motivation: A bounded default could reject a payload an existing system legitimately exchanges, so a patch release carrying the 256 MiB default from apache#3491 could break running systems on upgrade. The bound should be opt-in, matching the change made to pekko.serialization.max-decompressed-size in apache#3502. Modification: Default pekko.serialization.jackson.compression.max-decompressed-size (and the jackson3 equivalent) to -1, meaning no limit and matching the behaviour of releases before apache#3491. A negative maximum skips the gzip size check and the LZ4 declared-size check; a negative declared LZ4 size is still rejected, since it is malformed regardless of the limit. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Jackson payload decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 122 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 120 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean - sbt "serialization-jackson/mimaReportBinaryIssues" - no issues References: Refs apache#3491, Refs apache#3502 * also accept "unlimited" for max-decompressed-size Motivation: Review on apache#3515 noted that Pekko is inconsistent about unlimited spellings and an explicit keyword is clearer than a magic number, while the neighbouring read.max-document-length and read.max-token-count settings use -1. Accept both. Modification: The setting reads "unlimited" or any negative number as no limit; the reference.conf default is written as `unlimited`. Applied to both serialization-jackson and serialization-jackson3, with a test each for the keyword. Result: `max-decompressed-size = unlimited` and `= -1` both disable the bound. Tests: - sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*" - 123 passed - sbt "serialization-jackson3/testOnly org.apache.pekko.serialization.jackson3.*" - 121 passed - sbt "serialization-jackson/scalafmtCheckAll" "serialization-jackson3/scalafmtCheckAll" - clean References: Refs apache#3515
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
1 participant
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
Backport of #3491 and its follow-up #3515 to 1.7.x:
unbounded gzip copy, and an LZ4 decompressor allocation sized from a wire-declared
length with no upper check) — a small, well-formed message could drive an
OutOfMemoryErroron deserialization.-1/unlimitedbothaccepted), so a patch release does not start rejecting a payload an existing system
legitimately exchanges. The bound stays available and opt-in.
Modification
Cherry-pick of fd19b01 (#3491) and 9d80429 (#3515, itself a squash of the
default-to-unlimited change and the later "also accept
unlimited" follow-up fromreview).
Scope note: both commits also touch
serialization-jackson3, which does not existas a module on 1.7.x (Jackson 3 support is new in the 2.x line — see the
"New configuration" section of the 2.x migration guide, PR2348). Those hunks were
dropped; only the
serialization-jackson(Jackson 2) changes are included here.2.12 adaptation: the merged
maxDecompressedSizeusedraw.toLongOption, whichScala 2.12 does not have. Rewritten with
toLongin atry/catch NumberFormatException,same shape as the fix already needed for the #3503 backport (#3517).
Result
pekko.serialization.jackson.compression.max-decompressed-sizedefaults tounlimited; bothunlimitedand a negative number such as-1disable the bound.256 MiB) bounds decompression exactly as in fix: bound Jackson payload decompression size #3491: anover-expanding gzip payload or an LZ4 payload declaring a length past the limit is
rejected with an
IllegalArgumentExceptionbefore the large allocation, instead ofrisking
OutOfMemoryError.Tests
sbt "++ 2.12.21 serialization-jackson/Test/compile"— clean, validating Scala 2.12sbt "serialization-jackson/testOnly org.apache.pekko.serialization.jackson.*"— 123 passed, 1 pendingsbt "serialization-jackson/scalafmtCheckAll"— cleanReferences
Backport of #3491 and #3515.