Attacker-controlled images reach Pillow with no limits or isolation #65
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: MEDIUM
The bug
backend/app/services/exif_extractor.py:43-47callsImage.open(path)andimg._getexif()in-process, in the request handler, on fully attacker-controlled bytes — with no pixel limit,
no
verify(), no timeout, no memory cap, and no isolation.MAX_IMAGE_PIXELSemits a warning, not anexception, and the app never configures it or catches it. A small crafted file can expand to
enormous memory.
Image.openis lazy, but_getexif()triggers work on some formats, and anyfuture thumbnailing decodes fully.
API process, which holds the session-signing key and the database handle.
The bare
except Exceptionat line 46 catches Python exceptions but does nothing for a segfaultor an OOM kill — and it silently swallows genuine corruption, making a photo whose EXIF failed
to parse indistinguishable from one that has none.
Note the DoS is pre-validation:
_validate_uploadonly inspects a header, so bomb bytes reachPillow regardless.
Fix
Image.MAX_IMAGE_PIXELSdeliberately and turn the warning into an error.Image.open(...).verify()before any other processing.RLIMIT_AS/RLIMIT_CPU, so a crashcannot take down the API.
Done when
References
backend/app/services/exif_extractor.py:43-47backend/app/services/ingest.py:115Related: #2 (worker runtime) is the natural home for the isolation half.
Status check against the four "done when" items, since three are now closed and the fourth is the one that needs a decision.
verify_image_contentsets a deliberateMAX_IMAGE_PIXELS(300 MP; a 600dpi A4 scan is ~35 MP) and catchesDecompressionBombError. Pillow's own default only warns, which the app never configured or caught.verify()runs before any decode, and the format is allowlisted, so SVG and friends are refused rather than trusted.03bb2b3, see below.What landed just now (
03bb2b3)extract_exifcaught every exception and returned an emptyExifResult, collapsing three different situations into one indistinguishable answer: a scanned print with no EXIF block (the ordinary case), a photo whose EXIF is corrupt, and a file Pillow could not open at all.The second and third mean something is wrong with an original in an archive of irreplaceable scans, and nothing anywhere would have shown it — the photo would appear to have no EXIF permanently, and nobody would have reason to look again while the physical print is still around to rescan.
ExifResultnow carries anExifStatus(absent/present/unreadable) plus the exception class name, and the ingest audit event records both. The class name rather than the message deliberately: Pillow embeds file paths in some messages, and this string goes into the ledger.One thing I found while there: the pixel ceiling was only ever set inside
verify_image_content, which restores the previous value on exit — so theImage.open/_getexif()calls inextract_exifran under Pillow's default limit, which merely warns. Harmless today because ingest verifies first, butextract_exifis exactly what the worker will call, possibly on bytes that never went through verification. It is now wrapped in a sharedpixel_ceiling()context manager, so the bound travels with the Pillow call instead of depending on the caller's discipline.Also noted in the code:
_getexifis private Pillow API. If a release removes it, the result is nowunreadablefor every file — loud and visible in the ledger — rather than a silent archive-wide "no EXIF".16 tests in
backend/tests/test_exif_failure_visibility.py.The remaining item
Process isolation genuinely depends on the worker runtime (#2, v0.2.0), as the issue itself anticipated ("#2 is the natural home for the isolation half").
I could build it now with a
ProcessPoolExecutorandresource.setrlimit(RLIMIT_AS/RLIMIT_CPU)around the Pillow call. I do not think that is a good trade: it is a throwaway mini-worker for one call site, it adds process-spawn latency to every upload, and #2 will replace it wholesale. The remaining risk it would mitigate is a memory-safety bug in Pillow's C decoders reached through the ingest endpoint — which is now admin-only, behind the allowlist (#56), behind CSRF origin checking (#63), behindverify()and a pixel ceiling, and behind a rate limit and byte quota (#68). Pillow is also pinned and held from Renovate automerge, andpip-auditnow gates CI and reports it clean.Raising the milestone question with the user rather than deciding it myself, since it gates "when can this go back online".
Done in
5303f22. All four "done when" items now closed.You were right to overrule me, and I want to record why my reasoning was wrong rather than just say so.
Two flaws, both only visible once the code existed:
I counted
verify()and the pixel ceiling as mitigations. Both are executed by Pillow. They reduce how much of Pillow runs on hostile bytes; they do nothing about what happens when Pillow misbehaves, because they are the thing that might misbehave. I listed a control as mitigating the risk it is part of."Throwaway until #2 lands" was wrong. The worker contains a crash to the worker; it does not contain a crash within it. A worker that parses images in-process still dies mid-job on a decoder bug, possibly with a half-written transaction. This module is what the worker will call, not what it replaces.
What I got right was only the latency, and it turned out smaller than the argument needed.
Implementation, and one thing worth knowing
A plain subprocess, not
multiprocessing. I tried all three start methods.forkis unusable — uvicorn runs sync endpoints in a thread pool, forking a process whose other threads hold locks deadlocks, and the child would inherit the API's whole address space including its secrets.spawnandforkserverboth run_fixup_main_from_pathin every child, re-executing the parent's__main__; the forkserver preload list changes what the helper imports, not that.I found this the hard way — a benchmark script without a main guard failed on every parse. A security control whose correctness depends on whatever entry point is running being import-safe, failing at runtime on a real upload, is not one worth having.
python -m app.services.image_sandboxhas no such coupling, behaves identically on Windows, and costs ~45 ms of interpreter and Pillow startup.The child gets a minimal environment. Secrets live in
CIRCA_*variables, and the process parsing hostile input has no business readingCIRCA_SECRET_KEY. No fork-based approach can offer this — the child inherits everything. The subprocess boundary makes it a two-line allowlist.Results cross back as JSON, never pickle. The child is the process most likely to be compromised; unpickling its output would hand control of the parent straight through the boundary this exists to build.
RLIMIT_FSIZEis zero alongside the memory and CPU caps. Nothing in a parse legitimately writes a file, and #55 was an arbitrary-file-write bug in this same request path.Verified, not asserted
Every containment claim has a test that produces the real failure in a real process:
SIGKILLmid-parse (stands in for a decoder segfault)crashed, exit −9, parent serves the next requestRLIMIT_ASMemoryError, containedRLIMIT_CPURLIMIT_FSIZE=0CIRCA_SECRET_KEYNoneAll become a 415; none reach the client as a distinction, since which of them occurred is diagnostic information about our defences and the caller supplied the bytes.
Cost, and one thing removed
~125 ms per parse. That made the ingest route's duplicate verification worth dropping:
ingest_photoalready verifies before any storage or DB work and is the authoritative source of the stored MIME type, so the route's copy was paying a second process launch per file for no additional guarantee. Net effect on an upload is roughly break-even.Configurable via
CIRCA_IMAGE_SANDBOX_ENABLED/_MEMORY_MB/_CPU_SECONDS/_TIMEOUT_SECONDS. Enabled by default — a security control that ships off is not a control, and there is a test asserting that default.Caveat: on Windows you get process isolation but not the kernel limits (
resourceis POSIX-only). The rlimit tests skip there. Production is Linux, so this is a developer-machine caveat only.316 tests pass, verified on the real CI image (Python 3.11, package pip-installed) as well as locally.