Skip to content

Add read limits to guard against OOM on untrusted .lottie files - #23

Open
theashraf wants to merge 13 commits into
mainfrom
feat/oom-limits
Open

theashraf wants to merge 13 commits into
mainfrom
feat/oom-limits

Conversation

@theashraf

Copy link
Copy Markdown
Member

Fixes #22.

Reading a .lottie trusted the ZIP-declared uncompressed size and read entries with no upper bound, so a crafted archive could force a huge up-front allocation or a decompression-bomb OOM.

Adds a Limits type (max_entry_bytes / max_total_bytes / max_entries, default 256 MiB / 1 GiB / 10k, plus Limits::unlimited()) threaded through from_bytes/from_file/open. Entry count is checked from the central directory before any decompression, each entry is read through a bounded reader that caps the initial allocation and errors past the cap, and the cumulative size is tracked across eager loads. Violations return LimitExceeded.

Breaking for the Rust API (new required limits arg). The JS bindings take an optional trailing limits object, so existing JS callers are unaffected.

Note: on the native-fs path the underlying streaming reader materializes an entry (under its own ~2 GiB cap) before we can reject it, so there maxEntryBytes bounds the accepted result rather than the transient peak. In-memory fromBytes enforces the cap strictly.

Test plan

  • cargo test -p dotlottie-io (default + --features native-fs)
  • cargo clippy -p dotlottie-io --all-targets clean
  • napi + wasm bindings build

@changeset-bot

changeset-bot Bot commented Jul 7, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 26bae39

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Whatsonyourmind

Copy link
Copy Markdown

Reviewed this against the same "parsing untrusted, machine-generated .lottie" use case from #22 — I've shipped the near-identical decompression-cap hardening upstream elsewhere (gtfs-analyzer, a Rust PPDB reader in getsentry/symbolic), so a few notes from that experience. Overall this is a clean, correct design — the read_bounded idiom in particular is exactly right: capping the initial reserve at INITIAL_RESERVE instead of the declared size kills the up-front-allocation vector, and .take(max_entry_bytes.saturating_add(1)) bounds the actual inflate incrementally rather than trusting the header (with the saturating_add handling the unlimited() edge). Checking max_entries from the central directory before any decompression is the right order, too. Three things worth considering:

1. The native-fs residual gap — can it be closed with the same read_bounded? You've documented it honestly (the declared-size pre-check is the guard; s-zip materializes under its own ~2 GiB cap before the post-check). Worth spelling out which attack each path stops: the declared-size pre-check catches a classic bomb (small compressed / honestly-declared-huge uncompressed) even on native-fs, since the header itself trips the cap. The residual window is the narrower lying-header case — declared ≤ cap but the deflate stream actually expands past it — where the in-memory path trips mid-inflate via take() but the native-fs path can transiently materialize first. If the native-fs streaming reader exposes a per-entry Read handle (the way the in-memory by_name(...) entry does), routing it through the same read_bounded(&mut entry, …) would close the gap and unify the two paths on one guard. If s-zip only hands you a read_entry_by_name → Vec on the fs path, then the documented limitation is a reasonable known-tradeoff — but it'd be worth a one-line debug_assert/comment that the two paths have different peak-memory guarantees so it doesn't silently regress. Happy to check the s-zip fs API if useful.

2. No compression-ratio guard — I think that's the right call here, and worth keeping. #22 floated a ratio heuristic; you (correctly, IMO) went with absolute caps only. .lottie payloads are JSON manifests + Lottie JSON + occasionally base64/text assets — all legitimately high-ratio, often 20–50×. A naive uncompressed/compressed guard false-positives on perfectly valid files (I got bitten by exactly this on a transit feed where stop_times.txt compresses ~30×). Absolute per-entry + total + count caps are the honest, FP-free control for this payload profile. If a ratio guard is ever added later, it needs a RATIO_FLOOR / minimum-absolute-size exemption so small highly-compressible entries can't trip it — but I wouldn't add one now.

3. Defaults vs. the wasm target. The reporter calls out browser/WASM as the untrusted path, and that's where the 256 MiB entry / 1 GiB total defaults are least conservative — a single ~1 GiB allocation frequently fails on wasm32 (and on mobile browsers, well before that), so the process can still OOM inside the "allowed" envelope. Since the limits are already configurable, this may just be a docs note ("on wasm, tighten maxTotalBytes to your memory budget"), or a lower default when compiled to wasm32. Not a correctness issue — just that the default envelope is sized for native.

If helpful I can contribute a crafted-fixture test for the lying-header case (declared-small / inflates-past-cap) to pin the in-memory strict-bound behavior and document the native-fs difference — that's the case most likely to regress quietly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reader trusts ZIP-declared entry size / no decompression limits — OOM risk on untrusted .lottie

2 participants