tools: enforce BashTool whitelist across compound commands - #279
tools: enforce BashTool whitelist across compound commands#279FireCollector wants to merge 3 commits into
Conversation
BashTool previously skipped standalone commands and the final command in a pipeline, while other shell separators were not treated as command boundaries. Parse and validate every executable segment, fail closed on unverifiable substitution syntax, and preserve the unrestricted default work-directory behavior. Fixes trpc-group#277 RELEASE NOTES: Fixed BashTool command whitelist validation for standalone and compound shell commands.
AI Code ReviewBash 被拦截了。让我手动追踪一下逻辑。 让我仔细分析一下关键的行为变更和解析逻辑。 新的
现在让我检查一下解析过程中存在的实际问题。看看 让我追踪一下
现在关键问题来了: 但这里有一个微妙的问题:在 escaped 分支中, 让我检查一个真正的绕过向量:命令替换检测。
等等,让我重读一下:检查是 在双引号内呢: 反引号在双引号内:同样返回 None。很好。 现在,真正令人担忧的问题:之前代码的行为。旧代码使用 让我检查 现在,一个真正的问题:默认情况下( 让我检查实际的 Bug。考虑重定向到相对路径的命令,如 考虑一个非白名单且在 workdir 内的命令:被允许。这符合设计。 现在让我看看测试 现在是否有任何真正被破坏的解析案例?考虑 考虑 考虑 bash here-string 让我核实一下用于赋值前导的 让我检查 现在一个真正值得关注的问题:在引号块内, 现在真正的 Bug —— 看一下 这是一个真正的安全问题吗?在工作目录内,所有操作无论如何都是被允许的,所以那里没有差别。但考虑一下带有 但是 真正值得关注的是: 所以 那缺失的 现在让我考虑一个实际的绕过:带空格的赋值前导不是问题。 那针对 让我重新考虑 OK,考虑相反的情况:一个绕过,其中 示例: 那 关键点:每个段都会被独立检查。因此,即使
所以合并两个命令的真正风险仅在 所以我认为解析器是 fail-safe 的:它可能会误报(阻塞合法命令),但不会漏报被禁止的命令,因为段总是被拆分,且每个段的首个 token 都会被检查。 但是等等 —— 真正微妙的绕过:如果一个非白名单命令作为第一个命令的参数,但不会被执行呢?那不是注入。通过 一个值得注意的遗漏:不带 所以解析器是稳健的。主要的发现是误报问题:合法的 shell 结构(heredoc、 让我确认一下那个行为变更,因为它很重要: 旧代码:循环遍历 base_commands;如果 现在 让我专注于 diff 新增的测试有效性。 但是等等 —— 打补丁的目标是 现在一个真正的问题:在这个测试中, 现在让我重新考虑是否有什么真正被破坏的地方。让我重新检查 那尾部的 我认为逻辑是稳健的。让我考虑一下 现在,让我考虑一个值得提出的真正的兼容性/误报问题:heredoc/here-string 和 赋值前导前缀,这些在白名单模式下会导致合法命令的误报,且这些命令原本是有效的。鉴于白名单模式是显式启用的,且文档说明仅允许白名单命令,heredoc 的误报是一个 Warning 级别的问题。 实际上,让我检查赋值前缀: heredoc:旧代码使用 shlex 分割 这是一个回归:旧代码允许使用 heredoc,新代码将其阻塞(因为 heredoc 定界符 但更重要的是 —— 这是一种安全权衡吗?新行为更安全(检查更多)。但它破坏了合法的 heredoc 用法。考虑到文档说明白名单限制了命令,并且 heredoc 主体包含任意命令,将其视为命令进行检查,可以说更安全。我将把它标记为 Warning,作为一个值得注意的兼容性/误报问题。 还有换行符作为分隔符的问题:在 让我再考虑一个真正的问题: 现在,让我思考一下是否还有任何真正的安全绕过。让我重新考虑引号内引用的情况:单引号内的 双引号内的反引号 —— 被捕获(返回 None)。很好。 但是双引号内的 我认为没有安全漏洞。现在让我写一下审查报告。让我重新考虑严重程度。核心逻辑看起来是 fail-safe 的。我发现的主要问题:
所以主要值得注意的发现是 heredoc/换行符拆分的回归。让我确认一下旧代码在换行符上的行为:旧代码使用 punctuation_chars="|" 的 shlex.shlex —— 换行符默认是空白字符(whitespace)。所以 所以新代码在换行符安全性上严格更好。heredoc 的误报是一个权衡:它破坏了 heredoc,但 heredoc 正文包含任意命令,所以在白名单模式下检查它们可以说更安全。我认为这甚至不是一个真正的 Warning —— 这是更严格的安全行为。但文档/声明的契约说“支持管道、重定向、复杂命令”,而 heredoc 是一个合法的特性,现在在白名单模式下失效了。我会将其作为一个低优先级的 Suggestion/Warning 提出。 让我重新考虑是否有什么真正被破坏或错误的地方。我相当有信心解析器是 fail-safe 的。让我也验证一下 让我再检查一下:在白名单分支中, 让我检查 我认为这个 PR 基本是稳健的。让我写下简洁的审查报告。鉴于 fail-safe 的行为和改进的安全性,我会指出 heredoc/multiline 的误报作为 Warning,可能还有一个关于弱测试的说明。让我重新检查弱测试 让我再次考虑 heredoc 问题的严重程度。实际上,因为 但注意:在默认模式下(无白名单),在 对于 (b),ALLOWED_COMMANDS_OUTSIDE_WORKDIR = [ls,pwd,cat,grep,find,head,tail,wc,echo] —— 这些是面向读取的。heredoc 对于这些命令是有意义的(例如, 我也考虑一下 here-string 最终答案:我将报告一个关于 heredoc/多行命令在白名单/外部 cwd 模式下产生误报的 Warning,以及可能的一个关于……让我想想是否有什么真正的问题。我将保持简洁。 实际上,让我重新考虑这到底是不是一个“问题”还是预期的安全收紧。PR 的提交信息说“enforce BashTool whitelist across compound commands”。拆分换行符是有意的。Heredoc 正文包含可执行命令,因此在白名单模式下阻塞它们在安全性上是可以辩护的。但文档契约(“支持复杂命令”)现在在白名单模式下排除了 heredoc。我会将其标记为 Warning 并建议:要么在文档中说明在白名单模式下不支持 heredoc,要么显式跳过 heredoc 正文。建议级别。我将把它作为 Warning 提出,因为这是一个真正破坏了文档中记载的特性集的回归。 让我完成审查。I've traced The key behavioral change: the new parser treats 发现的问题
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
Coverage ? 88.45146%
==========================================
Files ? 491
Lines ? 46179
Branches ? 0
==========================================
Hits ? 40846
Misses ? 5333
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Treat heredoc bodies as input data while continuing to fail closed on unquoted command substitutions, unterminated delimiters, and non-whitelisted commands after the terminator. Updates trpc-group#277 RELEASE NOTES: NONE
AI Code Review基于我对 发现的问题🚨 Critical
💡 Suggestion
总结整体是一组高质量的安全加固(复合命令分段、heredoc/引号/重定向处理、替换闭合拒绝),fail-closed 设计正确;但存在一个真实可利用的白名单绕过: 测试建议
|
| index = 0 | ||
| previous_is_redirect = False | ||
| pending_heredocs: list[tuple[str, bool, bool]] = [] | ||
|
|
There was a problem hiding this comment.
反斜杠-换行行续行可拆分操作符绕过命令/进程替换检测
解析器把 \<newline> 当作转义下一字符,而 bash 会作为行续行删除。echo $\<newline>(blocked_cmd) 在解析器中 $ 和 ( 被换行隔开,漏检 $(...,命中白名单后放行;bash 执行时还原为 echo $(blocked_cmd) 绕过白名单执行任意命令。建议进入主循环前先规约(删除)\<newline> 再做操作符检测,或对未引号包裹的 \<newline> 直接判为不安全。
Fail closed on unquoted shell line continuations before they can join command or process substitution operators. Add static regression cases while preserving literal content inside single quotes. Updates trpc-group#277 RELEASE NOTES: NONE
AI Code ReviewI've reviewed the diff (a rewrite of 发现的问题
|
|
感谢审查。反斜杠续行导致的绕过问题已在 c9b78d4 中修复,并补充了命令替换以及输入、输出进程替换的静态回归测试。 对于进程替换及其他无法可靠分类的 shell 语法,本 PR 在白名单受限场景下会有意采用 fail-closed 策略直接阻止。这是一项明确的兼容性收紧;若要在保持安全边界的同时支持嵌套 shell 结构,需要引入更完整的 shell 解析能力,超出了本次修复范围。 目前所有自动检查均已通过。烦请维护者在方便时进行人工评审,谢谢! |
Summary
Problem
BashTool command validation only recorded a command when it encountered a pipe and did not record the final segment. As a result, standalone commands could produce no commands to validate, the final command in a pipeline was skipped, and compound separators were not handled consistently. This could bypass a custom whitelist or the default outside-working-directory restrictions.
Fix
The validation now statically extracts every executable command segment while respecting quotes, escapes, redirections, pipelines, compound separators, and newlines. Unparseable input and command or process substitution fail closed. The existing unrestricted behavior for commands executed inside the configured working directory is preserved.
No shell command samples are executed by these tests. They exercise the safety predicate directly or mock subprocess creation.
Validation
Fixes #277
RELEASE NOTES: Fixed BashTool command whitelist validation for standalone and compound shell commands.