Devolutions / Devolutions/IronRDP

pdu: audit Vec preallocation paths for unbounded attacker-controlled inputs

Open
#1,315 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
3.2k
Forks
275
Avg merge
1d 11h
Merged PRs (30d)
189

Description

## Background

Several recent PRs landed retroactive caps on `Vec` preallocation paths in `crates/ironrdp-pdu/src/` where the size argument was attacker-controllable via wire bytes:

- **PR #1197** (Progressive RFX) — `SurfaceTiles::new` was allocating `Vec` capacity directly from decoded width/height u16 fields. Fix: cap at `MAX_SURFACE_DIM = 32768` per MS-RDPEGFX 2.2.2.14 normative ceiling.
- **PR #1271** (egfx PDU fuzz coverage) — `Avc420BitmapStream::decode` was allocating `Vec::with_capacity(num_regions as usize)` from a decoded u32. ASan flagged OOM. Fix: cap against remaining buffer size.
- **PR #1174** (ClearCodec) — per-pixel-count cap was insufficient against degenerate aspect ratios. Worst case observed: 197MB allocation at width=63961 height=771 within the original `8192*8192` cap. Fix: per-dimension cap (each axis ≤ 8192).

Each of these was found *retroactively* — either during PR review (fuzzer flagged it) or during a precheck-driven audit pass on the specific PR's diff. The shape is consistent enough across the three cases to suggest a systematic audit would surface more.

This issue tracks a proactive audit across `crates/ironrdp-pdu/src/` for the same pattern.

## Scope

`crates/ironrdp-pdu/src/` only for this issue. `ironrdp-egfx`, `ironrdp-graphics`, `ironrdp-bulk`, etc. can be audited under separate sibling issues if this one is productive.

## Approach

Walk every `Vec::with_capacity`, `Vec::reserve`, `Vec::reserve_exact`, `vec![value; n]`, and similar allocation calls in `crates/ironrdp-pdu/src/`. For each:

1. Identify the size argument's data flow.
2. Check whether it is attacker-controlled (decoded from wire bytes without intermediate validation).
3. If yes, evaluate whether the existing code path includes a bound:
- Spec-derived cap (preferred when the protocol defines a maximum, e.g., MS-RDPEGFX 2.2.2.14 for surface dimensions)
- Buffer-remaining-size cap (preferred when no spec bound exists, e.g., \"cannot exceed what is still readable from the cursor\")
- No bound (the bug)
4. File a fix PR for each unbounded site with the appropriate cap and a regression test.

## Method

The `pdu_decode` and `pdu_round_trip` fuzz oracles are the primary tooling — both target `Vec::with_capacity` paths and reach them within bounded smoke fuzzing for most PDU types. Memory-budget fuzzing (`cargo fuzz run -rss_limit_mb=128`) catches the OOM-class bugs; the alloc-cap-style oracle pattern raised in #1120 `issuecomment-4483012331` is the long-term shape for systematic detection but is blocked on maintainer signal.

For an interim approach, a grep-based first pass identifies candidate sites quickly; each candidate then gets evaluated by hand against the fuzz oracle's reach.

## Relationship to F13 architectural invariant

This audit is the direct follow-up to the F13 finding from the 2026-05-18 internal ARCHITECTURE.md audit: *\"Vec preallocation must be bounded against attacker-controlled inputs.\"* That invariant has been applied case-by-case as PRs hit the issue; the audit converts it from reactive to proactive.

## Success criteria

- Every `Vec::with_capacity` / `Vec::reserve` site in `crates/ironrdp-pdu/src/` whose size argument is attacker-controlled either has an explicit cap or is documented as safe with rationale.
- Each unbounded site found gets a fix PR with regression test (corpus file in `pdu_decode` or `pdu_round_trip` regression dir when applicable).
- Audit summary posted in a closing comment listing per-site evaluation.

## Out of scope

- Audits outside `ironrdp-pdu`. Codec subtrees in `ironrdp-egfx` (ZGFX) and `ironrdp-graphics` (RFX/NSCodec/RLE) have related concerns but warrant separate tracking (see #1120 `issuecomment-4483012331` for the codec memory-budget oracle shape question).
- Memory-DoS attacks via slow-allocation patterns (deallocate-free-list pathologies, alignment-fault paths). Different threat model.
- Heap fragmentation across long sessions. Different attack surface.

## Provenance

- PR #1197 (SurfaceTiles cap)
- PR #1271 (Avc420BitmapStream num_regions cap + the precheck-skill codec_pr_requirements heuristic that surfaced #1174's gap)
- PR #1174 (ClearCodec per-dimension cap)
- F13 architectural invariant from internal 2026-05-18 audit
- Lamco priority assessment 2026-05-25 (Tier 2 item D in our internal ironrdp-pdu state assessment)

Happy to drive this audit ourselves; filing as an issue so it has a public tracking surface and the maintainer can prescribe a different shape if preferred.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.