mirror of https://github.com/garrytan/gstack.git
fix(careful): close three check-careful bypasses via real JSON extraction
The grep-based command extractor stopped at the first escaped quote, so any quoted argument truncated the command before the pattern checks ran — `git commit -m "wip" && rm -rf /` was silently allowed. Replace it with a python3/node JSON parse that fails CLOSED on unreadable payloads, add an IFS/base64-to-shell obfuscation tripwire, and stop multi-line commands from riding the single-line safe-exception whitelist (line-based grep would have approved `rm -rf /` when a later line matched node_modules — a hazard the real newline decoding exposed). Contributed by @wtamminga (PR #2426; the -R hunk was dropped — it landed in v1.61.0.0 — and output shapes updated to the nested hookSpecificOutput form). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
parent
7a7a83b73e
commit
5084f85e00
|
|
@ -9,16 +9,47 @@ set -euo pipefail
|
|||
# Read stdin (JSON with tool_input)
|
||||
INPUT=$(cat)
|
||||
|
||||
# Extract the "command" field value from tool_input
|
||||
# Try grep/sed first (handles 99% of cases), fall back to Python for escaped quotes
|
||||
CMD=$(printf '%s' "$INPUT" | grep -o '"command"[[:space:]]*:[[:space:]]*"[^"]*"' | head -1 | sed 's/.*:[[:space:]]*"//;s/"$//' || true)
|
||||
# Extract the "command" field value from tool_input with a real JSON parser.
|
||||
#
|
||||
# The previous extractor was
|
||||
# grep -o '"command"[[:space:]]*:[[:space:]]*"[^"]*"'
|
||||
# whose [^"]* stops at the first escaped quote in the JSON string value. Any
|
||||
# destructive command preceded by a quoted argument was therefore truncated
|
||||
# away before the pattern checks ever ran:
|
||||
#
|
||||
# git commit -m "wip" && rm -rf / -> CMD='git commit -m \' -> allowed
|
||||
# bash -c "rm -rf /" -> CMD='bash -c \' -> allowed
|
||||
# echo "x"; rm -rf ~ -> CMD='echo \' -> allowed
|
||||
#
|
||||
# The python3 fallback never rescued these because CMD was non-empty, so the
|
||||
# `[ -z "$CMD" ]` guard did not fire. Parse the payload properly instead, and
|
||||
# fail CLOSED when it cannot be parsed at all — a hook that gates destructive
|
||||
# commands must not allow-by-default on unreadable input.
|
||||
#
|
||||
# python3 is tried first because it ships with macOS and most Linux distros and
|
||||
# is reliably on PATH in a hook environment; node is the fallback.
|
||||
extract_cmd() {
|
||||
if command -v python3 >/dev/null 2>&1; then
|
||||
printf '%s' "$INPUT" | python3 -c 'import sys,json; d=json.loads(sys.stdin.read()); c=d.get("tool_input",{}).get("command",""); sys.stdout.write(c if isinstance(c,str) else "")' 2>/dev/null && return 0
|
||||
fi
|
||||
if command -v node >/dev/null 2>&1; then
|
||||
printf '%s' "$INPUT" | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{try{const j=JSON.parse(s);const c=(j&&j.tool_input&&j.tool_input.command)||"";process.stdout.write(typeof c==="string"?c:"")}catch(e){process.exit(3)}})' 2>/dev/null && return 0
|
||||
fi
|
||||
return 1
|
||||
}
|
||||
|
||||
# Python fallback if grep returned empty (e.g., escaped quotes in command)
|
||||
if [ -z "$CMD" ]; then
|
||||
CMD=$(printf '%s' "$INPUT" | python3 -c 'import sys,json; print(json.loads(sys.stdin.read()).get("tool_input",{}).get("command",""))' 2>/dev/null || true)
|
||||
set +e
|
||||
CMD=$(extract_cmd)
|
||||
EXTRACT_RC=$?
|
||||
set -e
|
||||
|
||||
# No parser available, or the payload is not parseable JSON. Fail closed.
|
||||
if [ "$EXTRACT_RC" -ne 0 ] && [ -n "$INPUT" ]; then
|
||||
printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"[careful] Could not parse the tool payload to safety-check this command. Approve only if you know what it does."}}\n'
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# If we still couldn't extract a command, allow
|
||||
# Parsed fine, but there is genuinely no command field (non-Bash payload) — allow.
|
||||
if [ -z "$CMD" ]; then
|
||||
echo '{}'
|
||||
exit 0
|
||||
|
|
@ -27,6 +58,23 @@ fi
|
|||
# Normalize: lowercase for case-insensitive SQL matching
|
||||
CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]')
|
||||
|
||||
# --- Shell-obfuscation tripwire ---
|
||||
# Every check below inspects the command as a STRING, but bash executes what the
|
||||
# string MEANS after expansion. ${IFS} holds the default field separator and
|
||||
# contains no literal whitespace, so
|
||||
#
|
||||
# rm${IFS}-rf${IFS}/
|
||||
#
|
||||
# matches none of the `rm\s+` patterns while executing as a full recursive
|
||||
# delete. The same holds for a command assembled by a base64 decode piped to a
|
||||
# shell. Rather than try to out-parse bash, treat these splitting/decoding
|
||||
# primitives as a reason to ask: they are vanishingly rare in commands a human
|
||||
# actually means to run unattended.
|
||||
if printf '%s' "$CMD" | grep -qE '\$\{IFS\}|\$IFS|\$\(echo[^)]*base64[^)]*\)|base64[[:space:]]+(-d|--decode)[^|]*\|[[:space:]]*(sh|bash)' 2>/dev/null; then
|
||||
printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"[careful] Shell obfuscation detected (IFS word-splitting or base64-to-shell). Read the command carefully before approving."}}\n'
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# --- Check for safe exceptions (one standalone rm of build artifacts) ---
|
||||
# Match the complete command. Parsing only the last rm is unsafe because shell
|
||||
# syntax or comments can hide an earlier destructive command, for example:
|
||||
|
|
@ -39,10 +87,20 @@ CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]')
|
|||
# ENDS in a whitelisted suffix (`rm -rf $(./wipe-all)/node_modules`)
|
||||
# cannot ride the whitelist. Plain $VAR expansion (no parenthesis) is
|
||||
# still allowed.
|
||||
if printf '%s' "$CMD" | grep -qE '^[[:space:]]*rm[[:space:]]+(-[a-zA-Z]*[rR][a-zA-Z]*[[:space:]]+|--recursive[[:space:]]+)(([^[:space:];&|#(`]*/)?(node_modules|\.next|dist|__pycache__|\.cache|build|\.turbo|coverage)[[:space:]]*)+$' 2>/dev/null; then
|
||||
echo '{}'
|
||||
exit 0
|
||||
fi
|
||||
# - multi-line commands never ride the whitelist: grep matches the anchored
|
||||
# shape against EACH line, so `rm -rf /\nrm -rf node_modules` would be
|
||||
# allowed by its second line. With the JSON-parser extraction the \n in
|
||||
# the payload is a real newline (the old grep extractor kept it as two
|
||||
# literal characters, which broke the anchored match by accident).
|
||||
case "$CMD" in
|
||||
*$'\n'*) : ;; # multi-line: fall through to the destructive checks
|
||||
*)
|
||||
if printf '%s' "$CMD" | grep -qE '^[[:space:]]*rm[[:space:]]+(-[a-zA-Z]*[rR][a-zA-Z]*[[:space:]]+|--recursive[[:space:]]+)(([^[:space:];&|#(`]*/)?(node_modules|\.next|dist|__pycache__|\.cache|build|\.turbo|coverage)[[:space:]]*)+$' 2>/dev/null; then
|
||||
echo '{}'
|
||||
exit 0
|
||||
fi
|
||||
;;
|
||||
esac
|
||||
|
||||
# --- Destructive pattern checks ---
|
||||
WARN=""
|
||||
|
|
|
|||
|
|
@ -161,6 +161,21 @@ describe('check-careful.sh', () => {
|
|||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
|
||||
// The old grep extractor stopped at the first escaped quote in the JSON
|
||||
// string, so any quoted argument truncated the command BEFORE the pattern
|
||||
// checks ran — hiding everything after it. (#2426)
|
||||
test.each([
|
||||
'git commit -m "wip" && rm -rf /',
|
||||
'bash -c "rm -rf /"',
|
||||
'echo "x"; rm -rf ~',
|
||||
'npm run build --msg "done" && rm -rf /',
|
||||
])('a quoted argument cannot hide a later destructive command: %s', (command) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete');
|
||||
});
|
||||
|
||||
// JSON-escaped newline (literal two-char \n surviving the grep extraction
|
||||
// path) breaks the anchored whitelist shape → falls through to the warn.
|
||||
test('newline-chained rm warns (escaped-newline separator branch)', () => {
|
||||
|
|
@ -197,11 +212,62 @@ describe('check-careful.sh', () => {
|
|||
});
|
||||
});
|
||||
|
||||
// --- Shell obfuscation ---
|
||||
|
||||
describe('shell obfuscation', () => {
|
||||
test.each([
|
||||
'rm${IFS}-rf${IFS}/',
|
||||
'rm$IFS-rf$IFS/',
|
||||
'echo cm0gLXJmIC8= | base64 -d | sh',
|
||||
])('asks when the command hides its shape behind expansion: %s', (command) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('obfuscation');
|
||||
});
|
||||
|
||||
test('ordinary commands are unaffected', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('cat file.b64 | base64 -d > out.bin'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// --- JSON payload extraction ---
|
||||
|
||||
describe('command extraction', () => {
|
||||
test('fails closed when the payload is not valid JSON', () => {
|
||||
const { exitCode, output } = runHookRaw(CAREFUL_SCRIPT, 'this is not json');
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('parse');
|
||||
});
|
||||
|
||||
test('allows a well-formed payload with no command field', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, { tool_input: { file_path: '/tmp/x' } });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
|
||||
test('allows when command is present but not a string', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, { tool_input: { command: 42 } });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
|
||||
test('preserves escaped quotes in the extracted command', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('echo "hello world"'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// --- SQL destructive commands ---
|
||||
// Note: SQL commands that contain embedded double quotes (e.g., psql -c "DROP TABLE")
|
||||
// get their command value truncated by the grep-based JSON extractor because \"
|
||||
// terminates the [^"]* match. We use commands WITHOUT embedded quotes so the grep
|
||||
// extraction works and the SQL keywords are visible to the pattern matcher.
|
||||
// Embedded double quotes are now safe to use here. They previously truncated the
|
||||
// extracted command (the grep-based extractor stopped at the first \"), which hid
|
||||
// the SQL keyword from the pattern matcher — so the older tests had to be written
|
||||
// without quotes, in a shape no one actually types. The JSON-parser extraction
|
||||
// fixed that, and the quoted forms below are the realistic ones.
|
||||
|
||||
describe('SQL destructive commands', () => {
|
||||
test('psql DROP TABLE warns with DROP in message', () => {
|
||||
|
|
@ -211,6 +277,16 @@ describe('check-careful.sh', () => {
|
|||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('DROP');
|
||||
});
|
||||
|
||||
test.each([
|
||||
'psql -c "DROP TABLE users"',
|
||||
'psql -c "TRUNCATE orders"',
|
||||
'mysql -e "DROP DATABASE prod"',
|
||||
])('a quoted SQL statement is still inspected: %s', (command) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
});
|
||||
|
||||
test('mysql drop database warns (case insensitive)', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('mysql -e drop database mydb'));
|
||||
expect(exitCode).toBe(0);
|
||||
|
|
@ -325,10 +401,13 @@ describe('check-careful.sh', () => {
|
|||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
|
||||
test('malformed JSON input allows gracefully (exit 0, output {})', () => {
|
||||
const { exitCode, raw } = runHookRaw(CAREFUL_SCRIPT, 'this is not json at all{{{{');
|
||||
test('malformed JSON input fails CLOSED (asks instead of allowing)', () => {
|
||||
// Pre-#2426 this allowed (`{}`) — a hook that gates destructive commands
|
||||
// must not allow-by-default on input it cannot read.
|
||||
const { exitCode, output } = runHookRaw(CAREFUL_SCRIPT, 'this is not json at all{{{{');
|
||||
expect(exitCode).toBe(0);
|
||||
expect(raw).toBe('{}');
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('parse');
|
||||
});
|
||||
|
||||
test('Python fallback: grep fails on multiline JSON, Python parses it', () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue