From 2dc3b8c4e644981ade8fd3067dae982896bf9f18 Mon Sep 17 00:00:00 2001 From: Cipher208 <269750686+Cipher208@users.noreply.github.com> Date: Thu, 8 Oct 2026 01:34:51 +0200 Subject: [PATCH] fix(crypto): decide "is this encrypted" by decrypting, not by sniffing a byte MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_encrypted_blob asked a security question and answered it from one byte. The envelope is nonce(24) || ciphertext, so its first byte is the first byte of a random nonce — one of `{`, `[`, space or newline in 4 cases out of 256. The old test (`head not in (b"{", b"[", b" ", b"\n")`) therefore called real, readable ciphertext "plain JSON". Measured over 4096 draws: 69 misclassifications, 1.68%. That is the wrong direction for this predicate to fail — it turns a readable secret into a missing one. Detection now attempts decryption: a blob is encrypted when it decrypts under the given key, and nothing else counts. Truncated blobs, filler bytes, plain JSON, tampered ciphertext and blobs encrypted under a foreign key all report False, each for the right reason. Breaking, in shared/crypto.py: is_encrypted_blob(blob_head: bytes) becomes is_encrypted_blob(blob: bytes, master_key: bytes). A one-byte prefix cannot be authenticated, so the old signature was incapable of a correct answer; pass the whole blob. The path-based helpers — shared.master_key.is_encrypted_blob(path) and shared.saga.impl.crypto.is_encrypted_blob(path) — keep their signatures and are decrypt-based too. Nothing inside the package used any of them to decide anything: the saga read path already decrypted first, which is why this was a trap for callers rather than a live defect. They are exported utilities, so the trap was real for anyone outside. The bug had already been worked around rather than fixed, and the workaround is now gone. tests/test_shared/test_crypto_smoke.py redrew up to 32 blobs until one looked encrypted, because a single draw flaked about 1.6% of runs; it now asserts the opposite — it waits for the `{`-first nonce and requires detection to survive it. tests/test_secrets.py had been proving encryption with filler bytes (b"\xab\xcd" * 30), which only ever passed because anything not `{` counted as encrypted; it now writes a real envelope. Red/green on a single live blob: with a `{`-first nonce the old logic returns False for a blob that decrypts, the new one returns True. --- CHANGELOG.md | 5 +++ shared/crypto.py | 22 ++++++++++---- shared/master_key.py | 13 ++++---- shared/saga/impl/crypto.py | 16 +++++++--- tests/test_secrets.py | 8 +++-- tests/test_shared/test_crypto_smoke.py | 42 +++++++++++++++++++++----- 6 files changed, 80 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff7d84c8..174169b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ Format follows [Keep a Changelog](https://keepachangelog.com/). - **Five files left the tree without leaving the disk (2026-10-08).** `AUDIT_REPORT_20260629.md` audits 18 of what are now 731 files; `ROADMAP.md` opens by declaring itself legacy and frozen against reality; `.ai-memory.toml` and `.codegraph/` are local tool state; `.compose/context/gates.md` is a working note. Each was checked for live references first — none had any, beyond `.compose` in docstrings that turned out to mean the operator's `~/.compose`, not this one. They stay locally, named in `.gitignore`. `.codegraph/` is ignored from the root file now instead of by its own nested `.codegraph/.gitignore`, because git honours a nested ignore file and build backends do not — the same asymmetry that put a 5.6 GB venv into a source distribution in the previous commit. ### Fixed +- **`is_encrypted_blob` answered a security question by sniffing one byte, and was wrong 1.68% of the time (2026-10-08).** The envelope is `nonce(24) || ciphertext`, so its first byte is the first byte of a random nonce — which is `{`, `[`, a space or a newline in **4 cases out of 256**. The old test (`head not in (b"{", b"[", b" ", b"\n")`) therefore called real, readable ciphertext "plain JSON"; measured over 4096 draws: **69 misclassifications, 1.68%**. Detection now attempts decryption: a blob is encrypted when it decrypts under the given key, and nothing else counts. Truncated blobs, filler bytes, plain JSON, tampered ciphertext and blobs encrypted under a foreign key all correctly report `False`. + + **Breaking change**, `shared/crypto.py`: `is_encrypted_blob(blob_head: bytes)` → **`is_encrypted_blob(blob: bytes, master_key: bytes)`**. A one-byte prefix cannot be authenticated, so the old signature was incapable of a correct answer — pass the whole blob. The path-based helpers `shared.master_key.is_encrypted_blob(path)` and `shared.saga.impl.crypto.is_encrypted_blob(path)` keep their signatures and are now decrypt-based too. Nothing inside the package used any of these to decide anything (the saga read path already decrypted first); they are exported utilities, which is why the wrong answer mattered for callers rather than for us. + + The workaround this bug had accumulated is gone: `tests/test_shared/test_crypto_smoke.py` used to redraw up to 32 blobs until one *looked* encrypted, because a single draw flaked ~1.6% of runs. It now asserts the opposite — it waits for the `{`-first nonce and requires detection to survive it. `tests/test_secrets.py` had been proving encryption with filler bytes; it now writes a real envelope. - **Two assertions were a 1-in-256 flake, and they were asserting the wrong thing (2026-10-08).** `test_saga_crypto_coverage` checked `not data.startswith(b"{")` to prove a state file had been encrypted. The blob is `nonce(24) || ciphertext`, so its first byte is the first byte of a random nonce — `{` in 1 write out of 256. Measured over 4096 `encrypt_json` calls: **14 began with `{`** (0.34%), and one of them failed this suite's own pre-commit gate. Fixed by asserting behaviour instead of a byte: the encrypted file must fail a plain-JSON decode, `read_state` must round-trip it, and a second `read_state_legacy_or_encrypted` must return the same value *without* a rotation warning. Verified over 2048 fresh blobs, including the 5 that begin with `{` — zero failures. Recorded separately, not fixed: the exported helper `is_encrypted_blob` still sniffs a single byte and so calls 4 of 256 real ciphertexts plain JSON (measured **69/4096 = 1.68%**); the saga read path no longer uses it, so this is an API footgun rather than a live defect. - **One persona's base was collecting five other personas (2026-10-04).** Found preparing her history import: the source database is **multi-agent** — `agent_id` is `''` for the default agent and names every other agent sharing the base — and the agent config filtered only on `role`, so the daemon tailed every agent's chat into one persona's memory. The `role` column is the owner/persona axis *within* an agent; it does not name the agent, which is precisely the mistake. Measured: **168** foreign rows in her `l0_journal`, **53** in her dispatch log, **19** mentions of other personas' names in `core_memory` and `episodes`. diff --git a/shared/crypto.py b/shared/crypto.py index 59025016..a30666cd 100644 --- a/shared/crypto.py +++ b/shared/crypto.py @@ -47,12 +47,22 @@ def decrypt_json(blob: bytes, master_key: bytes) -> Any: return json.loads(box.decrypt(ct, nonce).decode("utf-8")) -def is_encrypted_blob(blob_head: bytes) -> bool: - """Check if data starts like an encrypted blob (heuristic). +def is_encrypted_blob(blob: bytes, master_key: bytes) -> bool: + """Return True when `blob` is a readable SecretBox envelope. - JSON starts with { or [. + Decided by attempting decryption, never by inspecting a byte. The envelope + is `nonce(24) || ciphertext`, so its first byte is the first byte of a + random nonce — `{` in roughly one draw out of 256. The previous first-byte + test consequently reported real ciphertext as "plain JSON" in 4 cases out + of 256 (measured: 69 of 4096 = 1.68%), which is the wrong way for a + predicate to fail: it turns a readable secret into a "missing" one. + + Pass the whole blob. A truncated one cannot authenticate and reports False. """ - if not blob_head: + try: + decrypt_json(blob, master_key) + except Exception: + # Too short, bad MAC, wrong key, or a plaintext that is not JSON: in + # every one of those cases this is not a readable envelope. return False - head = blob_head[:1] - return head not in (b"{", b"[", b" ", b"\n") + return True diff --git a/shared/master_key.py b/shared/master_key.py index ed8c7e1f..5e0fc55f 100644 --- a/shared/master_key.py +++ b/shared/master_key.py @@ -177,14 +177,15 @@ def decrypt_json(blob: bytes) -> Any: def is_encrypted_blob(path: Path) -> bool: - """Check if file is encrypted (not plain JSON). + """Return True when the file is a readable encrypted envelope. - Heuristic: encrypted blobs start with random 24 bytes (nonce), - JSON starts with { or [. + Reads the file and attempts decryption with this process's master key. The + previous version sniffed the first byte; because the envelope begins with a + random nonce, one byte in 256 looks like JSON (`{`, `[`, space, newline) and + real ciphertext was reported as plain JSON in 4 cases out of 256 (measured + 69/4096 = 1.68%). """ if not path.exists(): return False # Path is verified to be within app data dir by caller, safe. - with path.open("rb") as f: - head = f.read(1) - return bool(_is_encrypted_blob(head)) + return bool(_is_encrypted_blob(path.read_bytes(), _get_master_key())) diff --git a/shared/saga/impl/crypto.py b/shared/saga/impl/crypto.py index c3f5997a..dab3b697 100644 --- a/shared/saga/impl/crypto.py +++ b/shared/saga/impl/crypto.py @@ -14,7 +14,6 @@ from typing import TYPE_CHECKING, Any from shared.master_key import decrypt_json, encrypt_json -from shared.crypto import is_encrypted_blob as _is_crypto_encrypted_blob logger = logging.getLogger(__name__) @@ -29,12 +28,19 @@ def is_encrypted_blob(path: Path) -> bool: - """Check if file is encrypted (not plain JSON).""" + """Return True when the file is a readable encrypted envelope. + + Decrypt-based, matching shared.master_key.is_encrypted_blob. The previous + first-byte sniff called real ciphertext plain JSON whenever the nonce began + with `{`, `[`, a space or a newline — 4 bytes in 256 (measured 1.68%). + """ if not path.exists(): return False - with path.open("rb") as f: - head = f.read(1) - return bool(_is_crypto_encrypted_blob(head)) + try: + decrypt_json(path.read_bytes()) + except Exception: + return False + return True if TYPE_CHECKING: diff --git a/tests/test_secrets.py b/tests/test_secrets.py index 67f94ba1..5059aabf 100644 --- a/tests/test_secrets.py +++ b/tests/test_secrets.py @@ -27,12 +27,16 @@ def test_tampered_ciphertext_rejected(): def test_is_encrypted_blob(tmp_path: Path): - from features.secrets import is_encrypted_blob + from features.secrets import encrypt_json, is_encrypted_blob plain = tmp_path / "plain.json" enc = tmp_path / "enc.json" plain.write_text('{"a": 1}') - enc.write_bytes(b"\xab\xcd" * 30) + # A real envelope. This used to be filler bytes (b"\xab\xcd" * 30), which + # only ever passed because the old check looked at a single byte and + # anything not `{`/`[`/space/newline counted as encrypted. Detection now + # decrypts, so the file has to be genuinely encrypted to count. + enc.write_bytes(encrypt_json({"a": 1})) assert not is_encrypted_blob(plain) assert is_encrypted_blob(enc) diff --git a/tests/test_shared/test_crypto_smoke.py b/tests/test_shared/test_crypto_smoke.py index 6bf851c1..f2108c3c 100644 --- a/tests/test_shared/test_crypto_smoke.py +++ b/tests/test_shared/test_crypto_smoke.py @@ -19,18 +19,46 @@ def key() -> bytes: def test_roundtrip_dict(key): payload = {"k": "v", "n": 42, "nested": {"a": [1, 2, 3]}} - # 19.09: is_encrypted_blob is a first-byte heuristic, and the nonce is - # random — P(first byte looks like JSON) ≈ 4/256 per draw. A single draw - # flakes ~1.6% of gate runs. Redraw boundedly; the heuristic intent stays. + blob = crypto.encrypt_json(payload, key) + assert crypto.is_encrypted_blob(blob, key) + assert crypto.decrypt_json(blob, key) == payload + + +def test_detection_survives_a_nonce_that_looks_like_json(key): + """Regression: detection must not depend on the nonce's first byte. + + The envelope is nonce(24) || ciphertext and the nonce is random, so its + first byte is `{` in about 1 draw out of 256. The old first-byte test + pronounced such real ciphertext "plain JSON" — measured 69 of 4096 draws + (1.68%) — which made a readable secret look missing. This test waits for + that exact draw instead of avoiding it: the loop needs ~256 iterations, and + (255/256)**16384 ≈ 0, so it is deterministic in practice. + """ + payload = {"secret": "value"} blob = b"" - for _ in range(32): - blob = crypto.encrypt_json(payload, key) - if crypto.is_encrypted_blob(blob[:8]): + for _ in range(16384): + candidate = crypto.encrypt_json(payload, key) + if candidate[:1] == b"{": + blob = candidate break - assert blob != b"" and crypto.is_encrypted_blob(blob[:8]) + assert blob, "could not obtain a blob starting with '{' — test would be vacuous" + assert blob[:1] == b"{" # the very draw that used to break detection + assert crypto.is_encrypted_blob(blob, key) assert crypto.decrypt_json(blob, key) == payload +def test_detection_rejects_anything_that_is_not_a_readable_envelope(key): + assert not crypto.is_encrypted_blob(b'{"plain": true}', key) # plain JSON + assert not crypto.is_encrypted_blob(b"", key) # empty + assert not crypto.is_encrypted_blob(b"\x00" * 10, key) # shorter than nonce+MAC + assert not crypto.is_encrypted_blob(b"\xab\xcd" * 30, key) # filler, not an envelope + foreign = crypto.encrypt_json({"a": 1}, os.urandom(32)) + assert not crypto.is_encrypted_blob(foreign, key) # encrypted, but under another key + tampered = bytearray(crypto.encrypt_json({"a": 1}, key)) + tampered[-1] ^= 0xFF + assert not crypto.is_encrypted_blob(bytes(tampered), key) # MAC no longer matches + + def test_roundtrip_list_and_unicode(key): payload = ["строка", "второй", 3] blob = crypto.encrypt_json(payload, key)