Skip to content

fix(cosh-ng): block path traversal in readonly - #2708

Open
jfeng18 wants to merge 1 commit into
mainfrom
fix/cosh-ng/readonly-path-traversal
Open

fix(cosh-ng): block path traversal in readonly#2708
jfeng18 wants to merge 1 commit into
mainfrom
fix/cosh-ng/readonly-path-traversal

Conversation

@jfeng18

@jfeng18 jfeng18 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Why

is_blocked_special_path matched /proc, /dev and /sys by raw prefix only, so spellings like /../proc/version bypassed the blocklist and were allowed through the readonly auto-execution chain — a real bypass of the readonly safety check.

What changed

  • Paths are normalized lexically (no filesystem access) before matching the special-path blocklist.
  • The raw-prefix pass is kept first so the check stays fail-closed for anything normalization cannot resolve.
  • $-quoting forms remain intercepted upstream by the exec layer; noted in a code comment.

Related issue

closes #2184

User / Agent impact

Commands whose path arguments lexically resolve into /proc, /dev or /sys — previously executed via the readonly chain — are now blocked. Legitimate paths are unaffected.

Risk and compatibility

  • Public CLI, API, configuration, or documented behavior changed
  • Privileged or security-sensitive behavior changed
  • Cross-component contract changed
  • Migration or rollback guidance is needed

Security tightening: the newly intercepted set is exactly the spellings that lexically resolve into /proc, /dev or /sys, all of which were genuine bypasses before the fix. An anti-false-positive matrix pins legitimate paths that must keep passing.

Validation

  • ECS discriminative two-direction test: bypass reproduces (allowed) on pre-fix code, blocked on fixed code.
  • ECS cargo test: 1343 passed (baseline 1340 + 3 new).
  • cargo fmt and clippy clean with zero warnings.

Documentation and rollback

Single commit — revert it directly to restore the previous matching behavior if ever needed.

@jfeng18
jfeng18 requested a review from SunnyQjm as a code owner August 20, 2026 08:09
@github-actions github-actions Bot added the component:cosh-ng src/cosh-ng label Aug 20, 2026

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本次审查范围内未发现需要修改的问题。


🤖 Generated by QoderView workflow run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f5d3c168e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/cosh-ng/crates/cosh-shell/src/tools/readonly_rules/evaluator.rs
@SunnyQjm

SunnyQjm commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

PR number: #2708
head_sha: 2f5d3c1
reviewed_at: 2026-08-20T08:36:11Z

Findings

  • [P1] 相对路径 .. 穿越仍然放行,且被新测试固化为预期行为evaluator.rs:422-445normalize_path_lexically 对相对路径保留前导 ..("cannot be resolved without the working directory"),于是 has_blocked_special_prefix 永远不命中相对拼写;tests.rs:469-471 显式断言 ../proc/versionis_safe_readonly_path。但 readonly 自动执行链是在真实 shell cwd 下执行的:当 cwd 是 / 的子目录(如 /tmp)时,cat ../proc/version 在 OS 层解析为 /proc/version,穿越绕过对本类拼写仍然敞开。本 PR 的修复目标(closes [cosh-ng] bug: is_blocked_special_path 纯字符串前缀匹配,路径穿越与 $'...' ANSI-C quoting 可绕过 readonly_rules 黑名单 #2184,阻断进入 /proc//dev//sys 的穿越)只覆盖了绝对路径拼写。建议:在执行点用真实 cwd 解析相对路径后再过 blocklist,或在无法解析时 fail-closed(退回人工审批),而不是放行并把不安全结果钉成测试合同。
  • [P2] 当前 head 的 CI 为失败状态,与 PR 描述的自证不符 — PR state 中 Test cosh-ngTest cosh-ng fast checks 在该 head_sha 上均为 FAILURE,而 PR body 声称 "1343 passed、fmt/clippy clean"。合并前必须让这两项转绿并确认失败原因与本次改动无关(或已修复)。

结构评审确认项(无 blocking 组织问题)

  • Owner 归属正确:改动落在 tools/readonly_rules/,readonly 路径策略属 tools/ owner(tool classification / readonly policy),符合 standard.md §2。
  • Public API 未扩大:不触碰 lib.rs,无新增 pub mod/pub use;新增 has_blocked_special_prefixnormalize_path_lexically 均为 private fnis_blocked_special_path 既有 pub 签名不变,无需更新 public-api-inventory。
  • 无 forbidden dependency / self-crate path:仅用 std 字符串操作,未引入 crate::agent/runtime/ui/...,无 cosh_shell::...
  • 范围收敛:单 owner、两个文件、单 commit,无跨 crate 配套改动;未新增 root src/*.rs
  • Fail-closed 设计正确:raw prefix 先行、/proc/../etc 类拼写保持阻断;anti-false-positive 矩阵覆盖 /home/../usr/bin/ls 等合法路径。
  • 测试位置:用例放入既有 tools/readonly_rules/tests.rs,与现状一致,测试组织细节归测试 SDD。

剩余风险

  • Symlink 别名与 ~ 展开不在 lexical normalization 覆盖范围,doc comment 已声明无 filesystem access 契约,属设计内取舍。
  • $ 引用/变量展开依赖上游 broker is_shell_meta 与 compound executor rule 5 拦截,本层不构成完整 sandbox,doc comment 已明确分工;后续改上游拦截逻辑时不得破坏该前提。

未跑验证(以 CI 实测为准)

  • 本评审不执行工具;evaluator.rs 新增约 49 行(diff 推算),该文件未登记在 large-file-inventory,是否触及 700 行阈值以 check-layout.sh 实测为准。
  • PR body 的测试结果与 fmt/clippy 结论未由本评审复核,且与当前 CI FAILURE 状态冲突,见 P2。

@jfeng18
jfeng18 force-pushed the fix/cosh-ng/readonly-path-traversal branch from 2f5d3c1 to 0ae466c Compare August 21, 2026 03:27
@jfeng18

jfeng18 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings addressed in 0ae466c (single commit, force-pushed):

P1 (relative traversal) — fixed fail-closed and cwd-independently: after lexical normalization, a leading .. chain whose first non-.. component is dev/proc/sys is now blocked (relative_traversal_enters_special_dir in evaluator.rs). This layer has no cwd and no filesystem access by contract, so instead of resolving against the execution cwd it fails closed for every cwd; the accepted cost (an ordinary directory literally named proc/dev/sys reached via ..) is documented and such commands still go through interactive approval. The old src tests that pinned ../proc/version as safe were removed; the relocated tests now assert it blocked plus ../notes.txt/../../tmp/x allowed (relative_traversal_into_special_dirs_is_blocked, traversal_normalization_does_not_over_block).

P2 (CI vs body) — the fast-checks failure was the test-inventory overlap ratchet: the three new tests had landed in tools/readonly_rules/tests.rs, which is compiled into both the lib and bin test targets (main.rs re-declares lib modules), pushing the lib/bin overlap to 697 > 694 ceiling. They were moved to tools/readonly_rules/traversal_tests.rs, declared only from lib.rs via the existing wrap_tests pattern (is_safe_readonly_path is not publicly exported, so a tests/ integration file cannot reach it), restoring overlap = 694 = ceiling — the audit gate now passes locally. The Test cosh-ng failure is provider_lifecycle::cosh_core_sync_drains_child_output_while_writing_large_prompt timing out after 3s (71/72 pass; the same code is green on main; zero interaction with this diff) — will re-run it off-peak to confirm the environmental attribution.

@SunnyQjm

Copy link
Copy Markdown
Collaborator

PR number: #2708
head_sha: 0ae466c
reviewed_at: 2026-08-21T06:26:24Z

Findings

  • [P2] 当前 head 的 Test cosh-ng fast checks 仍为 FAILURE,与作者"ratchet 已修复"的自证冲突 — status checks 显示 Test cosh-ng fast checks0ae466c 上 FAILURE(04:26:16),而作者在 PR 级回复中称 overlap 已恢复 694=ceiling、audit gate 本地通过。Test cosh-ng 已转 SUCCESS(03:37:27),作者归因的环境性超时解释初步成立。合并前需让 fast checks 转绿,或给出失败日志证明与本改动无关(fast checks 通常正是 layout/test-inventory/fmt/clippy 门禁所在,恰是本 PR 声称修复的那类检查)。

结构评审确认项(无 blocking 组织问题)

未发现 blocking package/module/public API 组织问题:

  • Owner 归属正确:全部改动落在 tools/readonly_rules/,readonly 路径策略属 tools/ owner(tool classification / readonly policy),符合 standard.md §2。
  • Public API 未扩大lib.rs 新增的 mod readonly_traversal_tests(lib.rs:32-34)是 #[cfg(test)] private mount,非 pub mod/pub useis_blocked_special_path 既有 pub 签名不变,has_blocked_special_prefix/relative_traversal_enters_special_dir/normalize_path_lexically 均为 private fn,无需更新 public-api-inventory。为规避 lib/bin overlap ratchet 而把测试挂到 lib-only 位置,作者说明沿用既有 wrap_tests pattern,测试挂载细节归 shell-test-organization SDD。
  • 无 forbidden dependency / self-crate path:仅 std 字符串操作,无 crate::agent/runtime/ui/...,无 cosh_shell::...
  • 范围收敛:单 owner、三个文件、单 commit,无跨 crate 配套改动,未新增 root src/*.rs 实现文件。
  • 前轮 P1 修复有效relative_traversal_enters_special_dir(evaluator.rs:416-431)对前导 .. 链 fail-closed;normalizer 不变量(surviving .. 只可能位于前导,内层均被 pop)成立,../../.. 纯穿越正确放行;traversal_tests.rs 断言方向已反转(../proc/version../../dev/urandom../foo/../proc/version 全部 blocked),旧 head 中把 ../proc/version 钉为 safe 的不安全断言已随 tests.rs 还原一并移除(tests.rs 不再是 changed file)。fail-closed 次序正确:/proc/../etc 经 raw prefix 先行仍阻断。
  • Anti-over-block 矩阵充分/home/../usr/bin/lsusr/../local/file../notes.txt../../tmp/x 均保持 allowed;procx 类前缀不误伤(split('/') 首段精确匹配)。

剩余风险

  • cwd 本身位于 /proc//dev//sys 内部时(如 cwd=/proc/selfcat ../version 解析为 /proc/version),lexical 层无 cwd 无法识别;属 doc comment 已声明的无 filesystem access 契约内的已知取舍。
  • symlink 别名与 ~ 展开不覆盖(已声明);$ 变量/quoting 依赖上游 broker is_shell_meta 与 compound executor rule 5,后续修改上游拦截逻辑不得破坏该分工前提。
  • fail-closed 的已知代价:经 .. 访问恰好名为 proc/dev/sys 的普通目录会被拒(仍可走交互审批),已在函数文档中记录,可接受。

未跑验证(以 CI 实测为准)

  • 本评审不执行工具;evaluator.rs 按 diff 估算净增约 92 行,该文件未登记 large-file-inventory,是否越过 700 行阈值以 check-layout.sh 实测为准。
  • Test cosh-ng fast checks 的失败原因未由本评审复核,见 P2;Test cosh-ng SUCCESS 是否覆盖新增 traversal 用例未复核。
  • PR body 的 1343 通过数与 fmt/clippy clean 自证未复核。

is_blocked_special_path matched /proc, /dev and /sys by raw prefix
only, so spellings like /../proc/version bypassed the blocklist.
Normalize lexically (no filesystem access) before matching, keep
the raw-prefix pass first (fail-closed), and block relative
..-prefixed spellings into proc/dev/sys cwd-independently.
$-quoting forms stay intercepted upstream by the exec layer.
Traversal tests moved to a lib-only module (overlap ratchet).

Closes #2184

Assisted-by: Qoder
Signed-off-by: Jiangtian Feng <jiangtianf97@163.com>
@jfeng18
jfeng18 force-pushed the fix/cosh-ng/readonly-path-traversal branch from 0ae466c to eb1cb67 Compare August 21, 2026 09:13
@jfeng18

jfeng18 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the follow-up monitor pass. The Test cosh-ng fast checks failure flagged here was on the previous head 0ae466c, whose old base carried a floating toolchain. After rebasing onto eb1cb67 (main now pins 1.97.1), all jobs pass on the current head (2026-08-21):

  • Test cosh-ng fast checks — SUCCESS (09:20:33)
  • Test cosh-ng — SUCCESS (09:23:30)
  • Build cosh-ng release — SUCCESS (09:19:04)

On the evaluator.rs line-count item from the not-run list: it is enforced by the fast-checks layout gate, which passes on the current head — so the 700-line threshold is cleared by measurement, not just the diff-based estimate.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[cosh-ng] bug: is_blocked_special_path 纯字符串前缀匹配,路径穿越与 $'...' ANSI-C quoting 可绕过 readonly_rules 黑名单

2 participants