fix(security): match internal secret env names by shape, not exact suffix - #78330
fix(security): match internal secret env names by shape, not exact suffix#78330itskaism wants to merge 1 commit into
Conversation
…ffix
`_is_hermes_internal_secret` decides which dynamically-named Hermes-internal
secrets are stripped from child-process environments. It enumerated exact
suffixes — `_API_KEY` / `_BASE_URL` under `AUXILIARY_`, and `_SECRET` /
`_KEY` / `_TOKEN` under `GATEWAY_RELAY_` — so adjacent real spellings of the
same secret fell straight through to the child.
Measured by spawning a real child process with canary values and reading
the child's own environment, on upstream/main:
AUXILIARY_<TASK>_APIKEY leaked (no separator before KEY)
AUXILIARY_<TASK>_SECRET leaked (aux matched _API_KEY/_BASE_URL only)
AUXILIARY_<TASK>_TOKEN leaked (same)
AUXILIARY_<TASK>_PASSWORD leaked (same)
GATEWAY_RELAY_SECRET_V2 leaked (trailing qualifier defeats endswith)
GATEWAY_RELAY_PASSWORD leaked (PASSWORD absent from the suffix list)
All three spawn-env builders behave identically — `_make_run_env`,
`_sanitize_subprocess_env` and `hermes_subprocess_env` all delegate to this
predicate — so every terminal, Docker and subprocess path was affected,
including the `inherit_credentials=True` path a model-driving CLI receives.
Fix: classify on name shape. Under the two internal prefixes, any name
containing KEY / SECRET / TOKEN / PASSWORD is internal. This is not a new
policy — `code_execution_tool.py` already scrubs its child env by substring
over the same words (`_SECRET_SUBSTRINGS`), and its comments call out
`APIKEY` written without an underscore as a real-world spelling worth
catching. The predicate's own docstring cited that substring matching as
the reason this predicate exists; it just did not apply it. The two child
surfaces now agree.
False positives were the real risk, since the docstring promises non-secret
`GATEWAY_RELAY_*` routing hints stay visible. Two names in the tree contain
a credential-shaped word but are not credentials:
GATEWAY_RELAY_ROUTE_KEYS comma-separated relay route identifiers
GATEWAY_RELAY_IDP_TOKEN_URL the OIDC token endpoint URL, not a token
Both are enumerated as non-secret. Enumerating the safe side rather than the
secret side is deliberate: a secret list fails open on every name nobody
thought of, which is exactly the bug above, while a non-secret list fails
closed. `AUXILIARY_*_BASE_URL` stays explicitly matched — it carries no
credential-shaped word but may address a private endpoint.
Verified against a real child process, union across all four spawn-env
builders: 6 leaks before, 0 after, with 0 over-blocking — `GATEWAY_RELAY_URL`,
`GATEWAY_RELAY_PLATFORMS`, `GATEWAY_RELAY_PLATFORM`, `GATEWAY_RELAY_ENDPOINT`,
`GATEWAY_RELAY_ROUTE_KEYS`, `GATEWAY_RELAY_IDP_TOKEN_URL`,
`AUXILIARY_*_MODEL` and `AUXILIARY_*_PROVIDER` all still reach the child.
Tests assert the relation — credential-shaped names under these prefixes are
stripped while routing hints survive — rather than a frozen suffix list, so a
future edit that reintroduces enumeration fails them. They spawn real child
processes and are parameterized across all four builders. Confirmed RED on
unpatched main (6 failed / 4 passed, the 4 being the over-block guards, which
are correctly green in both states) and GREEN with this change.
|
Bound to the child-process credential-inheritance class under #83565 (#83565) — same bug class, different surface. internal-secret name-shape matching; Wave B. Currently blocked with no check-runs at head — needs CI before it can merge. The EPIC carries the live class table, dedup adjudication, and the dependency-driven merge order. |
|
suggesting changes The patch's whole-name credential substring matching removes valid non-secret plugin routing selectors when a task key contains a credential word. Security evidence: trust boundary: Model-authored child processes receive environments assembled by local, container, and non-terminal subprocess builders; process environment and plugin configuration are sources, the internal-secret predicate validates names, and the child environment is the sink. The changed predicate applies even when credentials are inherited. source/sink/invariant: The boundary must remove dynamic credentials under current-main reproduction: Current main removes credential-shaped internal names while preserving normal auxiliary selectors, including PR-head or patch-replay validation: The reviewed change removes intended credential variants and preserves standard routing hints, but also removes positive/negative cases: Credential-shaped names are absent from child environments; documented auxiliary and relay routing hints remain visible; a valid key-like selector containing residual bypass search: The shared sanitizer covers local, container, and non-terminal child-environment builders examined; no additional bypass was introduced by this change. reviewer validation: Independent source review, focused regression coverage, and child-environment checks confirmed both the intended credential removal and the plugin-routing regression. Not checked:
Signed: GPT-5.6-luna-max in Codex |
This builds on #77027 (@andrexibiza) and #77193 (@andrexibiza), which are
open against the same function and which I'd like to see land first — this is a
narrow follow-up to the part neither covers, not a competing rewrite. Happy to
rebase behind whichever merges, or to fold this into one of them if that's
easier for review.
The gap
_is_hermes_internal_secretmatches a fixed set of exact suffixes:_API_KEY/_BASE_URLforAUXILIARY_*, and_SECRET/_KEY/_TOKENforGATEWAY_RELAY_*. Adjacent real spellings fall straight through and reach thechild environment.
Measured end to end by spawning a real
/usr/bin/envthroughhermes_subprocess_env()with canary values — this is what a model-authoredshell command actually sees:
mainStill leaking after both open PRs:
(#77027 does close
GATEWAY_RELAY_PASSWORD, which is why its row is 4 and not 5.)The fix
Match by shape rather than by an enumerated suffix list: under the
AUXILIARY_andGATEWAY_RELAY_prefixes, a name containingKEY/SECRET/TOKEN/PASSWORDis internal.This is not a new policy — it's the rule
code_execution_tool.pyalreadyapplies, and the existing docstring says as much:
The terminal backend now uses the same rule as the code-execution backend.
Not over-blocking
The docstring explicitly protects non-secret routing hints, so those are the
important negative controls:
GATEWAY_RELAY_URLGATEWAY_RELAY_PLATFORMSPATH,HOMEAUXILIARY_VISION_API_KEYGATEWAY_RELAY_SECRET4/4 routing hints survive; the two already-caught controls are unchanged.
Tests
tests/tools/test_internal_secret_name_shape.pyasserts the relation — underthese prefixes a credential-shaped name is stripped while routing hints survive
— rather than freezing a suffix list, so it keeps working as new spellings
appear.
Verified RED before GREEN: applying only the test file to unpatched
mainfails6 of 10. With the fix, 10/10.
All three spawn builders are affected identically (
_make_run_env,_sanitize_subprocess_env,hermes_subprocess_env), so this covers theterminal, Docker and plain-subprocess paths.