Security patch: address all High findings for v0.1.2 - #4
Merged
Conversation
Set Pillow MAX_IMAGE_PIXELS to ~50 MP and convert DecompressionBombWarning to an error so a malicious image-block can never trigger huge memory allocations. The dispatcher converts the raised ValueError into an extraction_error and skips the block.
asyncio.gather(..., return_exceptions=True) silently dropped detector exceptions; a spaCy/Flair/GLiNER load error would degrade the ensemble to whatever still worked, with no signal. Replace with an explicit per-detector wrapper that logs each failure under detection.detector_failed so operators can detect the degraded state and downstream PII may not silently leak.
A daemon peer could previously stream an unbounded line to wedge the asyncio StreamReader into eating memory until OOM. Cap the read buffer at 32 MB and reply with a structured bad_request when a peer exceeds it. Add Pydantic max_length on every protocol string field so a too-long path or text value is refused at the validation layer before any work is queued.
Namespace names came straight from user input and were joined into the
namespace root with no validation. A name like '../../etc' or '/abs'
would silently escape the configured root. Whitelist names to
[A-Za-z0-9][A-Za-z0-9._-]{0,63} so traversal, separators, and shell
metacharacters all raise ValueError before any filesystem I/O.
The previous _ensure_key:
- created the namespace dir with the user's umask, leaving
intermediate dirs potentially world-readable
- wrote the key with default mode then chmod'd 0600 (window where
another UID could open the file)
- check-then-write race could clobber a concurrently-created key
Replace with mkdir(mode=0o700) + explicit chmod, then atomic
os.open(O_CREAT|O_EXCL, 0o600). The key is born with the right mode
and a concurrent writer is detected by FileExistsError and re-read.
… (NDS-034)
-o and --output-dir flowed straight into Path joins. Two failure modes:
- Resolved output could land outside the requested --output-dir
(Path semantics mostly cover this, but the contract was implicit)
- Nothing prevented writing inside DEFAULT_NAMESPACE_ROOT, so a
crafted -o could clobber a Fernet key or mapper.enc and silently
destroy reversibility for that namespace.
_guard_output_path now resolves both inputs, asserts is_relative_to the
requested dir, and refuses anything inside DEFAULT_NAMESPACE_ROOT. The
basename for derived names is taken from input_path.name only so a
crafted relative input cannot route the output elsewhere.
ns show printed the entire pseudonym→original map. The README's command table called it a "summary", and any wrapper, transcript, or log capture immediately defeated redaction for that namespace. Refuse to run without --unsafe and point users at ns summary for the safe counts-only view. Update the README table accordingly.
The redact handler accepted any input.path and output_path string from the socket peer. Cross-user is blocked by the 0o600 socket inside a 0o700 dir, but any same-UID process able to talk to the socket could turn the daemon into a confused-deputy primitive: read any user-readable file (and exfil through the redacted output), or overwrite any user-writable file with the redacted bytes of an arbitrary input. Resolve both paths and stat them against os.getuid() before doing any I/O. Refuse with ERR_BAD_REQUEST if the input file or the output parent dir is not owned by the daemon's UID. Add ValueError to the bad_request branch in _dispatch so a refused request gets a structured response instead of the generic internal error code.
…017) PDF documents commonly carry the same names, titles, and addresses in their Info dictionary (Author, Title, Subject, Creator, Producer, Keywords) that the body does. Without extraction those entries never reach the detector ensemble, so redacted PDFs leaked the originals while pretending the body was clean. extract_pdf and extract_pdf_with_ocr_fallback now read the Info dict via pypdfium2.PdfDocument.get_metadata_value and prepend non-empty fields to the page text. Detection treats them as ordinary text and the PDF output path (plain-text fallback) carries the pseudonymized versions instead of the originals.
DOCX and XLSX are zip envelopes around XML. python-docx already pins its lxml parser with resolve_entities=False, but openpyxl only swaps in defusedxml when the package is importable. We now declare defusedxml as a baseline dependency so openpyxl's iterparse path becomes entity-safe by default. The zip envelope itself was unguarded. A new check_ooxml_zip_safe helper runs before python-docx / openpyxl touches the bytes and refuses archives whose central directory advertises more than 200 MB uncompressed or a compression ratio above 100x — the standard zip bomb shapes. Wired into extract_docx, extract_xlsx, and the smart xlsx inference path.
… NDS-014) The decompression-bomb guard mutated PIL's module-global MAX_IMAGE_PIXELS without restoring it, leaking the lowered cap into later code paths (notably PDF rendering). Wrap the assignment in a try/finally so each call restores the previous value.
extract_docx previously only saw paragraphs and table cells in the
document body. Section headers, footers (default + first-page +
even-page variants), and review comments routinely carry the same
PII the body does — the detector ensemble never saw it, so it
survived into the redacted output untouched.
Both extract_docx and _reconstruct_docx now walk those surfaces.
PII embedded in headers/footers/comments now reaches detection on
the way in and gets pseudonymized in the output bytes on the way
out.
Footnotes, endnotes, and tracked-change deletions remain known
gaps; the standard mitigation ("Accept all changes" before
redacting) is documented in the README caveats.
11 High-severity findings from the internal 0.1.1 security review are addressed in this release: PDF metadata leak (NDS-017), DOCX header/footer/comment coverage (NDS-016), OOXML zip-bomb defense and defusedxml (NDS-015), OCR decompression-bomb guard (NDS-014), detector failure surfacing (NDS-023), daemon protocol size limits (NDS-027), daemon path-trust check (NDS-028), namespace name validation (NDS-009), Fernet key TOCTOU + 0700 dir mode (NDS-004), CLI output-path traversal guard (NDS-034), and ns-show --unsafe gate (NDS-033).
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
1 participant
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
Resolves all 11 High-severity findings from the internal 0.1.1 security review so we can cut v0.1.2 as a security patch.
Findings addressed
O_CREAT | O_EXCLand 0700 dir mode^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$)defusedxmlbaseline dep/Infometadata so detectors pseudonymize embedded PIIhandle_redactenforces same-UID ownership on input + output parentns showrequires--unsafe; points users atns summary-o/--output-dir; refuse writes into namespace storePlus a follow-up to NDS-014 scoping
Image.MAX_IMAGE_PIXELSper call so it cannot leak into other PIL consumers.CHANGELOG cut for 0.1.2 in the final commit.
Test plan
pytest -m "not slow"— 254 passed, 2 skipped, 28 deselected (matches CI selector in.github/workflows/ci.yml)tests/file_analysis/,tests/daemon/,tests/test_namespace.py,tests/test_cli.py,tests/test_ensemble.pyv0.1.2after merge to trigger hatch-vcs version + release workflow