Skip to content

permissions: cover MCP/hook process-source configs and trailing-dot variants in is_sensitive_path - #271

Open
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/cve-mcp-process-source-denylist
Open

raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/cve-mcp-process-source-denylist

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

What

Two targeted hardenings in core/harness/permissions.py, both about the path-matching layer:

  1. Protect MCP/hook process-source configs. Adds 5 names to SENSITIVE_PATH_PATTERNS:
    .mcp.json, mcp_servers_canonical.json, .deepcode/settings.json, .claude/settings.json,
    .deepcode-plus/settings.json.

    These files decide which external processes the next session spawns (MCP stdio servers,
    workspace hooks), so a written-in server definition is delayed code execution; the generated
    sync copies also carry plaintext env-derived keys. They belong in the same protected class
    as .env / .netrc already on the list, and they are script-maintained outside the agent
    tool path, so denying tool-path access breaks no legitimate in-session workflow.

  2. Match through trailing dots/spaces (CVE-2026-103572-class bypass). Windows drops
    trailing dots and spaces from path components at open time while os.path.normpath keeps
    them — the literal name .mcp.json. opens .mcp.json, yet a literal compare misses it.
    is_sensitive_path now also probes per-component rstrip(" .") variants (with . / ..
    components protected). The strip is match-only: the reported path stays as written.

Why

  • Delayed-RCE surface + plaintext key material should not be readable/editable through the
    file tools under any permission mode.
  • The denylist was bypassable by a known OS-vs-normalizer mismatch class; matching only the
    as-written form is what CVE-2026-103572 exploited upstream of this pattern family.

Tests

3 new regression tests (red → green):

  • MCP configs denied across permission modes, and allow rules cannot override the deny.
  • Trailing dot/space variant bypass blocked.
  • No false positive on .vscode/settings.json or plain JSON files.

Local run on Windows git-bash: tests/test_harness_permissions.py 38 passed. The
pre-existing test_denylist_follows_symlinks fails locally with WinError 1314 (symlink
privilege) — that test exists unchanged on main and is environmental, not introduced here.

Security Considerations

  • Deny surface is limited to tool-path enforcement (is_sensitive_path) for 5 explicit config
    names; no exec/sandbox control-flow change, patterns are data-only additions.
  • The normalization probe only affects matching; the reported path is never rewritten, so
    logs and error messages keep the user-visible name.
  • Intentional behavior change: scripts that edit these configs must run outside the agent
    tool path (that is the point for generated plaintext-key-bearing copies).

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 — this PR touches the sensitive-path denylist and a CVE-2026-103572-class normalization bypass, so it needs a security review. A fork can't set the security label or request reviewers, flagging it here for triage.

Summary for the reviewer:

  • Data-only additions to SENSITIVE_PATH_PATTERNS (5 MCP/hook config names) + a per-component rstrip(" .") match-only probe in is_sensitive_path.
  • No exec/sandbox control-flow change; reported paths are never rewritten.
  • 3 new regression tests (MCP configs denied across modes; trailing-dot/space bypass blocked; no false positives), plus full local preflight with a KNOWN-RED verdict (new=0; the known red is the pre-qualified Windows symlink-privilege case, unrelated).

@Zongwei9888

Copy link
Copy Markdown
Collaborator

Thank you, @raymondginger2018-sudo. The trailing-dot part is right: Windows drops trailing dots and spaces from path components when it opens a file, so secret.key. opens secret.key while the pattern check misses it. Please keep that probe and its tests.

The new patterns need to change before we can merge:

  1. They protect the wrong files. DeepCode loads hooks from ~/.deepcode/hooks.json, <workspace>/.deepcode/hooks.json and <workspace>/.claude/settings.json (core/harness/hooks/discovery.py:70-74). hooks.json isn't covered, while .mcp.json, mcp_servers_canonical.json, .deepcode/settings.json and .deepcode-plus/settings.json aren't files DeepCode reads.
  2. They use the wrong kind of protection. SENSITIVE_PATH_PATTERNS is the credential denylist: every tool is denied, reads included, and Grep/Glob skip the files (core/harness/tools/search.py:128,144,233). Hook files aren't secrets, and the agent should still be able to read .claude/settings.json; the risk is in writing them. (The sandbox already blocks sandboxed commands from writing these paths, core/harness/sandbox.py:258-264.)
  3. Please drop references readers here can't follow (the CVE id, the "2026-10-07 scan §9.4", the sync script) from the comments.

A version we'd merge: the trailing-dot probe as it is, plus a check that turns a file-tool write to one of the three hook files into an ask (never a deny for reads), with tests for both.

The three hook-discovery files (user and workspace hooks.json, workspace
.claude/settings.json) are not secrets: reads stay free. A written-in
handler runs commands in a later session, so a file-tool write that would
otherwise be allowed becomes an ask; stricter decisions (deny rules, plan
mode, the read-only preset) are never relaxed, and no allow rule can
silence the confirmation.

Also match sensitive paths through Windows trailing dots and spaces, which
the OS drops at open time while normpath keeps them: a stripped probe
variant is checked alongside the literal path (the reported path stays as
written).
@raymondginger2018-sudo
raymondginger2018-sudo force-pushed the pr/cve-mcp-process-source-denylist branch from 8c88f38 to b25569f Compare October 22, 2026 05:16
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 — rewritten as the version you described, pushed as b25569fc (based on the current main tip):

  • The five denylist patterns are gone. The PR no longer touches SENSITIVE_PATH_PATTERNS; hook files are not secrets.
  • Writes to the discovery files now ask. A new is_hook_config_path matches the three files discovery.py reads from by two suffixes — /.deepcode/hooks.json (covers both the user-level and workspace-level file) and /.claude/settings.json. In PermissionEngine._evaluate, any write that would otherwise be allowed (rule allow, full_auto) is downgraded to ask with a message that hook handlers run commands in a later session. Reads are never gated here, and a stricter decision keeps its strength — deny rules, plan mode and the read-only preset still deny.
  • The trailing-dot probe is kept as-is, and the new ask check uses the same probe, so .claude/settings.json. (Windows drops the trailing dot at open time) still asks.
  • Unfollowable references removed from comments; the only external pointer is the in-repo core.harness.hooks.discovery module.

Tests cover both directions: writes ask even in full_auto, reads stay allowed (file tools and search), plan/read-only never relax into an ask, and the trailing-dot denylist cases still hold.

Local preflight of this exact head (Windows, Python 3.12/3.13/3.14): no new failures; only the already-tracked Windows-only lifecycle/desktop flakes.

@Zongwei9888

Copy link
Copy Markdown
Collaborator

Thank you for the quick rewrite, @raymondginger2018-sudo. This is the shape we asked for: reads stay free, the trailing-dot probe is kept, and stricter decisions keep their strength. One change before merge.

Full Access. Full Access resolves to full_auto with approval_policy=never (core/domain/execution_security.py:175-180), and _apply_approval_policy turns an ask into a deny under never. So with this branch, a Full Access session can't write .claude/settings.json or hooks.json at all, and nothing can approve it. I checked through build_permission_engine(None, cwd=ws, execution_security_profile=ExecutionSecurityProfile.for_preset("full_access")): write_file on .claude/settings.json returns deny ("… requires confirmation …"), while on main it is allow. Full Access also runs commands without the sandbox, so a shell command can write the same file anyway; the deny would only block a user who asked for a hook.

Please skip this check under Full Access, the same way Full Access skips the sensitive-path guard and origin approval (core/harness/policy.py:204-210). For example, have build_permission_engine pass a flag that is off when profile.access_preset is ExecutionAccessPreset.FULL_ACCESS, and add a test through build_permission_engine with the full_access profile that expects allow. The legacy full_auto path with an approver (on_request) should keep asking, as it does now.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants