Archive import: files are unpacked with no size cap, and pictures and logos skip the checks their upload paths apply #466

New issue

Closed

opened 2026-10-01 10:46:54 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:54 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading how core/importer.py takes files out of an archive.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The profile form's call to avatars.process is now at accounts/forms.py:397; core/importer.py and the other locations have not moved.

The archive importer caps only one kind of file. A captured page goes through _extract_within, which asks the zip directory for the entry's size and reads at most one byte past the cap. Everything else goes through _extract, which calls archive.read() with no limit, and so does read_manifest. The files that come out are then stored as they are: the avatar is not passed through avatars.process and a logo is not passed through jobs.logos.process, which is where an SVG is sanitised and a raster image re-encoded.

Where:
- core/importer.py:1101-1107, _extract: return archive.read(stored_name), against _extract_within at core/importer.py:1066-1083, used only by _restore_kept_page (:1047-1052);
- core/importer.py:114-122, read_manifest: archive.read(MANIFEST_NAME) with no limit;
- core/importer.py:390-394: the avatar is saved with profile.avatar.save(..., ContentFile(content), save=True); the profile form runs avatars.process (accounts/forms.py:396), which caps, decodes and re-encodes it;
- core/importer.py:577-581: a new company's logo is saved as it came; the other paths run jobs.logos.process (jobs/logos.py:82), which sanitises an SVG with pictures.sanitise_svg and re-encodes anything else;
- core/importer.py:846-851 (uploads) and :880-884 (sent documents): no cap, where the upload form refuses more than MAX_UPLOAD_BYTES (documents/forms.py:26, 20 MB).
What happens: an archive whose entry is small in the zip but expands to several gigabytes is read whole into memory and exhausts the importing process. An avatar or logo that the normal paths would have refused, re-encoded or cleaned (an SVG with scripts or external references, an image with its metadata, a file of any size) is stored and later served as the person's picture or the company's logo.
Who can trigger it: only the operator, by running the import_data command (core/management/commands/import_data.py) on an archive; there is no web path to the archive importer. The realistic case is an archive somebody else produced and handed over for restoring.
Why it matters: this is hardening rather than an open hole. serve_private_file sends every private file with default-src 'none'; sandbox (core/files.py:62), so a script inside an imported SVG does not run when the file is visited, and the memory cost falls on a one-off command, not on the web server. But jobs/logos.py says of the sanitiser and the hardened serving that "the two are halves of one defence and neither is enough alone", and the importer supplies only one of them; it also contradicts its own capture path, which was capped for exactly this reason.
Fix: read every file through _extract_within with a cap per kind (avatars.MAX_UPLOAD_BYTES, logos.MAX_BYTES, documents.forms.MAX_UPLOAD_BYTES), and read the manifest through it too with a generous cap. Pass an imported avatar through avatars.process and an imported logo through jobs.logos.process, saving under the extension process returns. When a file is over its cap or refused, leave the field empty and add a line to report.skipped, as the capture path already does.

Done when

A test in tests/security/test_files.py builds an archive whose upload entry declares more than MAX_UPLOAD_BYTES, runs importer.load, and finds the upload restored without its file and a line in report.skipped.
A test in tests/security/test_files.py imports an archive whose company logo is an SVG carrying a <script> element and finds the stored logo without it (or refused and reported).
A test in tests/test_avatars.py imports an avatar larger than avatars.MAX_UPLOAD_BYTES, or one that is not an image, and finds it refused and reported.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading how `core/importer.py` takes files out of an archive. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The profile form's call to `avatars.process` is now at `accounts/forms.py:397`; `core/importer.py` and the other locations have not moved. The archive importer caps only one kind of file. A captured page goes through `_extract_within`, which asks the zip directory for the entry's size and reads at most one byte past the cap. Everything else goes through `_extract`, which calls `archive.read()` with no limit, and so does `read_manifest`. The files that come out are then stored as they are: the avatar is not passed through `avatars.process` and a logo is not passed through `jobs.logos.process`, which is where an SVG is sanitised and a raster image re-encoded. - **Where:** - `core/importer.py:1101-1107`, `_extract`: `return archive.read(stored_name)`, against `_extract_within` at `core/importer.py:1066-1083`, used only by `_restore_kept_page` (`:1047-1052`); - `core/importer.py:114-122`, `read_manifest`: `archive.read(MANIFEST_NAME)` with no limit; - `core/importer.py:390-394`: the avatar is saved with `profile.avatar.save(..., ContentFile(content), save=True)`; the profile form runs `avatars.process` (`accounts/forms.py:396`), which caps, decodes and re-encodes it; - `core/importer.py:577-581`: a new company's logo is saved as it came; the other paths run `jobs.logos.process` (`jobs/logos.py:82`), which sanitises an SVG with `pictures.sanitise_svg` and re-encodes anything else; - `core/importer.py:846-851` (uploads) and `:880-884` (sent documents): no cap, where the upload form refuses more than `MAX_UPLOAD_BYTES` (`documents/forms.py:26`, 20 MB). - **What happens:** an archive whose entry is small in the zip but expands to several gigabytes is read whole into memory and exhausts the importing process. An avatar or logo that the normal paths would have refused, re-encoded or cleaned (an SVG with scripts or external references, an image with its metadata, a file of any size) is stored and later served as the person's picture or the company's logo. - **Who can trigger it:** only the operator, by running the `import_data` command (`core/management/commands/import_data.py`) on an archive; there is no web path to the archive importer. The realistic case is an archive somebody else produced and handed over for restoring. - **Why it matters:** this is hardening rather than an open hole. `serve_private_file` sends every private file with `default-src 'none'; sandbox` (`core/files.py:62`), so a script inside an imported SVG does not run when the file is visited, and the memory cost falls on a one-off command, not on the web server. But `jobs/logos.py` says of the sanitiser and the hardened serving that "the two are halves of one defence and neither is enough alone", and the importer supplies only one of them; it also contradicts its own capture path, which was capped for exactly this reason. - **Fix:** read every file through `_extract_within` with a cap per kind (`avatars.MAX_UPLOAD_BYTES`, `logos.MAX_BYTES`, `documents.forms.MAX_UPLOAD_BYTES`), and read the manifest through it too with a generous cap. Pass an imported avatar through `avatars.process` and an imported logo through `jobs.logos.process`, saving under the extension `process` returns. When a file is over its cap or refused, leave the field empty and add a line to `report.skipped`, as the capture path already does. ## Done when - A test in `tests/security/test_files.py` builds an archive whose upload entry declares more than `MAX_UPLOAD_BYTES`, runs `importer.load`, and finds the upload restored without its file and a line in `report.skipped`. - A test in `tests/security/test_files.py` imports an archive whose company logo is an SVG carrying a `<script>` element and finds the stored logo without it (or refused and reported). - A test in `tests/test_avatars.py` imports an avatar larger than `avatars.MAX_UPLOAD_BYTES`, or one that is not an image, and finds it refused and reported.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:46:54 +00:00

tiagoagueda added the

security

enhancement

tier

labels

2026-10-01 10:46:54 +00:00

tiagoagueda referenced this issue

2026-10-03 01:58:39 +00:00

Export and import one kind of record as JSON: companies, contacts, listings and applications, each as a file of its own #659

tiagoagueda referenced this issue