diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index f9efaaa..0ec8bf7 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8502,3 +8502,68 @@ The three surfaces now share **one** liveness helper, because they had been disa **Source:** zizmor-1280 handoff, 2026-08-02, which reported claim `7` stranded by a prune and filed the mechanism as unbuilt. Half A and Half B were then re-verified against the code directly rather than inherited: the absent claim handling by search over `prune-merged.ps1`, the `-Take`/`-Release` blindness by reading both `main` and `claim-liveness`. --- + +## 347. A PHI-at-rest assertion that can pass for the wrong reason β€” short substring vs. random ciphertext + +> 🚧 **Status OPEN (filed 2026-08-02).** `tests/test_store_encryption.py:95` asserts `raw.startswith(MARKER_PREFIX) and "DOE" not in raw` β€” three characters of a 76-character body β€” as the proof that a patient surname is unreadable at rest. **The instrument is wrong in both directions.** It **fails when encryption worked perfectly** (the value is encrypted under `make_cipher(generate_key())`, a fresh random key every run, so the base64 body is fresh random text and base64's alphabet contains `D`, `O` and `E`), and β€” the half that matters β€” it would **PASS on a weak encoding that merely happened to avoid those three characters**. A test that can pass for the wrong reason is a false assurance about PHI; one that occasionally fails for the wrong reason is only noise. **The flake is what made someone look; it is not what is wrong.** + +**Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** build (small). **Severity:** medium (a PHI-at-rest gate that certifies a property it cannot see), medium (likelihood: measured below, and it has already fired). + +**How it surfaced.** PR #142, CI job `91502517146`, leg `test (windows-2022, py3.14)`: `AssertionError: assert (True and 'DOE' not in 'mfenc:v1:7f...oHUc/t9nnmT9')`. + +**The rate β€” the per-CI-run figure, not the per-assertion one.** For a uniform base64 string of length *L*, a given *k*-character pattern is expected `(L-k+1)/64^k` times. At the observed ciphertext (~146 chars for the `ADT` fixture: 12-byte nonce + 76-byte body + 16-byte tag, base64'd) a 3-char literal gives **p = 5.49e-4 per assertion per run**. Derived here exactly (`Fraction`, cross-checked with `-expm1(N*log1p(-x))`); **the 200k-trial simulation corroborating it came with the originating defect report, not from this filing**; one session reproduced it independently at N=144 windows and another recomputed it analytically. All four agree. But **there are two such assertions** (`:95` and `:303`, both `DOE`) and this repo runs **three OS legs** (`ubuntu` + `windows-2022` + `windows-2025`, one Python version β€” [`ci.yml`](../.github/workflows/ci.yml)), so what an operator actually experiences is: + +| Scope | Rate | +| --- | --- | +| one assertion, one leg | 1 in 1,821 | +| either assertion, one leg | 1 in 911 | +| **either assertion, one full CI run (3 legs)** | **1 in 304** | + +**Both caveats, because neither number should be handed on bare.** *L* is taken from one measured at-rest value and real strings vary in length; and the `mfenc:v1::` prefix region is not base64, so the effective window count is lower and **every figure above is a slight over-estimate**. Same order, not exact. What is not in doubt is the scope correction: 1 in 304 CI runs is an operational cost, where 1 in 1,821 reads as ignorable. + +**Confirmed by prediction, not by agreement.** After the failure, PR #142's full re-run came back **25 passed, 0 failed** with `test_bodies_encrypted_at_rest` green. That prediction (P(same collision twice) β‰ˆ 5e-4) was written down *before* the re-run β€” so a green re-run confirms a chance collision rather than resetting the question. Had it failed twice, the diagnosis would have been falsified and something real would be at fault. + +**The convention already exists in the same file; the sweep was incomplete.** `test_cipher_round_trip_and_hides_plaintext` (:49–58) was already converted to the deterministic form and carries the rule in a comment β€” *"NEVER assert short-substring absence ('MSH'/'DOE') … that assertion HAS flaked in CI"* β€” and `test_v2_round_trip_marker_and_decrypt` (:531) cites it. The **call sites were never swept**, so the identical assertion survives at :95 and :303. + +**The rule, so the next call site has a boundary rather than a precedent.** A substring assertion against ciphertext is safe on **either** of two grounds, and they are not equally good: + +1. **Deterministic β€” the token contains a character the *whole stored value* cannot contain.** The haystack is `:`, so the test is against that, not against the base64 alphabet alone: `|` and `\r` qualify, **`:` does not** (the marker carries colons). Where it holds, the assertion cannot fail by chance **at any length** β€” a *proof*, not a probability. +2. **Probabilistic β€” the token is β‰₯ 6 characters.** The exponent is the token length, so risk collapses fast: at 6 chars p = 2.10e-9 (1 in 477 million), at 8 chars 5.12e-13, at 14 chars 7.44e-24. Below 6, unsafe. + +**Prefer (1). It is the same principle as the recommended fix** β€” `assert ADT not in raw` is deterministic precisely because the fixture carries `|` and `\r` β€” so the rule and the remedy are one idea, not two. Ground (2) is what to fall back on when the token must be a bare identifier; it makes an assertion *improbable*, never *impossible*. + +> **A trap for whoever re-derives these.** The obvious expression `1-(1-64**-k)**N` **underflows to exactly `0.0` at k=14** β€” `1 - 64**-14` is not representable in float64 and rounds to `1.0` β€” so it reports a probability of zero, silently, with no warning, in a column of otherwise plausible values. **It is correct everywhere you would sanity-check it and silently wrong only in the tail**, which is why it survives review: the first row you try agrees with every other method to six figures. That is how the 14-char row was first written down as "unreachable", inside an analysis arguing that token length is the discriminator, at the one row where length was extreme enough to break the arithmetic. Use `-expm1(N*log1p(-64**-k))`, or `Fraction`; both give 7.44e-24. **The figures above were computed both ways and agree.** Reproducing that zero in a doc about an assertion that states more confidence than it has would have been the same defect one level up β€” hence "7.44e-24", not "0". + +**Audit of the siblings β€” the answer is neither "just one" nor "all of them".** + +| Site | Literal | Chars | p per assertion per leg | Verdict | +| --- | --- | --- | --- | --- | +| `test_store_encryption.py:95` (`test_bodies_encrypted_at_rest`) | `DOE` | 3 | 5.49e-4 (1 in 1,821) | **fix β€” the one that fired** | +| `test_store_encryption.py:303` (`test_summary_and_metadata_…`) | `DOE` | 3 | 5.49e-4 (1 in 1,821) | **fix β€” same shape, never observed** | +| `test_content_search.py:123` | `JANE` | 4 | 8.58e-6 (1 in 116,509) | **fix β€” below the β‰₯6 rule** | +| `test_store_encryption.py:232/233/251`, `:303` (`999001`), `:304`, `:1062`; `test_sqlserver_store.py:1582/1584/1682/1920/2044`; `test_postgres_store.py:2163/2164`; `test_reference_sets.py:170`; `test_transform_state.py:281` | `SECRET…`, `999001`, `WESTWING`, `bad parse`, … | β‰₯6 | ≀2.10e-9 | **leave alone** | + +**Do not rewrite the β‰₯6 group.** They are the same *pattern* but not a defect at any rate that will ever be observed, and churning a dozen correct assertions makes the diff harder to review for no risk reduction. The pattern-propagation concern is real but is answered by **writing the β‰₯6 rule into the :49–58 comment**, not by the churn. + +**The count, stated with its basis, because three different numbers were quoted before anyone checked.** Within `test_store_encryption.py` there are **7 lines carrying 8 substring-absence clauses against at-rest ciphertext** (:95, :232, :233, :251, :303 Γ—2, :304, :1062), of which **2 β€” both `DOE`, at :95 and :303 β€” are below the β‰₯6 rule**. Repo-wide the shape appears ~16 times. Correctly **excluded** and not to be counted again: `:905–908` and `:927–928` assert against exception/`caplog` text, not ciphertext (a different shape a grep sweeps up); `:56`, `:532`, `:625` (`ADT not in token`) are safe on **ground (1)** β€” `|` and `\r` appear nowhere in `:`. `:512` (`":v2:" not in produced`) is deterministic too but **not** on ground (1), and the distinction is worth keeping straight: `:` *is* present in the haystack (the marker is `mfenc:v1::`), so ground (1) does not apply. It holds instead because the marker's layout is fixed and its version field reads `v1`, while the base64 body contains no `:` for the run to straddle β€” structure, not alphabet. + +`test_off_by_default_stores_plaintext` (:111) **does not share the shape** β€” `_raw_at_rest(db) == ADT`, deterministic equality against known plaintext. Nor do the many `"DOE" not in …` assertions elsewhere in `tests/` that check *scrubbed plaintext* (`safe_text`, the anonymizer, ACK detail): deterministic output, not ciphertext, correct as written. + +**Fix direction (maintainer's choice β€” do NOT simply widen or delete the substring check):** +1. Assert against the **decoded** ciphertext bytes rather than the base64 text, or +2. assert the plaintext is **not recoverable** from the stored value (the property actually claimed), or +3. assert full-plaintext absence β€” `assert ADT not in raw` β€” deterministic because the fixture contains `|` and `\r`, bytes base64 can never emit. That is the idiom :56 already uses, so it is the cheapest change. + +The `startswith(MARKER_PREFIX)` half stays in every case. + +**Whichever is chosen, prove the new assertion can FAIL before trusting that it passes** β€” break the encryption deliberately (hand the store an `IdentityCipher`, or plant a plaintext body) and watch the rewritten test go red, then restore. This item exists because a green was taken as evidence for a property it could not see; shipping its replacement on an unfalsified green would reproduce the defect in the fix. Note the trap that makes this more than a formality, learned the hard way elsewhere in this repo today: proving the *instrument* can fire is only half β€” the *workload* must also be able to produce the failure class. An 800-iteration repro loop returned 800/800 green against a live SQL Server while hunting a lock-contention bug, because running the tests in isolation was the one configuration that could not generate contention. A rig that excludes the condition it is hunting reports silence, and silence reads like evidence. + +**Related:** [`tests/test_store_encryption.py`](../tests/test_store_encryption.py):95, :303, :49–58 (the convention comment, already correct β€” the natural home for the β‰₯6 rule); [`tests/test_content_search.py`](../tests/test_content_search.py):123; CLAUDE.md Β§9 (the PHI-at-rest guarantee the assertion is reaching for); [`Secure_Development_Standards`](Secure_Development_Standards.md) Β§3 (reviewing security prose by what a reader would DO with it β€” the same question, asked of a test instead of a paragraph). + +**#344 β€” cited for the harm, NOT the cause; do not fold them.** Both are individually-blameless CI reds that invite the wrong fix, in a repo whose two famous "flakes" turned out to be a livelock and a test that was right. But: **#344's thesis is a fixed bound meeting variable latency; #347 is a deterministic property tested by a probabilistic proxy.** (Its *thesis* deliberately β€” that item's instance 2 has since been re-diagnosed as a swallowed lock-timeout rather than a bound at all, so "#344 = timeouts" is not a premise to lean on.) Neither is fixed by changing a number, for opposite reasons. The one-line discriminator, from #344's owner: **this one would fire at exactly the same rate on an infinitely fast machine.** A reader who follows the link lands on a wall-clock item and must not back-infer that this is a timing bug β€” it is not. + +**#346 β€” the closer sibling.** Same defect class stated generally: *an assertion that passes for a reason unrelated to the property it claims to test.* This one passes because random base64 usually lacks a 3-character run; #346's would have passed because nothing walks the imports. Both are green signals that are not evidence. + +**ADR 0158 β€” the taxonomy, and where this item sits in it.** Cited by **rule**, not just by number, because the rule is what transfers: *"An equality check satisfiable by coincidence is not an equality check."* That is this defect exactly. By the ADR's own one-line test for **Class 2** β€” *a control that cannot observe or act on its own failure*: **if this control were broken, what would tell me?** If the encryption were replaced tomorrow with a weak encoding, `"DOE" not in raw` would still go green. The answer is the control, which is the defect. *(Deliberately unlinked: the ADR is on PR #145's branch and not yet on `main`, so a relative link would render broken. Guessing its filename from its title is the same failure mode this item is about β€” it was guessed, checked, and was wrong.)* **Follow-up, deliberately not done here:** file this against ADR 0158 **once 0158 is on `main`**. Padding a document at merge time with instances its author did not choose is its own defect, and the ADR's instances are attributed by convention. + +**Source:** PR #142 (BACKLOG #323 layer 3, SMTP TLS), 2026-08-02 β€” observed on that PR's CI and deliberately **not** fixed there, because it is unrelated to the SMTP change and widening the PR would have obscured it. **Provenance is itemised, not aggregated** β€” "produced by N sessions" is a confidence claim, and an unsourced one of exactly that shape is what this item is about. **Rates:** derived here exactly, reproduced independently by the #142 session at N=144, recomputed analytically by #344's owner; the 200k-trial simulation came with the originating report. **Sibling audit:** derived twice from different scopes and reconciled. **Instrument-first framing, the β‰₯6 rule, the leave-the-rest-alone scoping:** from the #142 session's review. **The "infinitely fast machine" discriminator:** from #344's owner. **The demand to falsify the banner gate before trusting its green:** from the #346 session. No claim here rests on a count of who agreed. **Verification of this filing's own instruments:** every probability recomputed by two methods that agree (`Fraction` and `-expm1(N*log1p(-x))`), the audit counts re-derived from the working tree rather than quoted, and `backlog_status_check.py` **falsified against this item** β€” a deliberately doubled banner made it fail at `BACKLOG.md:8429` naming #347, so its green is evidence that it can see this item rather than evidence it skipped it.