sec-audit-fixes: CRITICAL & HIGH findings from the security sweep - #301
Merged
Conversation
dinstein
marked this pull request as ready for review
August 10, 2026 06:34
4 tasks
checkContainer only recognised single short flags (-i, -t) and the -v attached shorthand. A combined cluster like -it matched none of those, so it fell to the "assume it takes a value" branch and swallowed the FOLLOWING argument: `docker run -it --privileged img` consumed --privileged as -it's value, and `docker run -it -v /:/host img` consumed the bind mount, so the container-escape checks never saw either. A cluster ending in a value-taking short (`-itv /:/host`) hid a mount the same way, because checkContainerFlag was handed the key "-itv" and matched no case. Decompose a bare single-dash cluster the way docker/pflag do: leading boolean shorts (the "-x" entries already in containerBoolFlags) consume nothing, and the first value-taking short ends the cluster and takes the value, which is then judged under its "-x" key. An unrecognised letter is treated as value-taking rather than blocked, matching this layer's fail-open shape-check direction; the fixed failure was the opposite direction, a cluster swallowing the escape flag after it. The -v attached shorthand is now covered by the same path. Verified with a new table in containerescape_test.go covering -it/-itd/-itv hiding --privileged and bind mounts, plus benign clusters and attached values that must still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
DiscoverFromResource fetched RFC 9728 protected-resource metadata from
candidates the resource server (and its unauthenticated 401 pointer) names,
then overwrote the token's RFC 8707 resource/audience with prm.Resource
unconditionally. Only the authorization-server issuer was validated. A
malicious downstream could therefore publish {resource: https://victim,
authorization_servers: [the victim's real AS]}: the issuer match passes, the
user completes login, and a token minted for audience=victim is stored under
the malicious server's id and sent to it — a confused-deputy / cross-audience
redirection RFC 9728 resource-identifier validation exists to prevent.
Adopt prm.Resource only when it names the server actually contacted: its
canonical resource, or the contacted URL's bare origin (the origin-root shape
a deployment may legitimately publish, where the resource id is the origin
and the MCP endpoint sits under a path). On mismatch or an unparseable value,
keep the honest canonical value already computed from the contacted URL — a
token bound to the server we reached and sent to it is not a confused deputy.
This is distinct from the documented resource-origin issuer gap, which is a
different path and field.
Verified: a cross-origin advertised resource is refused while the origin-root
shape is still accepted; the existing exact-match and resource-origin tests
keep passing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
…t open Config.ScopeLayers — the only carrier of an HTTP agent token's server allowlist and profile pin — was wired only inside `if g.store != nil`. When registry.Open fails (store==nil), the scope resolver was never built, so currentScope returned nil (the allow-all baseline) and the regOK=false path served the full on-disk tool cache. A restricted token therefore saw every cached server's catalog: names, descriptions and input schemas of servers outside its allowlist. Nothing connects without a store, so tools/call still answers busy, but the disclosure crosses the per-credential boundary. When a credential brought narrowing layers but no store could be opened, fail the scope closed instead of widening: currentScope now returns an empty EffectiveScope (nothing visible) under a new scopeFailClosed flag, so BOTH the pipeline scope gate and the discovery surface (tools/list, search_tools) hide every server. Isolation a config claims must be delivered or refused, never degraded to the uncredentialed baseline. The no-credential local path (ScopeLayers==nil) is untouched and still serves the cache. Verified: a restricted token gets nothing from search_tools against a broken registry with a cached secret server, while an uncredentialed session still sees the cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
connDiagnosis feeds two very different consumers. serverStates() goes to the daemon control link (a trusted, same-uid peer), but the string connDiagnosis builds is appended to the `status` meta-tool reply that crosses back to an MCP caller. It reported EVERY applied server's id and its raw connect detail, so a scope-restricted token learned the existence of servers outside its allowlist and read their failure text — which for an HTTP downstream can quote the URL query, i.e. a credential. connDiagnosis now takes the caller's effective scope and reports only servers visible in it (nil scope = a local session with no authority, which still sees all; empty scope hides everything), and it no longer renders the raw detail: a server id plus a needs-auth hint is the actionable part, while the full detail stays on the trusted daemon path via the unchanged serverStates(). Verified: a token scoped to one server sees neither the other server's id nor any credential-bearing detail, the in-scope failure still reports with its needs-auth hint, and a nil-scope session sees both servers but no raw detail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
…ting Two gaps let an imported skill corrupt a shared client rule file. The import sentinel-marker guard checked only name, description and body, but renderSkillBody also embeds the skill Version and every bundled file path into the managed block — and Version is attacker-controlled frontmatter. And applySentinel wrote the file with atomicWrite BEFORE findBlock verified the block read back, so a smuggled marker (which makes findBlock reject the duplicate) still committed a corrupt, unremovable block to a shared file and only then errored. Extend the guard (scanCarriesSentinelMarker) to name, description, body, Version and every file path, at both the Add and Update import sites; it matches the marker prefix, so a marker naming any skill id is refused, keeping the poison out of the library entirely. And reorder applySentinel to run findBlock before EnsureDir/backup/atomicWrite, so a body that does not read back leaves the target byte-for-byte unchanged. Verified: import refuses a sentinel in the version and in a bundled file path; applySentinel refuses a marker-bearing render and does not modify the target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
The SSRF story in security.md described DialControl as the real line of defense against a private-destination dial, but never noted that an environment proxy routes around it — a fact already recorded in foundation.md and dataplane.md as a gap owed a decision. A reader auditing SSRF from this file would conclude the screen is unconditional. Add a pointer beside the DialControl paragraph: with HTTP(S)_PROXY set the guarded transports hand DialControl the proxy address and the proxy reaches the real target, so the screen is advisory whenever a proxy is configured. The precondition is operator-set (no client or downstream can set the daemon's environment), which is why it is owed a decision, not a patch; the decision itself stays in the two plane docs. Doc only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pj3yGS6d6GMPvDFqBQgReN
dinstein
force-pushed
the
sec-audit-fixes
branch
from
August 10, 2026 08:40
39eb462 to
2a01ed2
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes from the security-audit sweep: the CRITICAL and HIGH findings that survived adversarial verification, one commit per finding, each with its failing test first.
-it,-itv) no longer swallows the following escape flag / mount valueresourcevalue against the contacted server before adopting it as the token audiencestatusmeta-tool filters connection diagnosis by the caller's scope and stops rendering raw failure detail over MCP