3Hren / 3Hren/msgpack-rust

Depth-limit guard from #277 doesn't cover deserialize_option, deserialize_newtype_struct, or map-shaped enum newtype variants — reachable crash via attacker-controlled recursion

Open
#381 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.4k
Forks
162
PR merge metrics
No merged PRs in 30d

Description

#276/#52 (fixed by #277) added a `depth_count!` guard to `Deserializer`, but it only wraps three
call sites: `visit_seq`, `visit_map`, and the `Ext`-marker `visit_newtype_struct` branch inside
`deserialize_any`. Three other recursion-capable forwarding points were never covered and bypass
the guard (and `set_max_depth`) entirely:

1. `deserialize_option`'s `visitor.visit_some(self)`
2. `deserialize_newtype_struct`'s `visitor.visit_newtype_struct(self)` (both the general branch
and the `MSGPACK_EXT_STRUCT_NAME` branch)
3. `VariantAccess::newtype_variant_seed`'s `seed.deserialize(self.de)` (the map-as-single-variant
enum path)

For a type shaped as `Option>` or `Wrap(Box)` (a newtype struct wrapping itself),
a stack overflow (process abort) is reachable with essentially no recursion-driving input —
`Option>` overflows on a single non-nil byte; the newtype-struct case overflows on an
**empty buffer**. The enum-variant path does need attacker-controlled input length (a chain of
`{"Node": ...}` maps), but that's still just a few hundred bytes at the default depth of 1024.

`set_max_depth` gives no protection against any of these three — the guard is never consulted on
these paths, so a caller who explicitly configured a lower limit still crashes.

This is a crash-only bug — no memory corruption, no data exposure, just a reachable panic/abort
via `rmp_serde::from_slice`/`Deserializer` on attacker-supplied bytes. Worth fixing regardless of
where you'd land it on a severity scale; flagging that framing up front rather than dressing it up.

### Reproduction (rmp-serde 1.3.1, and current master @ cf88001)

```rust
use serde::Deserialize;

#[derive(Deserialize)]
struct ONode(Option>);

fn main() {
// A single byte -- 0x00 is a fixint 0, NOT the 0xc0 nil marker -- drives unbounded
// recursion for ONode's Option> shape. Aborts with a stack overflow.
let _: Result = rmp_serde::from_slice(&[0x00]);
}
```

```rust
use serde::Deserialize;

#[derive(Deserialize)]
struct Wrap(Box);

fn main() {
// Empty buffer. deserialize_newtype_struct never reads a byte before recursing.
let _: Result = rmp_serde::from_slice(&[]);
}
```

```rust
use serde::Deserialize;

#[derive(Deserialize)]
enum EList { Leaf, Node(Box) }

fn main() {
let mut data = Vec::new();
for _ in 0..200_000 {
data.extend_from_slice(&[0x81, 0xa4, b'N', b'o', b'd', b'e']); // {"Node": ...}
}
data.extend_from_slice(&[0xa4, b'L', b'e', b'a', b'f']);
let _: Result = rmp_serde::from_slice(&data);
}
```

### Fix

PR incoming: extends `depth_count!` to all three sites, with an explicit reborrow (`&mut *self`)
where the callee is generic over `D: Deserializer<'de>` — Rust doesn't auto-reborrow through a
generic parameter the way it does for a concretely-typed `&mut T` parameter, so a bare
`self`/`self.de` would move rather than reborrow there and fail to compile.

---
Discovered by the [rust-in-peace](https://github.com/scadastrangelove/rust-in-peace/) security pipeline.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at Deserializer's existing depth_count! wrappers and inspect deserialize_option, deserialize_newtype_struct, and VariantAccess::newtype_variant_seed. Run the supplied recursive Option, newtype-struct, and map-shaped enum reproductions; done means all three paths honor set_max_depth without reaching stack overflow.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.