mirror of https://github.com/razor-ai/soup.git
fix(canary): harden the secret store — 0600 + ESC-strip + entry cap
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.
This commit is contained in:
parent
b0ac95ecdc
commit
1eaeb8e46b
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue