From 1eaeb8e46b58cc45d65de9b67fd42de2fc2b600c Mon Sep 17 00:00:00 2001 From: Alpamys Date: Thu, 16 Jul 2026 18:39:50 +0500 Subject: [PATCH] =?UTF-8?q?fix(canary):=20harden=20the=20secret=20store=20?= =?UTF-8?q?=E2=80=94=200600=20+=20ESC-strip=20+=20entry=20cap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ECC security-review. Its core point stands: canary.py is BY ITS OWN DOCSTRING a secret store, yet it was the one new file skipping the two conventions this codebase applies to every other secret-bearing artifact. Both claims verified before fixing, not assumed. MEDIUM — manifest was world-readable. atomic_write_text does no chmod, so under the usual 022 umask the file lands 0644 and any local user on a shared box can read every canary without ever running `check`. 9+ existing modules (registry/store.py, adapter_sign.py, audit_log.py, ...) chmod 0600; canary.py now does too, via _harden_permissions. Proven by exercising the POSIX branch directly, since the test skips on Windows. MEDIUM — terminal escape injection. Verified: rich.markup.escape() passes a raw ESC byte straight through (it only neutralises [...]), and load_manifest accepted an OSC 52 sequence in a `secret`. The manifest is explicitly a shareable artifact, so a hostile one is realistic, and the injected sequence renders right above the MAJOR verdict it could obscure. Now stripped via _for_terminal, mirroring data_doctor.py / shrink.py. LOW — load_manifest had no entry cap. The 4 MB size cap alone still admits tens of thousands of minimal entries, each costing a model forward pass in check. Now bounded by the same _MAX_CANARIES the generators use. Also closed a gap the review noted in passing: `check --output` embeds every secret, so it is as sensitive as the manifest but carried no warning and no permissions. It now gets both. 262 tests green (1 POSIX-only skip on Windows), ruff clean. --- src/soup_cli/commands/data_canary.py | 27 +++++++++- src/soup_cli/utils/canary.py | 22 ++++++++- tests/test_v07136.py | 73 ++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 2 deletions(-) diff --git a/src/soup_cli/commands/data_canary.py b/src/soup_cli/commands/data_canary.py index 8671be1..3a0f792 100644 --- a/src/soup_cli/commands/data_canary.py +++ b/src/soup_cli/commands/data_canary.py @@ -22,6 +22,7 @@ from rich.table import Table from soup_cli.data.loader import load_raw_data from soup_cli.utils.canary import ( + _harden_permissions, build_canary_report, canary_report_to_dict, canary_rows, @@ -41,6 +42,22 @@ app = typer.Typer( no_args_is_help=True, help="Dataset canaries (memorization probe)." ) +# Strip C0/DEL before any manifest-derived string reaches the terminal. +# rich.markup.escape() only neutralises Rich's own [...] tag syntax — a raw +# ESC byte survives it. The manifest is explicitly a shareable artifact, so +# a hostile one is a realistic input: an OSC 52 / cursor sequence in a +# `secret` could spoof the title bar or obscure the MAJOR verdict printed +# right below the table. Mirrors commands/data_doctor.py + commands/shrink.py. +# --output JSON is unaffected: json.dumps already \\u00XX-escapes these. +_CONTROL_STRIP_TABLE = { + i: None for i in range(0x20) if i not in (0x09, 0x0A, 0x0D) +} +_CONTROL_STRIP_TABLE[0x7F] = None + + +def _for_terminal(text: str) -> str: + return text.translate(_CONTROL_STRIP_TABLE) + # Fixed seed for the control draw: controls are a null distribution, not a # secret, so reproducibility is the useful property here. _CONTROL_SEED = 12345 @@ -210,7 +227,7 @@ def check( "nan" if math.isnan(exposure.loss) else f"{exposure.loss:.4f}" ) table.add_row( - escape(exposure.secret.strip()), + escape(_for_terminal(exposure.secret.strip())), loss_text, f"{exposure.percentile * 100:.1f}%", "[red]YES[/]" if exposure.memorized else "no", @@ -220,10 +237,18 @@ def check( console.print(f"[{colour}]Verdict: {report.verdict}[/]") if output is not None: + # The report embeds every secret, so it is as sensitive as the + # manifest and gets the same 0600 + warning. `insert` warns about + # the manifest; without this the report would be the quiet leak. atomic_write_text( json.dumps(canary_report_to_dict(report), indent=2), output ) + _harden_permissions(output) console.print(f"[green]Report written:[/] [bold]{escape(output)}[/]") + console.print( + "[yellow]The report lists the canary secrets — treat it like the " + "manifest and do not commit it.[/]" + ) if report.verdict == "MAJOR": raise typer.Exit(2) diff --git a/src/soup_cli/utils/canary.py b/src/soup_cli/utils/canary.py index 7dfa6ed..aa91ad2 100644 --- a/src/soup_cli/utils/canary.py +++ b/src/soup_cli/utils/canary.py @@ -132,7 +132,10 @@ def write_manifest(canaries: Sequence[Canary], path: str) -> str: """Persist the secrets. THIS FILE IS THE SENSITIVE ARTIFACT. Anyone holding it can reproduce the canaries, so it must not be - committed alongside the dataset it protects. + committed alongside the dataset it protects, and it is chmod-600'd on + POSIX: under the usual 022 umask a generic write lands 0644, letting + any local user on a shared box read every canary without ever running + ``check``. Mirrors registry/store.py, adapter_sign.py, audit_log.py. """ safe = enforce_under_cwd_and_no_symlink(str(path), "manifest") payload = { @@ -145,9 +148,19 @@ def write_manifest(canaries: Sequence[Canary], path: str) -> str: ], } atomic_write_text(json.dumps(payload, indent=2), safe) + _harden_permissions(safe) return safe +def _harden_permissions(path: str) -> None: + """Best-effort 0600 on a secret-bearing file (POSIX; no-op on Windows).""" + if os.name != "nt": + try: + os.chmod(path, 0o600) + except OSError: + pass + + def load_manifest(path: str) -> tuple[Canary, ...]: """Read a canary manifest written by :func:`write_manifest`.""" safe = enforce_under_cwd_and_no_symlink(str(path), "manifest") @@ -165,6 +178,13 @@ def load_manifest(path: str) -> tuple[Canary, ...]: entries = payload.get("canaries") if not isinstance(entries, list): raise ValueError("manifest 'canaries' must be a list") + if len(entries) > _MAX_CANARIES: + # `check` runs one model forward pass per entry; the 4 MB size cap + # alone still admits tens of thousands of minimal entries. + raise ValueError( + f"too many canaries in manifest ({len(entries)}); " + f"max {_MAX_CANARIES}" + ) out = [] for entry in entries: if not isinstance(entry, dict): diff --git a/tests/test_v07136.py b/tests/test_v07136.py index bbdcb58..b5af13b 100644 --- a/tests/test_v07136.py +++ b/tests/test_v07136.py @@ -1430,6 +1430,40 @@ class TestCanaryManifest: with pytest.raises(ValueError, match="too large"): load_manifest("big.json") + @pytest.mark.skipif( + __import__("os").name == "nt", reason="POSIX permissions only" + ) + def test_manifest_is_not_world_readable(self, tmp_path, monkeypatch): + """The manifest IS the secret. On a shared box a 0644 file lets any + local user read every canary without ever running check.""" + import os + import stat + + from soup_cli.utils.canary import generate_canaries, write_manifest + + monkeypatch.chdir(tmp_path) + write_manifest(generate_canaries(count=2, seed=0), "m.json") + mode = stat.S_IMODE(os.stat("m.json").st_mode) + assert not (mode & stat.S_IRGRP), f"group-readable: {oct(mode)}" + assert not (mode & stat.S_IROTH), f"world-readable: {oct(mode)}" + + def test_load_manifest_caps_entry_count(self, tmp_path, monkeypatch): + """A manifest packed with entries would run unbounded forward passes.""" + from pathlib import Path + + from soup_cli.utils.canary import _MAX_CANARIES, load_manifest + + monkeypatch.chdir(tmp_path) + entries = [ + {"carrier": "c", "secret": f"s{i}"} + for i in range(_MAX_CANARIES + 1) + ] + Path("big.json").write_text( + json.dumps({"canaries": entries}), encoding="utf-8" + ) + with pytest.raises(ValueError, match="too many canaries"): + load_manifest("big.json") + @pytest.mark.skipif( not hasattr(__import__("os"), "symlink"), reason="POSIX only" ) @@ -1969,6 +2003,45 @@ class TestDataCanaryCli: assert res.exit_code == 1 assert "could not load" in _clean(res.output).lower() + def test_esc_bytes_from_a_manifest_are_stripped(self, tmp_path, monkeypatch): + """rich escape() neutralises [...] but NOT raw ESC bytes. + + The manifest is a shareable artifact ("anyone holding it can + reproduce the canaries"), so a hostile one is a realistic input. An + OSC 52 sequence in a secret would reach the terminal raw and could + obscure the MAJOR verdict printed right below the table. + """ + from pathlib import Path + + from typer.testing import CliRunner + + from soup_cli.cli import app + from soup_cli.commands import data_canary as cmd + + monkeypatch.chdir(tmp_path) + evil = "7c3f\x1b]52;c;ZXZpbA==\x07-9a21" + Path("m.json").write_text( + json.dumps({"canaries": [{"carrier": "c", "secret": evil}]}), + encoding="utf-8", + ) + monkeypatch.setattr( + cmd, "_load_pair", lambda *a, **k: (object(), object(), "cpu") + ) + monkeypatch.setattr( + cmd, "compute_pair_losses", + lambda model, tok, pairs, **kw: ( + [5.0] + [float(i) for i in range(len(pairs) - 1)] + ), + ) + res = CliRunner().invoke( + app, ["data", "canary", "check", "--manifest", "m.json", + "--base", "fake/model", "--controls", "8"], + ) + assert res.exit_code == 0, (res.output, repr(res.exception)) + assert "\x1b" not in res.output, ( + "a raw ESC byte from the manifest reached the terminal" + ) + def test_check_missing_manifest(self, tmp_path, monkeypatch): from typer.testing import CliRunner