From bc3df8a4d58e180ab46cf27b947e13e75a07ba51 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 21:19:14 -0700 Subject: [PATCH] fix: NT-namespace guard fires before every sibling resolve (checkpoint, ACP bridge, @file:) Three paths still resolved the raw model/remote-supplied string before the guard could refuse it, so on Windows the NTLM-leak trigger (resolving the path) ran anyway: the file-checkpoint helper stats write_file/patch targets before the tool executes; the ACP file bridge resolves fs/read_text_file and fs/write_text_file paths before its read/write denylists; and @file:/@folder: references resolve their target before the reference allow-check. Each now checks the raw string first and refuses. The GLOBALROOT form now requires its path separator so a GLOBALROOT-prefixed local name is not misclassified. The rationale comment names the vector instead of another product's changelog, and the security docs say the row is enforced on reads as well as writes, since it sits under the write-guard table. --- agent/context_references.py | 3 +++ agent/copilot_acp_client.py | 12 ++++++++---- agent/file_safety.py | 9 ++++----- agent/tool_executor.py | 5 +++++ tests/agent/test_nt_namespace_guard.py | 23 ++++++++++++++++++++--- website/docs/user-guide/security.md | 2 +- 6 files changed, 41 insertions(+), 13 deletions(-) diff --git a/agent/context_references.py b/agent/context_references.py index 5911ff7f84..a23504c8c0 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -340,6 +340,9 @@ def _is_under(path: Path, root: Path) -> bool: def _resolve_path(cwd: Path, target: str, *, allowed_root: Path | None = None) -> Path: + from agent.file_safety import is_nt_namespace_path + if is_nt_namespace_path(target): # raw-string check: resolving such a path is the NTLM-leak trigger + raise ValueError("path uses a Windows NT/device namespace prefix and cannot be attached") resolved = (cwd / Path(os.path.expanduser(target))).resolve() # `/` keeps an absolute target as-is if allowed_root is not None and not _is_under(resolved, allowed_root): raise ValueError("path is outside the allowed workspace") diff --git a/agent/copilot_acp_client.py b/agent/copilot_acp_client.py index 3df2f1477b..4c6641b775 100644 --- a/agent/copilot_acp_client.py +++ b/agent/copilot_acp_client.py @@ -27,7 +27,8 @@ from agent.acp_openai_bridge import ( extract_tool_calls_from_text as _extract_tool_calls_from_text, render_tool_bridge_sections as _render_tool_bridge_sections, ) -from agent.file_safety import get_read_block_error, get_write_denied_error, is_write_approval_required +from agent.file_safety import ( + get_nt_namespace_error, get_read_block_error, get_write_denied_error, is_write_approval_required) from agent.redact import redact_sensitive_text from tools.environments.local import hermes_subprocess_env @@ -217,7 +218,10 @@ def _render_message_content(content: Any) -> str: return str(content).strip() -def _ensure_path_within_cwd(path_text: str, cwd: str) -> Path: +def _ensure_path_within_cwd(path_text: str, cwd: str, *, verb: str) -> Path: + # Raw-string check BEFORE resolve(): resolving an NT-namespace path is the NTLM-leak trigger. + if nt_error := get_nt_namespace_error(path_text, verb=verb): + raise PermissionError(nt_error) if not Path(path_text).is_absolute(): raise PermissionError("ACP file-system paths must be absolute.") resolved, root = Path(path_text).resolve(), Path(cwd).resolve() @@ -237,7 +241,7 @@ def _effective_timeout(timeout: Any) -> float: def _fs_read_text_file(params: dict[str, Any], cwd: str) -> Any: - path = _ensure_path_within_cwd(str(params.get("path") or ""), cwd) + path = _ensure_path_within_cwd(str(params.get("path") or ""), cwd, verb="Read") if block_error := get_read_block_error(str(path)): raise PermissionError(block_error) try: @@ -252,7 +256,7 @@ def _fs_read_text_file(params: dict[str, Any], cwd: str) -> Any: def _fs_write_text_file(params: dict[str, Any], cwd: str) -> Any: - path = _ensure_path_within_cwd(str(params.get("path") or ""), cwd) + path = _ensure_path_within_cwd(str(params.get("path") or ""), cwd, verb="Write") if denied := get_write_denied_error(str(path)): raise PermissionError(denied) if is_write_approval_required(str(path)): # soft-gated for interactive tools; the ACP shim has no human channel → fail closed diff --git a/agent/file_safety.py b/agent/file_safety.py index 89453ef548..13d0912b61 100644 --- a/agent/file_safety.py +++ b/agent/file_safety.py @@ -77,10 +77,9 @@ def _home_and_resolved(path: str) -> tuple[str, str]: # --------------------------------------------------------------------------- # Windows NT-namespace path guard # -# Inspired by Claude Code v2.1.234 (Aug 2026), which made its pre-approval -# file accesses "reject Windows NT-namespace (`\\??\\`) paths, hardening the -# remaining pre-approval file accesses against the NTLM credential-leak -# vector." +# Pre-approval file accesses reject Windows NT-namespace (``\??\``) paths so +# the remaining unguarded path touches cannot be turned into an NTLM +# credential leak. # # The vector: on Windows, merely *resolving or touching* a path such as # ``\\??\\UNC\\attacker.example\\share\\x`` (or the ``\\\\?\\UNC\\`` / @@ -131,7 +130,7 @@ def is_nt_namespace_path(path: str) -> bool: if s.startswith("\\\\?\\"): rest = s[4:] upper = rest.upper() - if upper.startswith("UNC\\") or upper.startswith("GLOBALROOT"): + if upper.startswith("UNC\\") or upper.startswith("GLOBALROOT\\"): return True return False diff --git a/agent/tool_executor.py b/agent/tool_executor.py index fa73ec6b62..12ce22b175 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -80,8 +80,13 @@ def _ensure_file_checkpoint(agent, function_name: str, function_args: dict, effe file_path = function_args.get("path", "") if not file_path: return + from agent.file_safety import is_nt_namespace_path from tools.file_tools_paths import _resolve_path_for_task + # Resolving an NT-namespace path is itself the NTLM-leak trigger; leave the + # tool's raw-string guard to refuse it without a checkpoint stat. + if is_nt_namespace_path(file_path): + return resolved_path = _resolve_path_for_task(file_path, effective_task_id or "default") agent._checkpoint_mgr.ensure_checkpoint( agent._checkpoint_mgr.get_working_dir_for_path(str(resolved_path)), f"before {function_name}", diff --git a/tests/agent/test_nt_namespace_guard.py b/tests/agent/test_nt_namespace_guard.py index 1d53a2f640..93c688e211 100644 --- a/tests/agent/test_nt_namespace_guard.py +++ b/tests/agent/test_nt_namespace_guard.py @@ -9,7 +9,8 @@ rely on. So the guard must fire on the model-supplied string before import os import unittest -from unittest.mock import patch +from types import SimpleNamespace +from unittest.mock import MagicMock, patch from agent.file_safety import ( get_read_block_error, @@ -54,14 +55,21 @@ class TestNtNamespaceGuard(unittest.TestCase): self.assertNotIn("NT/device namespace", get_write_denied_error(p) or "", p) def test_file_tools_reject_raw_string_without_resolving(self): - """Every file-tool entry refuses BEFORE the task-base join / resolve — the - resolve is the leak, and on POSIX the join would hide the prefix.""" + """Every file-tool entry — and the sibling paths that touch a file path before + the tool runs (checkpoint helper, ACP file bridge, @file: references) — refuses + BEFORE the task-base join / resolve: the resolve is the leak, and on POSIX the + join would hide the prefix.""" + from pathlib import Path + + from agent import context_references, copilot_acp_client, tool_executor from tools.file_tools import patch_tool, read_file_tool, search_tool, write_file_tool bad = "\\??\\UNC\\attacker.example\\share\\x" with patch("agent.file_safety.Path") as fs_path, \ patch("tools.file_tools._resolve_path_for_task") as ft_resolve, \ patch("tools.file_tools_write_guards._resolve_path_for_task") as wg_resolve, \ + patch("tools.file_tools_paths._resolve_path_for_task") as ckpt_resolve, \ + patch.object(Path, "resolve", side_effect=AssertionError("must not resolve")), \ patch.object(os.path, "realpath", side_effect=AssertionError("must not realpath")): results = [ read_file_tool(bad), @@ -69,9 +77,18 @@ class TestNtNamespaceGuard(unittest.TestCase): patch_tool(mode="replace", path=bad, old_string="a", new_string="b"), search_tool("x", path=bad), ] + checkpoint_agent = SimpleNamespace(_checkpoint_mgr=MagicMock(enabled=True)) + tool_executor._ensure_file_checkpoint(checkpoint_agent, "write_file", {"path": bad}, "default") + checkpoint_agent._checkpoint_mgr.ensure_checkpoint.assert_not_called() + for fs_handler in (copilot_acp_client._fs_read_text_file, copilot_acp_client._fs_write_text_file): + with self.assertRaisesRegex(PermissionError, "NT/device namespace"): + fs_handler({"path": bad, "content": "x"}, "/tmp") + with self.assertRaisesRegex(ValueError, "NT/device namespace"): + context_references._resolve_path(Path("/tmp"), bad) fs_path.assert_not_called() ft_resolve.assert_not_called() wg_resolve.assert_not_called() + ckpt_resolve.assert_not_called() for r in results: self.assertIn("NT/device namespace", r) diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index 71f6a10d11..4c6d950a0d 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -371,7 +371,7 @@ Unset the variable to restore unrestricted writes (subject to the protected-path Do not ask the agent to `patch` `~/.hermes/cron/jobs.json` directly. Use the `cronjob` tool, [`hermes cron`](./features/cron.md), or `/cron` — they update the job store through the supported API. The same applies to other Hermes control files when write safety blocks direct edits. :::note Defense-in-depth, not a hard boundary -Write guards apply to `write_file` and `patch` only. The `terminal` tool runs as the same OS user and can still `cat` or overwrite denied paths via shell commands. The denylist reduces accidental damage and gives models a clear stop signal; it does not sandbox a hostile or compromised agent. +Write guards apply to `write_file` and `patch` only, with one exception: the Windows NT/device-namespace row is also enforced on reads — `read_file`, `search_files`, `@file:`/`@folder:` context references and the ACP file bridge all refuse those paths on the raw string, before anything resolves them. The `terminal` tool runs as the same OS user and can still `cat` or overwrite denied paths via shell commands. The denylist reduces accidental damage and gives models a clear stop signal; it does not sandbox a hostile or compromised agent. ::: ## User Authorization (Gateway)