diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 70835567..2c416c7e 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -3066,7 +3066,7 @@ Honestly bounded: **this is build-time only.** No PHI path, no running-engine su ## 337. handler-security lint: `getattr` indirection and the undecorated helper -> πŸ”’ **Filed 2026-08-01 β€” not started.** Value **3/10** Β· Difficulty **3/10** Β· _fill-in_. `_AMBIENT_BARE_NAMES` (`checks.py:476`) still matches a literal name chain and `checks.py` contains no `getattr` resolution at all, and the rule loop still bails on `_message_fn_decorator(node) is None` (`:937`) so the `__transforms.py` helper CONNECTIONS.md steers PHI handling into is never opened β€” but the lint is advisory unless an adopter opts into `--strict-handler-security`, and evading it reaches neither the DEK nor the audit chain in either sandbox posture; ~15 lines splicing a constant into `_dotted_call_name` plus a `phi-to-log` widening that must be recalibrated against the two shipped sample helpers before it lands. +> βœ… **Done 2026-08-05 (#337).** Value **3/10** Β· Difficulty **3/10**. Both recall gaps in `_check_handler_security` (`checks.py`) are closed and pinned. A constant `getattr(mod, "name")` indirection now resolves in `_dotted_call_name`, so `getattr(os, "system")(...)` is flagged for `ambient-authority` (the shared resolver also flags a `getattr(time, "time")()` wall-clock read for `impure-transform`); and `phi-to-log` now scans undecorated `_*` transform helpers keyed on the first positional parameter, while `impure-transform` stays decorated-scope so the shipped `_pdf_mdm_transforms.py` ingest-time timestamp fallback stays clean. New `tests/test_checks_handler_security.py` cases cover both, and the change was proven green against `samples/config` before landing. Still an advisory-by-default filter β€” an evasion reaches neither the DEK nor the audit chain in either sandbox posture; ADR 0144 amended, to be re-scored upward when ADR 0147 (OS-level default-deny) lands. **Cluster:** Security & Compliance. **Priority:** P3. **Verdict:** build (small). **Severity:** low. diff --git a/docs/adr/0144-security-lint-gate-over-admin-authored-router-handler-config.md b/docs/adr/0144-security-lint-gate-over-admin-authored-router-handler-config.md index d950ed7a..83d37325 100644 --- a/docs/adr/0144-security-lint-gate-over-admin-authored-router-handler-config.md +++ b/docs/adr/0144-security-lint-gate-over-admin-authored-router-handler-config.md @@ -66,7 +66,10 @@ the `ruff`/`mypy` run-if-installed convention in escalation. logger-shaped receiver** (`log`/`logger`/`logging`/`getLogger(...)`; `.debug` excluded) β€” the CLAUDE.md Β§9 "never log full bodies at INFO+" rule. The logger-receiver gate keeps a FHIR/ACK/ validation builder (`outcome.error(...)`, `warnings.warn(...)`) from being mistaken for a sink. - Scoped to `@router`/`@handler` **bodies** (not the signature/defaults). + Scans **every** function body β€” a decorated `@router`/`@handler` or an undecorated `_*` transform + helper β€” keying on the first positional parameter as the message symbol (not the signature/defaults). + Widened from decorated-scope by BACKLOG #337 (see the 2026-08-05 amendment); `impure-transform` + stays `@router`/`@handler`-scoped. - **`unsafe-db-lookup`** β€” a **non-constant** interpolation (an f-string with a value, `+`/`%` with a variable operand, `.format(...)`) flowing into the `db_lookup`/`fhir_lookup` **statement/query** argument (2nd positional or `statement=`/`query=`). A pure-literal concat folds to a constant and is @@ -187,12 +190,24 @@ not a boundary) and recorded here rather than chased into diminishing-returns pr log.info(body)` β€” **recovered by the opt-in Semgrep taint leg (Increment 3, Control B)**; still a false-negative of the stdlib `check` itself. - **Aliased-import evasion** (`from subprocess import run as r; r(...)`) is out of scope of the AST scan β€” - **recovered by the Semgrep leg (Increment 3)**, which resolves the import alias. `getattr(os, "system")` - remains a false-negative; the lint is a fallible-author guardrail, not a malicious-bypass boundary - (that is ADR 0087's job). -- **Decorated-scope only** for `phi-to-log`/`impure-transform`: PHI logged or a wall-clock read inside an - **undecorated** helper is not scanned (the trade that keeps the shipped `_pdf_mdm_transforms.py` - timestamp fallback clean). + **recovered by the Semgrep leg (Increment 3)**, which resolves the import alias. A *constant* + `getattr(mod, "name")` indirection is now resolved in `_dotted_call_name` (BACKLOG #337), so + `getattr(os, "system")(...)` **is** flagged (and the same splice flags a `getattr(time, "time")()` + wall-clock read). The remaining false-negatives are a *dynamic* `getattr(os, name)`, a + `globals()["os"]` / subscript indirection, and the opt-in Semgrep leg (which still carries no `getattr` + pattern). The lint is a fallible-author guardrail, not a malicious-bypass boundary (that is ADR 0087's job). +- **`phi-to-log` reaches undecorated `_*` helpers** (BACKLOG #337): the decompose-by-role convention + (`docs/CONNECTIONS.md` Β§"Decomposing by role", #226) steers field-level PHI handling into undecorated + `__transforms.py` helpers, so `phi-to-log` scans **every** function body β€” decorated or not β€” + keying on the first positional parameter as the message symbol. Residuals: an `async def` handler is + still scanned by neither rule (`_message_fn_decorator` is `FunctionDef`-only β€” a third gap this lane + reported but did not scope), and a helper whose first parameter is *not* the message is an accepted + advisory FP/FN tail. +- **`impure-transform` stays decorated-scope only**: a wall-clock read inside an **undecorated** helper is + not scanned β€” the trade that keeps the shipped `_pdf_mdm_transforms.py` ingest-time timestamp fallback + clean (an undecorated `time.gmtime(ingest_time or time.time())`). This, not `phi-to-log`, is the actual + justification for the decorated-scope gate; widening `impure-transform` would re-introduce that false + positive and red the samples gate. - **Non-recursive** (`glob("*.py")`, no subdirs) β€” mirrors the existing `_check_raise_fstring` convention. - **Trusted-identifier concat** (`"select … from " + TABLE`, PHI parameterized) still nudges β€” SQL cannot parameterize an identifier, so the concatenation reminder is intentional (silence it with a literal @@ -211,6 +226,23 @@ not a boundary) and recorded here rather than chased into diminishing-returns pr resolve the distribution behind an import. Distinct typosquat/hallucinated names are caught; same-name substitution is ADR 0087's boundary, not this lint's. +> **AMENDED 2026-08-05 (BACKLOG #337) β€” two recall gaps closed; the severity of a miss is unchanged.** +> A *constant* `getattr(mod, "name")` indirection now resolves in `_dotted_call_name`, so +> `getattr(os, "system")(...)` is flagged for `ambient-authority` (the shared resolver also flags a +> `getattr(time, "time")()` wall-clock read for `impure-transform`), and `phi-to-log` now reaches +> undecorated `_*` transform helpers. The severity framing is deliberately conservative and holds in +> **both** sandbox postures: the lint is advisory and pre-deployment, and an evasion reaches only **host** +> actions the ADR 0087 sandbox does not confine. `DEFAULT_FORBIDDEN_MODULES` (`pipeline/sandbox.py`) blocks +> `socket`/`ssl`/`asyncio`/`multiprocessing`, the I/O-bearing `messagefoundry.*` subpackages and +> `cryptography`, but **not `os` or `subprocess`** β€” ADR 0087 confines the **address space**, not the +> **host**. So under `[sandbox].mode=off` the author already holds in-process execution, and under +> `mode=subprocess` an evasion still reaches neither the DEK nor the audit chain: no PHI-exposure path and +> no runtime-behaviour change in either posture ("a filter, not a fix"). This should be **re-scored upward +> when [ADR 0147](0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md) +> lands** (OS-level default-deny; *Proposed*, no code today), at which point the lint becomes load-bearing +> for exactly the class OS confinement is meant to close. MessageFoundry is a not-deployed beta, so a miss +> "would slip past a deploying site's CI", never "PHI is exposed". + ## Increment 3 (built) All four Increment-3 follow-ons shipped: diff --git a/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md b/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md index 8166a731..a016dd35 100644 --- a/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md +++ b/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md @@ -152,7 +152,7 @@ spaces collide, so every foreign ID below is prefixed. | `tests/test_last_resort.py` (104) | ASVS 16.5.4 at unit level: the asyncio loop handler **and** `sys.excepthook` both route an unhandled exception through `safe_exc` β€” exception type preserved, the planted PHI fragment absent from the log record β€” `KeyboardInterrupt` passes to `sys.__excepthook__` untouched, and a framed MLLP listener survives a raising handler (connection drops, server stays up). What it does **not** prove: that the handlers are installed in a real serving process, or that the redacted record stays clean across *every* configured sink (SEC-73). | | `tests/test_cert_cli.py` (446) + `tests/test_cert_expiry.py:308-312` | `messagefoundry/pki.py` exercised through the `cert` CLI: in-memory `.pfx` bundles built with `pkcs12.serialize_key_and_certificates`, `read_cert_facts` staying best-effort on an unparseable extension rather than sinking the cert, and `pki._SECONDS_PER_DAY` pinned equal to `pipeline/cert_expiry.py`'s. No adversarial PKCS#12 corpus and no key-material-egress assertion (SEC-75/SEC-76). | | `tests/test_anon_core.py` (326) + `test_anon_integration.py` + `test_anon_parity.py` | ADR 0030 end to end: salt-keyed determinism + weak-salt reject, the two-layer rule model, structure-preserving surrogates, FREETEXT blunt redact + OBX-2 gating, MSH-separator whole-field write, fail-closed no-MSH refusal, field-anchored site-code scrub, engine↔tee byte-identical parity. | -| `tests/test_checks_handler_security.py` (987) | ADR 0144 lint: all five rule families, decorated-scope vs whole-file scoping, the operator allow-root escape, and the advisory-vs-strict `CheckResult` shape. | +| `tests/test_checks_handler_security.py` (1,079) | ADR 0144 lint: all five rule families across their three scoping regimes (`impure-transform` decorated-scope, `phi-to-log` every-function-body per BACKLOG #337, the rest whole-module), the operator allow-root escape, and the advisory-vs-strict `CheckResult` shape. | | `.github/workflows/security.yml` `semgrep` job + `scripts/ci/assert_semgrep_handler_taint.py` | The packaged `handler-security.yml` rules are syntactically valid, the two recovered false-negatives fire exactly as annotated, `# ok` cases stay clean, and `samples/config` scans clean under `--error`. | | `tests/test_sandbox.py` (293) | ADR 0087: off-mode byte-identical in-process call, worker bootstrap, forbidden-import guard, resource caps, fail-closed `SandboxError` routing to ERROR/dead-letter post-ACK, and the `db_lookup`/`fhir_lookup` refusal. | | `tests/test_secrets_dpapi.py` (125) + `test_secretprovider.py` + `test_backup_crypto.py` | DPAPI machine- and user-scope round-trip (Windows-only), SecretProvider `none`/`env`/`vault` dispatch with fail-closed ref-without-provider and empty-value, and `.mfbak` chunked-AEAD tamper detection. | diff --git a/messagefoundry/checks.py b/messagefoundry/checks.py index be404055..35070d59 100644 --- a/messagefoundry/checks.py +++ b/messagefoundry/checks.py @@ -574,14 +574,50 @@ def _message_fn_decorator( return None +def _const_getattr_target(node: ast.expr) -> tuple[str, ast.expr] | None: + """For a ``getattr(receiver, "")`` call, return ``(attr_name, receiver)`` so the chain + resolver can splice the constant attribute in β€” turning ``getattr(os, "system")`` into ``os.system``. + + Returns None for anything else: a non-``getattr`` call, a *dynamic* attribute + (``getattr(os, name)`` β€” a variable, not a str constant), or a ``*args`` splat in the first two + positions (which makes positional indexing meaningless). Only a statically-known constant attribute + name is spliced; a dynamic indirection stays unresolvable, so benign reflection is not flagged.""" + if not ( + isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "getattr" + ): + return None + if len(node.args) < 2 or any(isinstance(a, ast.Starred) for a in node.args[:2]): + return None + attr = node.args[1] + if isinstance(attr, ast.Constant) and isinstance(attr.value, str): + attr_name: str = attr.value # bound locally for mypy-strict narrowing + return attr_name, node.args[0] + return None + + def _dotted_call_name(func: ast.expr) -> str | None: """Reconstruct a dotted Name/Attribute chain (``os.path.join``) from a call's ``func``, or None - when it is not a pure Name/Attribute chain (e.g. the receiver is itself a call or subscript).""" + when the chain does not bottom out in a plain Name (e.g. the receiver is a subscript or a + non-``getattr`` call). + + A *constant* ``getattr`` indirection is spliced: ``getattr(os, "system")`` resolves to ``os.system`` + (and ``getattr(pkg, "sub").fn`` to ``pkg.sub.fn``). A *dynamic* ``getattr(os, name)``, a subscript + (``globals()["os"]``), or any other call-shaped receiver stays unresolvable β†’ None. Each iteration + strictly descends a finite AST (Attribute β†’ its value, or a getattr splice β†’ its receiver), so the + loop always terminates.""" parts: list[str] = [] cur: ast.expr = func - while isinstance(cur, ast.Attribute): - parts.append(cur.attr) - cur = cur.value + while True: + if isinstance(cur, ast.Attribute): + parts.append(cur.attr) + cur = cur.value + continue + target = _const_getattr_target(cur) + if target is None: + break + attr_name, receiver = target + parts.append(attr_name) + cur = receiver if isinstance(cur, ast.Name): parts.append(cur.id) return ".".join(reversed(parts)) @@ -909,9 +945,10 @@ def _check_handler_security( that wants a hard gate on its own CI. The runtime half is the opt-in ADR 0087 sandbox. Mirrors :func:`_check_raise_fstring`: static ``ast`` only (never imports/executes the config), globs ``*.py`` under ``config_dir`` (helpers included), and skips a broken/unreadable file (``validate`` - reports those) so it never crashes the gate. ``phi-to-log`` and ``impure-transform`` are scoped to - ``@router``/``@handler`` bodies (so an undecorated helper's wall-clock fallback is not a false - positive); the other three scan the whole module.""" + reports those) so it never crashes the gate. ``impure-transform`` is scoped to ``@router``/``@handler`` + bodies (so an undecorated helper's wall-clock fallback is not a false positive); ``phi-to-log`` scans + every function body β€” decorated or an undecorated ``_*`` transform helper β€” keying on the first + positional parameter as the message symbol (BACKLOG #337); the other three scan the whole module.""" base = Path(config_dir) if not base.is_dir(): return CheckResult( @@ -946,19 +983,25 @@ def _check_handler_security( file_hits.append((node.lineno, "unsafe-db-lookup")) if _ambient_authority_hit(node): file_hits.append((node.lineno, "ambient-authority")) - # Decorated-scope rules β€” phi-to-log + impure-transform, over each router/handler's own body + # Per-FunctionDef body rules β€” phi-to-log + impure-transform, over each function's own body # only (not nested defs, not the signature), so each call is scanned once with its own message - # symbol and an import-time default arg is never mistaken for per-message impurity. + # symbol and an import-time default arg is never mistaken for per-message impurity. phi-to-log + # runs over EVERY function body β€” a decorated @router/@handler AND an undecorated `_*` transform + # helper (BACKLOG #337, CLAUDE.md Β§9; the decompose-by-role convention steers PHI handling into + # `_*` helpers, so a lint that only saw decorated bodies skipped the file the convention names), + # keying on the first positional parameter as the message symbol. impure-transform stays + # decorated-scope only, so the shipped `_pdf_mdm_transforms.py` ingest-time wall-clock fallback + # in an undecorated helper is not a false positive (the trade ADR 0144 records). for node in ast.walk(tree): - if _message_fn_decorator(node) is None: + if not isinstance(node, ast.FunctionDef): continue - assert isinstance(node, ast.FunctionDef) # narrowed by _message_fn_decorator + decorated = _message_fn_decorator(node) is not None params = node.args.posonlyargs + node.args.args msg_sym = params[0].arg if params else None for sub in _body_calls(node): if msg_sym is not None and _phi_to_log_hit(sub, msg_sym): file_hits.append((sub.lineno, "phi-to-log")) - if _impure_transform_hit(sub, imported): + if decorated and _impure_transform_hit(sub, imported): file_hits.append((sub.lineno, "impure-transform")) hits += [f"{path.name}:{lineno} [{rule}]" for lineno, rule in sorted(file_hits)] if not hits: diff --git a/tests/test_checks_handler_security.py b/tests/test_checks_handler_security.py index 8dd42f46..ddad2cd9 100644 --- a/tests/test_checks_handler_security.py +++ b/tests/test_checks_handler_security.py @@ -596,6 +596,65 @@ def h(msg): return Send("OB", msg) """, ), + # --- BACKLOG #337: constant getattr indirection is spliced in _dotted_call_name ------------- + # A constant getattr(mod, "name") indirection now resolves (getattr(os, "system") -> os.system), + # so it is flagged like the bare dotted call; a dynamic getattr(os, name) stays unresolved. + ( + "amb_pos_getattr_os_system", + "ambient-authority", + True, + """ + import os + + @handler("h") + def h(msg): + getattr(os, "system")("id") + return Send("OB", msg) + """, + ), + ( + "amb_neg_getattr_dynamic_attr", + "ambient-authority", + False, + """ + import os + + @handler("h") + def h(msg): + name = msg["PID-3.1"] + getattr(os, name)("x") + return Send("OB", msg) + """, + ), + # The splice feeds impure-transform too (shared _dotted_call_name), still import-gated on `time`. + ( + "impure_pos_getattr_time_time", + "impure-transform", + True, + """ + import time + + @handler("h") + def h(msg): + stamp = getattr(time, "time")() + return Send("OB", msg) + """, + ), + # --- BACKLOG #337: phi-to-log widened to undecorated `_*` transform helpers ----------------- + # The decompose-by-role convention steers PHI handling into undecorated helpers, so phi-to-log + # must reach them; it still keys on the first positional parameter as the message symbol. + ( + "phi_pos_undecorated_helper_logs_msg", + "phi-to-log", + True, + """ + import logging + log = logging.getLogger(__name__) + + def apply(msg): + log.info("transforming %s", msg.raw) + """, + ), ] @@ -980,6 +1039,39 @@ def test_allow_root_still_flags_its_ambient_use(tmp_path: Path) -> None: assert "unvetted-import:httpx" not in result.detail +# --- BACKLOG #337: widening phi-to-log must NOT widen impure-transform, and must still discriminate. +def test_widened_phi_to_log_does_not_widen_impure_transform(tmp_path: Path) -> None: + # The exact shipped `_pdf_mdm_transforms.py` ingest-time wall-clock fallback, in an UNDECORATED + # helper. phi-to-log now reaches undecorated helpers (#337), but impure-transform stays + # decorated-scope, so this timestamp fallback must not be flagged β€” it has no PHI log sink and the + # impure rule must not widen (the trade ADR 0144 records; widening it would red the samples gate). + (tmp_path / "_pdf_mdm.py").write_text( + "import time\n\n" + "def build(pdf, ingest_time=None):\n" + " return time.strftime(\n" + ' "%Y%m%d%H%M%S",\n' + " time.gmtime(ingest_time if ingest_time is not None else time.time()),\n" + " )\n", + encoding="utf-8", + ) + assert _check_handler_security(tmp_path).skipped + + +def test_phi_to_log_undecorated_helper_non_message_local_is_clean(tmp_path: Path) -> None: + # The widened phi-to-log still keys on the message symbol (first positional param): an undecorated + # helper that logs a NON-message local (count) must not flag β€” proving the rule discriminates and + # is not "any INFO+ log call in any function". + (tmp_path / "_helper.py").write_text( + "import logging\n" + "log = logging.getLogger(__name__)\n\n" + "def build(msg):\n" + " count = 5\n" + ' log.info("c=%s", count)\n', + encoding="utf-8", + ) + assert _check_handler_security(tmp_path).skipped + + def test_real_samples_config_is_clean() -> None: # False-positive calibration: the real shipped Router/Handler samples must not trip the lint. samples = Path(__file__).resolve().parents[1] / "samples" / "config"