Repository navigation
Decoding size limits for DoS protection #64
Closed
SeanTAllen
started this conversation in
Research
Replies: 3 comments
|
I prefer option B with conservative defaults. And for sizing violations, I think starting with the primitive makes the most sense. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
The problem
The decoder has no limits on the size of data it will attempt to allocate when decoding variable-length values. An attacker who controls the input can craft a msgpack message with a str_32 header declaring 4 GB of string data, a map_32 header declaring 2^32 key-value pairs, or similar. The library will either attempt to allocate the declared size (for str/bin/ext) or return a count that naive callers will use to size a collection (for array/map).
This is not theoretical. msgpack-python added size limits as a security improvement after recognizing the risk, and now ships with conservative defaults (1 MB for strings, binary, and ext; 131,072 for arrays and maps). msgpack-javascript, msgpack-cxx, rmp-serde (Rust), and MessagePack-CSharp all provide configurable limits. The Pony library currently has none.
Specific attack vectors
Memory exhaustion via str/bin/ext: A str_32, bin_32, or ext_32 header can declare up to 2^32 - 1 bytes (4 GB) of payload. The decoder calls
Reader.block(len)to extract that many bytes from the reader's internal buffer. If the attacker sends a large payload and the application buffers it (viaappend()on the streaming decoder, or by passing a pre-filledReaderto the low-level decoder), the decode proceeds and returns the full payload.MessagePackStreamingDecoder's existing size check only protects against reader corruption from partial reads — it confirms the data is present before consuming, but does nothing to prevent processing an oversized value.Collection pre-allocation via array/map counts: The library returns array and map element counts to the caller. A map_32 header can claim 2^32 pairs. Callers who pre-allocate a
MaporArraysized to this count will attempt a multi-billion-element allocation. The library doesn't allocate the collection itself, but it returns the unchecked count that drives the allocation.Nested container amplification: A sequence of array headers each claiming large counts creates multiplicative depth. While the library doesn't track nesting, callers building recursive decoders on top of it are vulnerable to stack overflow or exponential work.
How other libraries handle this
msgpack-python
The
Unpackerclass accepts per-type limits as constructor arguments:max_str_len(default: 1,048,576 — 1 MB)max_bin_len(default: 1,048,576)max_array_len(default: 131,072)max_map_len(default: 131,072)max_ext_len(default: 1,048,576)max_buffer_size(default: 100 MB)When a declared size exceeds the limit, the unpacker raises
ValueError. Older versions had no limits and amax_buffer_sizeof 0 (unlimited); the security-conscious defaults were added in a later release.msgpack-javascript (@msgpack/msgpack)
The
Decoderconstructor accepts an options object with:maxStrLength(default:4_294_967_295— effectively unlimited)maxBinLength,maxArrayLength,maxMapLength,maxExtLength(same default)Exceeding a limit throws
DecodeError. The defaults are permissive; security-conscious callers must opt in.msgpack-cxx
The
unpack()function accepts anunpack_limitobject witharray,map,str,bin,ext, anddepthfields. All default to0xFFFFFFFFexcept depth (512). Exceeding a limit throwsmsgpack::array_size_overflowor similar typed exceptions.rmp-serde (Rust)
The deserializer supports a depth limit via
Deserializer::set_max_depth(). Exceeding it returnsDepthLimitExceeded. Per-type size limits are not provided at the rmp layer; Rust's serde ecosystem typically handles this at the type level.Design
Which APIs get limits
MessagePackStreamingDecoderis the natural home. It's already stateful (a class, not a primitive), it's the recommended API for untrusted data (per the issue #14 warning), and it already peeks at declared sizes before consuming data — the limit check slots in between the peek and the consume.MessagePackDecoderis a stateless primitive. Adding limits would require either method parameters (cluttering every call site) or wrapping it in a stateful class (which is what the streaming decoder already is). The recommendation should be: for untrusted data, useMessagePackStreamingDecoder. The low-levelMessagePackDecoderis for trusted data or callers who've already validated sizes externally.Limits configuration
Two options:
Option A: Constructor parameters
Simple, direct, minimal API surface. Adding a new limit later changes the constructor signature but doesn't break existing callers (new parameters get defaults).
Option B: Separate configuration class
More extensible — the limits object is reusable, the
unlimited()factory communicates intent, and future limits (depth, buffer size) can be added to the class without changingMessagePackStreamingDecoder's constructor. If limits are later added toMessagePackDecoder(via a wrapper), the same type can be shared.Default values
Two schools of thought, represented by the libraries surveyed:
Permissive defaults (msgpack-javascript, msgpack-cxx): All limits set to max values, effectively off. No behavioral change for existing users. Secure callers must opt in. This is the "no surprises" approach — existing code keeps working identically.
Conservative defaults (msgpack-python): Reasonable limits like 1 MB for payloads, 131K for container counts. Security by default. Existing code that legitimately decodes large values would need to raise limits explicitly.
For a beta library, permissive defaults avoid breaking existing users while making the capability available. The documentation and README should prominently recommend setting limits for untrusted data.
Result type for limit violations
The streaming decoder currently returns
DecodeResult, which is(MessagePackValue | NotEnoughData | InvalidData). A limit violation is semantically distinct from both existing error cases:NotEnoughData— the data may be fully present.InvalidData— the data is valid msgpack; it just exceeds policy.The caller's response is also distinct: log the violation and close the connection, rather than "wait for more data" or "stream is corrupt."
Option A: New
LimitExceededprimitiveMinimal, consistent with
NotEnoughDataandInvalidDatabeing primitives. The caller knows a limit was hit but not which one. Matches the pattern established by the existing result variants.Option B:
LimitExceededclass with detailsRicher — the caller gets a diagnostic message without guessing. The result type has the same shape from the pattern-matching perspective (match on
LimitExceeded), but the value carries context. This follows the precedent ofMessagePackArray,MessagePackMap,MessagePackExt, andMessagePackTimestampbeing classes in the result union rather than primitives.Where the checks go
In
MessagePackStreamingDecoder, the limit check slots into each variable-length decode method between the size peek and the delegation toMessagePackDecoder:Methods affected:
_decode_fixstrmax_str_len_decode_strmax_str_len_decode_binmax_bin_len_decode_fixextmax_ext_len(always passes — max 16 bytes)_decode_ext_variablemax_ext_len_decode_fixarraymax_array_len_decode_arraymax_array_len_decode_fixmapmax_map_len_decode_mapmax_map_lenThe fixext methods (fixext_1 through fixext_16) have fixed data sizes (1, 2, 4, 8, or 16 bytes). Checking them against
max_ext_lenis technically correct but will never trigger for any reasonable limit. Including the check anyway provides defense-in-depth._decode_timestampis not listed because it is always reached via the_decode_fixextor_decode_ext_variablepaths, which checkmax_ext_lenbefore the timestamp guard runs. The timestamp data sizes are fixed and small (4, 8, or 12 bytes), so no separate limit is needed.Byte consumption on limit violation
When a limit is exceeded, the decoder must NOT consume the bytes. This is consistent with
NotEnoughDatabehavior — the reader remains at the same position. The caller can't meaningfully retry (unlikeNotEnoughData), but not consuming keeps the reader state clean for diagnostics or for callers who want to skip the offending value with a futureskip()method.Depth limits
Container depth tracking is a related but separate concern. The streaming decoder currently has no nesting awareness — it returns array/map headers and the caller manages nesting. Adding a depth limit would require the decoder to track an internal nesting counter, incremented when returning
MessagePackArray/MessagePackMapand decremented... when? The decoder doesn't know when the caller has finished reading a container's elements.Two approaches for depth limits:
begin_container()/end_container()protocol methods that the caller must call, allowing the decoder to track depth. This changes the usage pattern significantly.Depth limits are not addressed in this proposal. They're worth considering separately once the basic size limits are in place.
Scope
This proposal covers:
MessagePackStreamingDecoderThis proposal does not cover:
MessagePackDecoder(low-level API — use streaming decoder for untrusted data)append())All reactions