harden(rsky-video): bound transcode CPU, memory, disk and runtime - #211
Open
afbase wants to merge 2 commits into
Open
harden(rsky-video): bound transcode CPU, memory, disk and runtime#211afbase wants to merge 2 commits into
afbase wants to merge 2 commits into
Conversation
app.bsky.embed.video accepts only mimeType video/mp4, but iPhone camera
captures and screen recordings ship H.264/AAC inside a QuickTime container.
upload_video sent those bytes to the PDS verbatim with a hardcoded
Content-Type: video/mp4 header -- and that header is a no-op, because every
spec-compliant PDS tags blobs by sniffing the bytes:
- TypeScript PDS: `sniffedMime || userSuggestedMime`
(packages/pds/src/actor-store/blob/transactor.ts)
- rsky-pds: `sniffed_mime.unwrap_or(user_suggested_mime)`
(rsky-pds/src/actor_store/blob/mod.rs)
So the blob came back tagged video/quicktime, rsky-video handed that ref to
the client as-is, and the client's applyWrites failed record validation with
a 400. Every video shot on an iPhone failed to post, on web and mobile; MP4
uploads passed coincidentally because their bytes already sniff as video/mp4.
Detect QuickTime on upload and remux to MP4 with `-c copy -movflags
faststart` -- stream copy, no re-encode, so it is lossless and costs only
the time to copy the bytes once. The conversion runs before both the PDS
upload and the Bunny transcode, so Bunny receives the MP4 too and streaming
is unaffected. MOVs carrying codecs MP4 cannot hold (ProRes) fail the remux
with ffmpeg's error rather than storing a blob that cannot be embedded.
The detector mirrors what the PDS sniffers themselves treat as QuickTime,
since anything they tag video/quicktime fails validation: an `ftyp` box with
the `qt ` brand, or a leading moov/mdat/free/wide box for older MOVs with no
ftyp at all. Verified against file-type 16.x and the infer crate. ISO BMFF
brands (isom, mp42, ...) already sniff as video/mp4 and are left alone.
A MOV remux was written for this in May (4e889b3, branch
fix/rsky-video-mov-to-mp4) but never merged; the July GIF-transcode PR
(d8a4e0d) then created transcode.rs fresh with a GIF path only, so main has
never had QuickTime handling. This re-applies the fix in main's style and
factors the shared tempfile/ffmpeg plumbing out of gif_to_mp4 -- the GIF
argv is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Transcoding is the one place this service hands an attacker-supplied file to
a CPU- and memory-hungry subprocess, and until now that subprocess ran with
no ceiling of any kind: no deadline, no output cap, no thread cap, and no
limit on how many could run at once. A single crafted upload could pin the
box, and N concurrent uploads meant N concurrent ffmpeg processes.
Every conversion now runs under transcode::Limits:
- Wall-clock deadline (TRANSCODE_TIMEOUT_SECS, default 120). A conversion
that outlives it is SIGKILLed and reaped.
- Global concurrency limit (TRANSCODE_MAX_CONCURRENT, default half the
cores). This is what bounds total transcode CPU, memory and temp disk.
Uploads wait up to TRANSCODE_QUEUE_TIMEOUT_SECS (30) for a slot and are
then turned away with 429 rather than piling up unbounded waiters.
- Output ceiling (TRANSCODE_MAX_OUTPUT_BYTES, default 2x MAX_VIDEO_SIZE),
guarding against a small input that decodes into an enormous output.
- Thread cap per process (TRANSCODE_THREADS, default 2), so one conversion
cannot saturate every core.
ffmpeg's own -fs flag is deliberately not used: it is silently ignored both
for stream-copy remuxes and for re-encodes (verified against ffmpeg 8.1.2 --
a 50000-byte cap let 158022 bytes through). The ceiling is instead enforced
by watching the output file and killing the process, which does not depend
on ffmpeg honoring anything.
Also hardened, all in the same subprocess path:
- kill_on_drop(true). tokio does not kill a child when its future is
dropped, so before this a client that disconnected mid-upload left ffmpeg
running; repeated connect/disconnect was unbounded free CPU.
- The demuxer is pinned with -f to the format the magic bytes already
implied, plus -protocol_whitelist file, so a crafted input cannot steer
ffmpeg into a playlist-style demuxer that opens other local paths or
URLs.
- -loglevel error/-nostats keeps stderr small enough that ffmpeg can never
block on a full pipe while supervised, and stderr capture is itself
bounded at 64KB.
Two fixes outside transcode.rs:
- DefaultBodyLimit was 5GB while upload_video takes the body as `Bytes`,
which axum buffers entirely in memory before the handler's own
MAX_VIDEO_SIZE check can run -- so the 100MB limit did nothing to stop a
5GB allocation. The layer now uses max_video_size, refusing oversized
uploads with 413 while streaming.
- axum::serve had no graceful shutdown, so a redeploy's SIGTERM killed
in-flight uploads after their job rows were created, leaving jobs stuck.
It now drains on SIGTERM/SIGINT.
Tests: queue saturation returns 429 without spawning ffmpeg; the deadline
kills a slow transcode and releases its permit; the output ceiling rejects
oversized output; and the GIF and MOV paths still convert correctly with the
pinned demuxer and whitelist in place. The kill was also verified out of
band -- a 720p encode interrupted at 1ms leaves no surviving ffmpeg process,
checked while the same encode was measured at 1.72s of real work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Summary
Transcoding is the one place this service hands an attacker-supplied file to a CPU- and memory-hungry subprocess, and until now that subprocess ran with no ceiling of any kind: no deadline, no output cap, no thread cap, and no limit on how many could run at once. A single crafted upload could pin the box, and N concurrent uploads meant N concurrent
ffmpegprocesses competing for CPU, RAM and temp disk.Ceilings
Every conversion now runs under
transcode::Limits, all env-overridable:TRANSCODE_TIMEOUT_SECSTRANSCODE_MAX_CONCURRENTTRANSCODE_QUEUE_TIMEOUT_SECSTRANSCODE_MAX_OUTPUT_BYTESMAX_VIDEO_SIZETRANSCODE_THREADSWhy not ffmpeg's
-fs?Because it does not work.
-fsis silently ignored both for stream-copy remuxes and for re-encodes — verified against ffmpeg 8.1.2:Shipping
-fswould have been a cap that does nothing. The ceiling is instead enforced by watching the output file and killing the process, which doesn't depend on ffmpeg honoring anything.Other subprocess hardening
kill_on_drop(true)— tokio does not kill a child when its future is dropped. Before this, a client that disconnected mid-upload leftffmpegrunning to completion; repeated connect/disconnect was unbounded free CPU with no request left to attribute it to.-f <format>pins ffmpeg to the format the magic bytes already implied, and-protocol_whitelist filekeeps a crafted input from steering ffmpeg into a playlist-style demuxer that opens other local paths or URLs.-loglevel error -nostatskeeps stderr small enough that ffmpeg can never block writing to a full pipe while we supervise it, and the capture itself is capped at 64KB. (It also makes failure messages more useful: real error lines instead of progress spam.)Two fixes outside
transcode.rsDefaultBodyLimitwas 5GB.upload_videotakes the body asBytes, which axum buffers entirely in memory before the handler runs — so the handler's ownMAX_VIDEO_SIZEcheck (100MB) happened after the allocation and did nothing to prevent it. One request could force a multi-GB allocation; a few concurrently would OOM the host. The layer now usesmax_video_size, so oversized uploads are refused with 413 while streaming.No graceful shutdown.
axum::servehad none, so a redeploy's SIGTERM killed in-flight uploads after their job rows were created, leaving jobs stuck. It now drains on SIGTERM/SIGINT — relevant given a redeploy is imminent.Verification
cargo test -p rsky-video: 11 passed. With-- --ignored: 4 passed. Clippy: no new warnings.cargo fmt: clean.saturated_queue_is_rejected_with_429— turns uploads away instead of queueing forever, without spawning ffmpeg at all.deadline_kills_a_slow_transcode— deadline fires and the permit is released, so one slow upload can't wedge a slot permanently.output_ceiling_rejects_oversized_output—VideoTooLargerather than an unbounded write.gif_converts_to_an_mp4/mov_remux_yields_an_mp4_brand— both real paths still convert correctly with the pinned demuxer and whitelist in place.The kill was also verified out of band, since a TTL that doesn't actually kill is worth nothing: a 720p encode interrupted at 1ms leaves no surviving ffmpeg process, checked while that same encode was independently measured at 1.72s real / 3.70s user of work — so an unkilled process would still have been plainly visible.
Note
max_video_duration(90s) is config'd but enforced nowhere in the codebase — it predates this PR. I left it alone deliberately: the natural fix is ffmpeg-t, but that would silently truncate a user's video rather than reject it, which is a product decision rather than a hardening one. Worth a follow-up.Related Issues
Follow-up to #210, which introduced the MOV remux path and made ffmpeg load user-controlled.
Changes
Checklist
config.rs, matching how the existing ones are — this crate has no README.)🤖 Generated with Claude Code