fix(approval): escape '+' in generated matchers - #7
Merged
Merged
Conversation
quoteBytesForRegexp passed 15 bytes through verbatim, including '+' — a
regex quantifier. Of the punctuation on that allowlist (@%=:,/_-) it was
the only metacharacter, and it broke matchers in both directions:
- a matcher failed to match the command it was generated from, so the
grant silently never matched and the operator re-approved forever;
- the same pattern matched shorter variants, so an approved
"deploy --tag v1+build" also authorized "deploy --tag v1build".
Confirmed in production state: of 87 stored session grants, the 3
containing '+' were exactly the 3 that failed to match their own source,
and one persistent host rule authorized a command nobody approved.
The string-level invariant (anchors, no \s, no .*) cannot catch this —
it inspects the pattern's text, not its meaning. So the fix is paired
with construction-time proofs:
- literalSafePunct is now a named const, with a test asserting every
byte satisfies QuoteMeta(b) == b, so re-adding a metacharacter fails CI;
- newMatcher gates every generated matcher: it must compile, match its
own SourceCmd, and — for exact matchers — parse to exactly
Concat[BeginText, Literal(SourceCmd), EndText]. Self-match alone does
not imply exactness, and it is the exactness half that catches widening;
- FuzzExactMatcherSelfMatch proves exactness by single-byte mutation;
FuzzGeneralizeNoWiden covers the prefix path.
Construction failures return an error rather than panicking: Generalize
runs on agent-supplied input on the Authorize path, and a panic inside
ApplyDecision would land between the audit append and the grant write.
Those guards surfaced two further pre-existing defects:
- invalid UTF-8: Go regexp reads \xF3 as rune U+00F3, not byte 0xF3, so
a matcher cannot express a lone invalid byte and would authorize a
different command. Rejected via ErrInvalidUTF8Command, like NUL.
- leading whitespace: splitASCIIWords drops it while prefixMatcher
rebuilds from tokens, so the pattern missed its own source. Now exact.
Persisted state is repaired asymmetrically, because only one store keeps
the approved text. Session grants carry source_cmd and every one is minted
by Exact(req.Cmd), so re-deriving reproduces what approving the same
command today would produce — strictly narrowing, and it avoids re-running
the very re-approval churn this bug caused (sessionFileVersion 2 +
normalizeGrants). policy.yaml has no source_cmd, so legacy rules are
detected and refused rather than rewritten from an inferred source; the
command falls back to exit 7 and one approval writes a correct rule.
Also closes two secondary failure modes found along the way:
- a single uncompilable grant aborted the whole locked session, failing
every command in it rather than only its own;
- applyHostGrant validated the candidate only after the resolution and
the approval_granted audit record were written, so a rejection left an
approval that could never be re-decided and an audit trail asserting a
grant that did not exist. The check now runs first, and additionally
verifies the candidate matches its own command.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
newMatcher replaced it as the construction gate, leaving it uncalled. Caught by golangci-lint's unused check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
问题
quoteBytesForRegexp按白名单原样放行 15 个字节,其中+是正则量词却没被转义。放行表的标点里(@%=:,/_-)只有它是元字符,而它把 matcher 双向弄坏了:echo a+b生成\Aecho\x20a+b\z,它不匹配自己的源串。grant 静默失效 → 操作者反复重批。echo ab/echo aab。即批准deploy --tag v1+build会一并授权未批准的deploy --tag v1build。这不是理论问题。一次远端部署中,同一条命令在 12h session TTL 内被重批 3 次(
ap_08f192…→ap_aadaff…→ap_4e8206…,命令 SHA256 相同)。核对实际状态:+的 3 条与自匹配失败的 3 条 完全重合policy.yamlapproval/653e4be6015e自匹配失败,且当前授权去掉+的未批准变体新旧二进制对比是决定性的 —— 对同一条未批准命令,旧版直接授权执行(
policy_rule: approval/host/85269480dc33),新版返回 exit 7 要求审批。为什么不能只靠现有不变量
串级不变量(锚定 / 无
\s/ 无.*)对这个缺陷全部通过 —— 它们检查正则的文本,不检查正则的含义。所以修复配了构造期证明:literalSafePunct提为具名常量,测试逐字节断言QuoteMeta(b) == b,再加回任何元字符即 CI 失败;newMatcher是唯一构造闸门:必须能编译、匹配自己的SourceCmd、且 exact 档语法树恰为Concat[BeginText, Literal(SourceCmd), EndText]。仅自匹配挡不住"放宽"那一半 —— 正则可以匹配源串同时匹配更多,而正是放宽那一半有安全含义;FuzzExactMatcherSelfMatch用单字节增删改变异证明精确性(550 万次执行);FuzzGeneralizeNoWiden覆盖 prefix 路径。构造失败返回 error 而非 panic:
Generalize在Authorize热路径上处理 agent 提供的字符串,panic 等于不可信输入触发的 DoS;且ApplyDecision内的 panic 会落在"审计已写、grant 未写"的窗口里。护栏另外抓出的两个既有缺陷
\xF3表示 rune U+00F3 而非字节 0xF3 —— matcher 无法表达单个非法字节,会转而授权另一条命令。现按ErrInvalidUTF8Command拒绝,与 NUL 一致。splitASCIIWords丢前导空格,而prefixMatcher由 token 重建,导致匹配不上源串。现走 exact。存量修复(两个存储不对称)
只有一个存储保留了操作者批准过的原文:
source_cmd,且全部由Exact(req.Cmd)生成 —— 正则是SourceCmd的纯函数。按它重算与今天重新审批逐比特相同,严格收窄,且避免重现这次事故的重批噪声(sessionFileVersion2 +normalizeGrants)。policy.yaml没有source_cmd。从可轮转的审计日志反推原文去改写操作者可见的 allow 规则不可靠,故只拒绝采信、不改写;命令回落 exit 7,批一次即写入正确规则,旧规则作为惰性文本留待手动删除。实测(在
~/.agentssh副本上):51 条 grant 全部保留、3 条修复、0 条丢弃,文件升到 version 2;approval/653e4be6015e不再进hostMatchers。顺带堵住的两个次生问题
match对该错误return err,中止整个加锁回调),而非只失效自己;applyHostGrant在Resolve和写审计之后才第一次校验候选。失败时 resolution 与approval_granted审计已写、规则却没落盘 → 审批永久卡死,且审计记录声称存在一个并不存在的 grant。校验已前移,并额外验证候选匹配自己的命令。验证
gofmt -l ./go vet ./...干净;go test -race ./...全绿(含cmd/agentssh)FuzzExactMatcherSelfMatch550 万次执行零失败AuthAllowByGrant,去掉+的变体必须AuthNeedsApproval~/.agentssh副本上验证(真实状态未被触碰)不在本 PR 范围
plan execute、结构化 plan(含 stdin)、policy test --session、--fields错误响应保底字段。其中 plan 无法表达 stdin(plan.go:140固定传空 stdin hash,而 grant 必须精确绑定 stdin SHA)是真实的数据模型缺口,也是"批准 plan 后仍要逐条批 stdin"的根因,建议另开一轮。🤖 Generated with Claude Code