From 79a8908b1bf437ed06a48e9f95027abdf6bcece0 Mon Sep 17 00:00:00 2001 From: Douglas Mun Date: Mon, 17 Aug 2026 15:29:47 +0800 Subject: [PATCH] fix(pdf): fail closed when the outline / AcroForm walks hit their node cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _strip_pdf_outlines() ended its walk with `continue` and _strip_acroform_fields() with `break` on reaching _MAX_WALK_NODES, then returned normally — cdr_pdf shipped the file as sanitised carrying live actions past node 100,000. Reproduced at production cap size: an 11.6 MiB outline chain (well under the 100 MB _MAX_FILE_BYTES) returned sanitised with live /A JavaScript; same for a /Kids chain. Both now raise CdrReject, matching _walk_pdf_nodes (pitfall #59). Also: the #60 cap audit enumerated caps from memory and never listed _MAX_WALK_NODES; check_fail_closed.py is structurally blind to a cap that never raises. Recorded as pitfall #61 with the source-enumerated cap list. Runbook drift: the Terraform EventBridge rule is ${resource_prefix}-s3- object-created, not the literal cdr-s3-object-created; and MaxMemoryUsed is not an AWS/Lambda metric — replaced with the Logs Insights REPORT query docs/benchmark.py already uses. 454 tests pass (4 new); all 7 CI guards pass. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- docs/claude/checklist-and-invariants.md | 2 +- docs/claude/pitfalls.md | 11 ++++ docs/claude/testing.md | 2 +- docs/deployment-runbook.md | 36 ++++++----- src/lambda_function.py | 21 ++++++- src/test_cdr.py | 84 +++++++++++++++++++++++++ 7 files changed, 139 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index e3fd348..9f33a5a 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ python3.12 -m venv .venv && source .venv/bin/activate # Install dependencies (Lambda + local-service + test-only deps) pip install -r src/requirements.txt -r src/requirements-local.txt -r src/requirements-dev.txt -# Run the full test suite (450 tests: 400 CDR Lambda + 50 local variant) +# Run the full test suite (454 tests: 404 CDR Lambda + 50 local variant) cd src && pytest test_cdr.py test_cdr_local.py -v # Run one class or test diff --git a/docs/claude/checklist-and-invariants.md b/docs/claude/checklist-and-invariants.md index 0309714..8f3d69c 100644 --- a/docs/claude/checklist-and-invariants.md +++ b/docs/claude/checklist-and-invariants.md @@ -57,7 +57,7 @@ Run this checklist before considering any change complete. Each item maps to a c ### Test invariants — verify coverage was not regressed -- [ ] **450 tests pass** (all passed) — `cd src && pytest test_cdr.py test_cdr_local.py -v` shows no failures. +- [ ] **454 tests pass** (all passed) — `cd src && pytest test_cdr.py test_cdr_local.py -v` shows no failures. - [ ] **No routing/disarm logic duplicated into a front-end** — `handler` and `app.py` both delegate to `cdr_dispatch`; a security decision must live in exactly one place (pitfall #41). - [ ] **Every new CDR path has a test** — if you add a new strip rule, add a fixture that carries that threat and assert it is removed. - [ ] **Every new try/except warn-and-continue block has a failure-path test** — prove the success path completes even when the wrapped operation throws. diff --git a/docs/claude/pitfalls.md b/docs/claude/pitfalls.md index b3eaddb..3ff106c 100644 --- a/docs/claude/pitfalls.md +++ b/docs/claude/pitfalls.md @@ -94,6 +94,7 @@ it is listed once, under the group whose code you would be editing. - [#58 — A MIME parameter defeats a `+xml` suffix test — and the OPC container-layer sweep that found nothing else](#58-a-mime-parameter-defeats-a-xml-suffix-test-and-the-opc-container-layer-sweep-that-found-nothing-else) - [#59 — A resource cap that `return`s hands the attacker the sweep's coverage — bound the work, but fail closed](#59-a-resource-cap-that-returns-hands-the-attacker-the-sweeps-coverage-bound-the-work-but-fail-closed) - [#60 — Auditing every cap's failure direction, and making "fail closed" a CI guard instead of a habit](#60-auditing-every-caps-failure-direction-and-making-fail-closed-a-ci-guard-instead-of-a-habit) +- [#61 — The #60 audit enumerated caps it already knew about — `_MAX_WALK_NODES` was never on the list, and truncated in two sweeps](#61-the-60-audit-enumerated-caps-it-already-knew-about-_max_walk_nodes-was-never-on-the-list-and-truncated-in-two-sweeps) **Multi-bug audit batches** @@ -386,6 +387,7 @@ The direct follow-up to #59: if one resource cap silently truncated, the others | Cap | On exceeding | Correct? | |---|---|---| | `_PDF_WALK_MAX_NODES` | `CdrReject` | fixed in #59 | +| `_MAX_WALK_NODES` (outlines, AcroForm) | `continue` / `break` → truncate | **NO — missed by this audit, fixed in #61** | | `_EXTERNAL_REF_SCAN_MAX_NODES` | `return True` (= "is external", so the subtree is removed) | yes — closed by design | | `_DecompressionBudget` / `_MAX_TOTAL_ENTRY_BYTES` | `CdrReject` | yes | | `_MAX_ZIP_ENTRIES` | `_validate_zip_structure` hard-fail verdict | yes | @@ -400,3 +402,12 @@ That last row is why this guard is mutation-based rather than a grep for `return **Two things the guard needed in order not to become the very defect it checks for.** (a) **A baseline green run before any mutant means anything** — without it a broken checkout reports every mutant "caught" and the guard passes vacuously (#57). This fired immediately and usefully: the first version exported `SANITISED_BUCKET`/`QUARANTINE_BUCKET`, which the test module defaults with `os.environ.setdefault` — that *yields* to the environment, so five tests asserting the literal `test-sanitised`/`test-quarantine` names failed. The documented "run pytest BARE" rule, rediscovered by writing a tool that broke it. (b) **A negative control on the guard itself**: a deliberately unpinned `raise CdrReject` was injected and the guard correctly named it by line. A checker never shown failing is an assumption (#57 again). **General rule: when a review finds a bug class rather than a bug, the deliverable is the executable check, not the fixed instance.** This repo already had that instinct for docs (`check_test_count.py`, `check_cap_defaults.py`, `check_pitfalls_index.py`, `check_iac_parity.py`); security invariants deserve it more, because a doc drifting is embarrassing and a fail-open shipping is not. Related: #59 (the bug), #57 (instrument discipline), #53 (docstring vs behaviour). + +### 61. The #60 audit enumerated caps it already knew about — `_MAX_WALK_NODES` was never on the list, and truncated in two sweeps +An external review (Codex, 2026-08-17) found the exact bug class #59 documents and #60 claimed to have swept clean, in a constant the #60 table never mentions. + +`_strip_pdf_outlines` ended its walk with `continue` and `_strip_acroform_fields` with `break` on reaching `_MAX_WALK_NODES` (100,000). Both then returned normally and `cdr_pdf` shipped the file as sanitised. Reproduced end to end: a **12.2 MiB** PDF with a 100,002-node outline chain and a **10.6 MiB** PDF with a 100,002-node `/Kids` chain each came back "sanitised" carrying live actions — well under the 100 MB `_MAX_FILE_BYTES`, so no size guard is in the way. Fixed to `raise CdrReject`, matching `_walk_pdf_nodes`. Pinned by `test_outline_walk_cap_rejects_rather_than_truncating` / `test_acroform_walk_cap_rejects_rather_than_truncating` (exception) and the two `_payload_never_ships` tests (outcome), so any silent-completion replacement still fails. + +**Why #60 missed it, and the lesson that is not "audit harder".** #60's table is a *hand-written enumeration* — ten constants someone remembered. `_MAX_WALK_NODES` is a *shared* cap used by two functions and named nothing like `_PDF_WALK_MAX_NODES`, so it fell out of recall. `check_fail_closed.py` could not catch it either: that guard mutates rejections that **exist** and asks whether a test notices; it is structurally blind to a cap that never raises in the first place. **A guard that verifies existing invariants cannot find a missing one.** The complement to `check_fail_closed.py` is enumeration from the source, not from memory: `grep -n '_MAX_[A-Z_]*\|_BUDGET' src/lambda_function.py` yields the real list, and every hit must be traced to an explicit failure direction before the audit counts as complete. + +**General rule: when a review concludes "no second instance", state how the instances were enumerated.** "I checked the caps" and "I listed every `_MAX_`/budget constant in the module and traced each" are different claims with the same wording, and only the second one is falsifiable. Related: #59 (the bug class), #60 (the audit that missed this), #57 (a check that cannot fail is not a check). diff --git a/docs/claude/testing.md b/docs/claude/testing.md index 5f0e9d8..628efcc 100644 --- a/docs/claude/testing.md +++ b/docs/claude/testing.md @@ -21,7 +21,7 @@ > produce, since it never attempts the fetch. The manual pass therefore stands at **14 of 14 with > no engine unverified**. See `docs/viewer-validation/CHECKLIST.md`. -Tests (400 in `test_cdr.py` + 50 in `test_cdr_local.py` = **450 total**) construct malicious fixtures entirely in-memory — no fixture files on disk. S3/SNS calls are patched with `unittest.mock`. Required env vars are set automatically via `os.environ.setdefault`. `src/test_cdr.py` covers the CDR Lambda; `src/test_cdr_local.py` covers the pure `cdr_dispatch` core and the local FastAPI service (`app.py`), reusing the same in-memory fixtures. Run: `cd src && pytest test_cdr.py test_cdr_local.py -v`. +Tests (404 in `test_cdr.py` + 50 in `test_cdr_local.py` = **454 total**) construct malicious fixtures entirely in-memory — no fixture files on disk. S3/SNS calls are patched with `unittest.mock`. Required env vars are set automatically via `os.environ.setdefault`. `src/test_cdr.py` covers the CDR Lambda; `src/test_cdr_local.py` covers the pure `cdr_dispatch` core and the local FastAPI service (`app.py`), reusing the same in-memory fixtures. Run: `cd src && pytest test_cdr.py test_cdr_local.py -v`. > **Run it bare — do not export AWS env vars around it.** `src/conftest.py` sets the > credentials/region and `test_cdr.py` sets the bucket and topic names, both via diff --git a/docs/deployment-runbook.md b/docs/deployment-runbook.md index 12260bb..4e122ba 100644 --- a/docs/deployment-runbook.md +++ b/docs/deployment-runbook.md @@ -242,12 +242,12 @@ aws events list-targets-by-rule \ --query 'Targets[].Arn' ``` -**OpenTofu / Terraform** — the EventBridge rule is named `cdr-s3-object-created`: +**OpenTofu / Terraform** — the EventBridge rule is named `${resource_prefix}-s3-object-created`, i.e. `$PREFIX-s3-object-created` (it is *not* the fixed string `cdr-s3-object-created` unless you deployed with the default prefix): ```bash ( cd terraform && tofu state list ) # resource inventory aws events list-targets-by-rule \ - --rule cdr-s3-object-created \ + --rule $PREFIX-s3-object-created \ --query 'Targets[].Arn' ``` @@ -356,25 +356,33 @@ aws cloudwatch get-metric-statistics \ --extended-statistics p50 p99 \ --query 'Datapoints[0].{p50:ExtendedStatistics.p50,p99:ExtendedStatistics.p99}' -# Max memory used -aws cloudwatch get-metric-statistics \ - --namespace AWS/Lambda \ - --metric-name MaxMemoryUsed \ - --dimensions Name=FunctionName,Value=$PREFIX-lambda \ - --start-time $START \ - --end-time $END \ - --period 600 \ - --statistics Maximum \ - --query 'Datapoints[0].Maximum' +# Max memory used — NOT a CloudWatch metric. `MaxMemoryUsed` does not exist in the +# AWS/Lambda namespace; querying it returns an empty Datapoints list, which reads as +# "no memory pressure" when it actually means "no such metric". The number lives only +# in the Lambda REPORT log line, so pull it from Logs Insights (this is what +# docs/benchmark.py does): +START_EPOCH=$(python3 -c "from datetime import datetime,timezone,timedelta; print(int((datetime.now(timezone.utc)-timedelta(minutes=10)).timestamp()))") +END_EPOCH=$(python3 -c "from datetime import datetime,timezone; print(int(datetime.now(timezone.utc).timestamp()))") +QID=$(aws logs start-query \ + --log-group-name /aws/lambda/$PREFIX-lambda \ + --start-time $START_EPOCH \ + --end-time $END_EPOCH \ + --query-string 'filter @message like /REPORT/ | parse @message "Max Memory Used: * MB" as mem | stats max(mem) as max_mem' \ + --query queryId --output text) +sleep 5 +aws logs get-query-results --query-id "$QID" --query 'results[0][0].value' ``` +Or skip the manual queries entirely and run `python docs/benchmark.py --bucket `, +which reports p50/p99 duration, throttles and max memory in one pass. + ### Tuning thresholds | Metric | Current setting | Action if exceeded | |---|---|---| | p99 Duration > 250 s | Timeout = 300 s | Increase `Timeout` in `template.yaml`; re-deploy | | p99 Duration > 200 s on PDFs | 1024 MB memory | Increase `MemorySize` (Lambda CPU scales with RAM) | -| MaxMemoryUsed > 900 MB | 1024 MB memory | Increase `MemorySize` to 2048 MB | +| Max Memory Used > 900 MB (REPORT log line) | 1024 MB memory | Increase `MemorySize` to 2048 MB | | Throttles > 0 | `ReservedConcurrentExecutions: 20` | Increase reservation if throughput SLA demands it | | DLQ depth > 0 | — | Inspect DLQ messages: `aws sqs receive-message --queue-url ` | @@ -499,7 +507,7 @@ RULE_NAME=$(aws cloudformation describe-stack-resource \ aws events describe-rule --name "$RULE_NAME" --query 'State' # OpenTofu / Terraform: -aws events describe-rule --name cdr-s3-object-created --query 'State' +aws events describe-rule --name $PREFIX-s3-object-created --query 'State' ``` If disabled, enable it with `aws events enable-rule --name `. diff --git a/src/lambda_function.py b/src/lambda_function.py index 4a19d31..8255ef8 100644 --- a/src/lambda_function.py +++ b/src/lambda_function.py @@ -1930,7 +1930,17 @@ def _strip_pdf_outlines(catalog) -> list[str]: while stack: node = stack.pop() - if node is None or visited_count >= _MAX_WALK_NODES: + if visited_count >= _MAX_WALK_NODES: + # FAIL CLOSED (pitfall #59/#60): `continue`-ing here drains the stack without + # examining it, so outline items beyond the cap keep their /A /AA actions and + # cdr_pdf still returns a "sanitised" file. Node count is attacker-controlled + # — a 100_002-node outline tree fits well under _MAX_FILE_BYTES. + logger.warning("PDF outline walk hit the %d-node cap; rejecting", + _MAX_WALK_NODES) + raise CdrReject( + f"PDF outline tree exceeds the {_MAX_WALK_NODES}-node walk cap; " + "cannot prove all outline actions were stripped") + if node is None: continue try: ident = node.objgen @@ -1974,7 +1984,14 @@ def _strip_acroform_fields(fields) -> list[str]: while stack: field = stack.pop() if visited_count >= _MAX_WALK_NODES: - break + # FAIL CLOSED (pitfall #59/#60): `break`ing here abandons the rest of the + # field tree, leaving deeper /JS /A /AA live in a file that is then reported + # sanitised. See the identical bound in _walk_pdf_nodes. + logger.warning("PDF AcroForm walk hit the %d-node cap; rejecting", + _MAX_WALK_NODES) + raise CdrReject( + f"PDF AcroForm field tree exceeds the {_MAX_WALK_NODES}-node walk cap; " + "cannot prove all field actions were stripped") try: try: ident = field.objgen diff --git a/src/test_cdr.py b/src/test_cdr.py index 020c674..1c226cb 100644 --- a/src/test_cdr.py +++ b/src/test_cdr.py @@ -1801,6 +1801,45 @@ def test_acroform_deep_noncyclic_kids_fully_swept(self): assert len(removed) == depth, \ f"only {len(removed)}/{depth} fields swept — deep chain silently truncated" + def _kids_chain_pdf(self, depth): + pdf = pikepdf.Pdf.new() + pdf.add_blank_page() + prev = None + for i in range(depth): + field = pdf.make_indirect(pikepdf.Dictionary( + T=pikepdf.String(f"f{i}"), + FT=pikepdf.Name("/Tx"), + JS=pikepdf.String("app.alert('CAPTAIL');"), + )) + if prev is not None: + field["/Kids"] = pikepdf.Array([prev]) + prev = field + pdf.Root["/AcroForm"] = pikepdf.Dictionary(Fields=pikepdf.Array([prev])) + buf = io.BytesIO() + pdf.save(buf, compress_streams=False) + return buf.getvalue() + + # ── Audit fix: the field-tree cap `break`ed, abandoning the rest of the /Kids chain + # while cdr_pdf still reported the file sanitised. Same failure direction as the + # outline cap and _walk_pdf_nodes — bound the work, but fail closed (pitfall #59) ── + def test_acroform_walk_cap_rejects_rather_than_truncating(self, monkeypatch): + monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10) + raw = self._kids_chain_pdf(40) + assert b"CAPTAIL" in raw # precondition: payload really is in the input + with pytest.raises(cdr.CdrReject, match="walk cap"): + cdr.cdr_pdf(raw) + + def test_acroform_walk_cap_payload_never_ships(self, monkeypatch): + """Pins the outcome, not the exception.""" + monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10) + raw = self._kids_chain_pdf(40) + try: + clean, _ = cdr.cdr_pdf(raw) + except cdr.CdrReject: + return + assert b"CAPTAIL" not in clean, \ + "over-cap AcroForm field actions survived into the sanitised output" + class TestPdfNamesEmbeddedFiles: """PDF /Names./EmbeddedFiles is removed.""" @@ -3064,6 +3103,51 @@ def test_pdf_deep_noncyclic_outline_chain_fully_swept(self): assert len(removed) == depth, \ f"only {len(removed)}/{depth} outline nodes swept — deep chain silently truncated" + def _outline_chain_pdf(self, depth): + pdf = pikepdf.Pdf.new() + pdf.add_blank_page() + first = None + prev = None + for i in range(depth): + item = pdf.make_indirect(pikepdf.Dictionary( + Title=pikepdf.String(f"item{i}"), + A=pikepdf.Dictionary(S=pikepdf.Name("/JavaScript"), + JS=pikepdf.String("app.alert('CAPTAIL')")), + )) + if prev is not None: + prev["/Next"] = item + else: + first = item + prev = item + pdf.Root["/Outlines"] = pikepdf.Dictionary( + Type=pikepdf.Name("/Outlines"), First=first, Last=prev, Count=depth) + buf = io.BytesIO() + pdf.save(buf, compress_streams=False) + return buf.getvalue() + + # ── Audit fix: the outline cap `continue`d, draining the stack without examining it, + # so outline items past the cap kept their /A /AA and cdr_pdf still returned a + # "sanitised" file. Node count is attacker-controlled (a 100_002-node outline tree + # is ~12 MB, far under _MAX_FILE_BYTES) — the cap must fail closed (pitfall #59) ── + def test_outline_walk_cap_rejects_rather_than_truncating(self, monkeypatch): + monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10) + raw = self._outline_chain_pdf(40) + assert b"CAPTAIL" in raw # precondition: payload really is in the input + with pytest.raises(cdr.CdrReject, match="walk cap"): + cdr.cdr_pdf(raw) + + def test_outline_walk_cap_payload_never_ships(self, monkeypatch): + """Pins the outcome, not the exception: an over-cap outline tree must never reach + the sanitised bucket with live actions, whatever path replaces CdrReject.""" + monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10) + raw = self._outline_chain_pdf(40) + try: + clean, _ = cdr.cdr_pdf(raw) + except cdr.CdrReject: + return + assert b"CAPTAIL" not in clean, \ + "over-cap outline actions survived into the sanitised output" + def test_pdf_goToE_annotation_action_stripped(self): """/GoToE (re-reaches embedded files) was not in the old denylist; the new unconditional /A deletion catches it (M4)."""