Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clawpinch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ else
log_info "Piping $_non_ok_count findings to Claude for remediation..."
echo "$_non_ok_findings" | "$_claude_bin" -p \
--allowedTools "Bash,Read,Write,Edit,Glob,Grep" \
"You are a security remediation agent. You have been given ClawPinch security scan findings as JSON. For each finding: 1) Read the evidence to understand the issue 2) Apply the auto_fix command if available, otherwise implement the remediation manually 3) Verify the fix. Work through findings in order (critical first). Be precise and minimal in your changes."
"You are a security remediation agent. You have been given ClawPinch security scan findings as JSON. For each finding: 1) Read the evidence to understand the issue 2) Apply the auto_fix command if available, otherwise implement the remediation manually 3) Verify the fix. Work through findings in order (critical first). Be precise and minimal in your changes. IMPORTANT: All Bash commands executed through auto_fix are validated against a command allowlist defined in .auto-claude-security.json. Commands not in the allowlist will be rejected with an error message. Only use allowlisted commands such as jq, grep, sed, chmod, and other standard tools for fixes."

Choose a reason for hiding this comment

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

security-high high

The AI remediation pipeline grants the Claude CLI broad tool permissions (Bash, Read, Write, Edit, Glob, Grep) without enforcing the command allowlist at the tool level. While the prompt instructs the AI to follow the allowlist, there is no technical enforcement for the AI's Bash tool. A malicious finding could use prompt injection to trick the AI into executing arbitrary commands via its Bash tool, bypassing the validation logic implemented in the bash scripts.

else
log_info "No actionable findings for remediation."
fi
Expand Down
106 changes: 106 additions & 0 deletions scripts/helpers/common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,112 @@ require_cmd() {
fi
}

# ─── Command validation (allowlist) ─────────────────────────────────────────

validate_command() {
# Usage: validate_command <command_string>
# Returns 0 if ALL commands in the string are in allowlist, 1 otherwise
local cmd_string="$1"

if [[ -z "$cmd_string" ]]; then
log_error "validate_command: empty command string"
return 1
fi

# Find security config file (walk up from cwd to root)
local security_file=""
local dir
dir="$(pwd)"
while true; do
if [[ -f "$dir/.auto-claude-security.json" ]]; then
security_file="$dir/.auto-claude-security.json"
break
fi
if [[ "$dir" == "/" ]]; then
break
fi
dir="$(dirname "$dir")"
done

if [[ -z "$security_file" ]]; then
log_error "validate_command: .auto-claude-security.json not found"
return 1
fi
Comment on lines +228 to +237
Copy link

Choose a reason for hiding this comment

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

Missing configuration file .auto-claude-security.json breaks validation. File is gitignored but required for validate_command() to work.

Suggested change
if [[ -z "$security_file" ]]; then
log_error "validate_command: .auto-claude-security.json not found"
return 1
fi
if [[ -z "$security_file" ]]; then
log_error "validate_command: .auto-claude-security.json not found. Run 'clawpinch.sh --init' to create default config."
return 1
fi
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/common.sh
Line: 151:154

Comment:
Missing configuration file `.auto-claude-security.json` breaks validation. File is gitignored but required for `validate_command()` to work.

```suggestion
  if [[ -z "$security_file" ]]; then
    log_error "validate_command: .auto-claude-security.json not found. Run 'clawpinch.sh --init' to create default config."
    return 1
  fi
```

How can I resolve this? If you propose a fix, please make it concise.


# Check if jq is available
if ! has_cmd jq; then
log_error "validate_command: jq is required but not installed"
return 1
fi

# Get all allowed commands from security config
local allowed_commands
allowed_commands="$(jq -r '
(.base_commands // []) +
(.stack_commands // []) +
(.script_commands // []) +
(.custom_commands // []) |
.[]
' "$security_file" 2>/dev/null)"

if [[ -z "$allowed_commands" ]]; then
log_error "validate_command: failed to parse security config"
return 1
fi
Comment on lines +291 to +302

Choose a reason for hiding this comment

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

high

The current logic for parsing the .auto-claude-security.json file incorrectly treats a valid, empty allowlist as a parsing error. An empty allowlist should be a valid configuration, meaning no commands are permitted. The current implementation checks if the jq output is empty, which is true for both a jq parsing error and a valid empty list of commands. This can be fixed by separating the JSON validity check from the command extraction and relying on jq's exit code to detect parsing errors. This makes the function more robust and correctly handles the empty allowlist case.

Suggested change
allowed_commands="$(jq -r '
(.base_commands // []) +
(.stack_commands // []) +
(.script_commands // []) +
(.custom_commands // []) |
.[]
' "$security_file" 2>/dev/null)"
if [[ -z "$allowed_commands" ]]; then
log_error "validate_command: failed to parse security config"
return 1
fi
if ! jq '.' "$security_file" >/dev/null 2>&1; then
log_error "validate_command: security config file '$security_file' contains invalid JSON."
return 1
fi
allowed_commands="$(jq -r '
(.base_commands // []) +
(.stack_commands // []) +
(.script_commands // []) +
(.custom_commands // []) |
.[]
' "$security_file")"


# Extract ALL commands from the string (split by |, &&, ||, ;)
# This ensures we validate every command in a chain
# Try to use Python script for proper quote-aware parsing
local script_dir
script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
local parse_script="$script_dir/parse_commands.py"

# Require Python parser — fail closed if unavailable (no insecure fallback)
if ! [[ -f "$parse_script" ]] || ! has_cmd python3; then
log_error "validate_command: python3 or parse_commands.py not available. Cannot securely validate command."
return 1
fi

local base_commands_list
base_commands_list="$(python3 "$parse_script" "$cmd_string" 2>/dev/null)"
if [[ $? -ne 0 || -z "$base_commands_list" ]]; then
log_error "validate_command: Python helper failed to parse command string."
return 1
fi

# Check each base command
while IFS= read -r base_cmd; do
# Skip empty lines
[[ -z "$base_cmd" ]] && continue

# Skip flags/options (start with -)
[[ "$base_cmd" =~ ^- ]] && continue

# Skip quoted strings (they're arguments, not commands)
[[ "$base_cmd" =~ ^[\'\"] ]] && continue

Choose a reason for hiding this comment

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

security-critical critical

This logic explicitly skips any command token that starts with a quote character, assuming it is an argument rather than a command. However, because the original command string is later executed using eval (e.g., in interactive.sh), this creates a critical Remote Code Execution (RCE) vulnerability. An attacker can provide a command string like "'$(id)'". The shlex.split call in the Python helper will return '$(id)' as the first token. This line will then skip validation for that token, and eval will subsequently execute the command substitution $(id). Quoted strings should be validated against the allowlist after stripping the quotes, rather than being skipped entirely.


# Block path-based commands (/bin/rm, ./malicious, ~/script) — prevents allowlist bypass
if [[ "$base_cmd" =~ ^[/~\.] ]]; then
log_error "validate_command: path-based command '$base_cmd' is not allowed (use bare command names)"
return 1
fi

# Skip redirection operators
case "$base_cmd" in
'>'|'>>'|'<'|'2>'|'&>'|'2>&1') continue ;;
esac

Choose a reason for hiding this comment

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

security-high high

The validator explicitly skips redirection operators (>, >>, <, etc.), allowing them to pass validation even if they are not in the allowlist. This creates a critical vulnerability for unauthorized file operations, as these operators can appear at the start of a command segment (e.g., > /etc/passwd). These should be strictly blocked or explicitly included in the allowlist if required. Furthermore, there's a logic issue in the command validation that prevents it from working as intended for path-based scripts. The current implementation first blocks any command that looks like a path (e.g., starting with ./) and only then checks against the allowlist. This means an allowed script like ./clawpinch.sh from script_commands would be incorrectly blocked. The correct approach is to first check if the command is on the allowlist.


# Check if this command is in the allowlist (exact match)
if ! grep -Fxq -- "$base_cmd" <<< "$allowed_commands"; then
log_error "validate_command: '$base_cmd' is not in the allowlist"
return 1
fi
done <<< "$base_commands_list"

# All commands validated successfully
return 0
}

# ─── OS detection ───────────────────────────────────────────────────────────

detect_os() {
Expand Down
22 changes: 21 additions & 1 deletion scripts/helpers/interactive.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ _confirm() {

_run_fix() {
local cmd="$1"

# Validate command against allowlist
if ! validate_command "$cmd"; then
printf '\n %b✗ Command not in allowlist: %s%b\n' "$_CLR_CRIT" "$cmd" "$_CLR_RST"
return 1
fi

printf '\n %b$%b %s\n' "$_CLR_DIM" "$_CLR_RST" "$cmd"
if eval "$cmd" 2>&1 | while IFS= read -r line; do printf ' %s\n' "$line"; done; then
printf ' %b✓ Fix applied successfully%b\n' "$_CLR_OK" "$_CLR_RST"
Expand Down Expand Up @@ -479,7 +486,12 @@ review_findings() {
if (( has_fix )); then
printf '\n Command: %b%s%b\n' "$_CLR_DIM" "$f_auto_fix" "$_CLR_RST"
if _confirm ' Run this? [y/n]: '; then
_run_fix "$f_auto_fix"
# Validate command against allowlist before execution
if ! validate_command "$f_auto_fix"; then
printf ' %b✗ Command not in allowlist - execution blocked%b\n' "$_CLR_CRIT" "$_CLR_RST"
else
_run_fix "$f_auto_fix"
fi

Choose a reason for hiding this comment

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

medium

There is a redundant call to validate_command here. The _run_fix function, which is called within this block, already performs this validation. Calling it twice is unnecessary and can lead to maintenance issues if the validation logic changes in the future. The validation should be encapsulated within _run_fix to have a single point of enforcement before execution.

Suggested change
if ! validate_command "$f_auto_fix"; then
printf ' %b✗ Command not in allowlist - execution blocked%b\n' "$_CLR_CRIT" "$_CLR_RST"
else
_run_fix "$f_auto_fix"
fi
_run_fix "$f_auto_fix"

else
printf ' Skipped.\n'
fi
Expand Down Expand Up @@ -561,6 +573,14 @@ auto_fix_all() {
f_id="$(echo "$fixable" | jq -r ".[$i].id")"
f_cmd="$(echo "$fixable" | jq -r ".[$i].auto_fix")"
printf ' [%d/%d] %s ... ' $(( i + 1 )) "$fix_count" "$f_id"

# Validate command against allowlist
if ! validate_command "$f_cmd"; then
printf '%b✗ blocked (not in allowlist)%b\n' "$_CLR_CRIT" "$_CLR_RST"
failed=$(( failed + 1 ))
continue
fi

if eval "$f_cmd" >/dev/null 2>&1; then
printf '%b✓ pass%b\n' "$_CLR_OK" "$_CLR_RST"
passed=$(( passed + 1 ))
Expand Down
115 changes: 115 additions & 0 deletions scripts/helpers/parse_commands.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
#!/usr/bin/env python3
"""Parse shell command string and extract base commands while respecting quotes.

Security: rejects commands containing dangerous shell constructs that could
hide malicious commands (command substitution, process substitution, backticks).
"""
import sys
import shlex
import re


# Patterns that indicate hidden command execution — reject the entire string
_DANGEROUS_PATTERNS = [
r'\$\(', # command substitution: $(...)
r'`', # backtick command substitution: `...`
r'<\(', # process substitution: <(...)
r'>\(', # process substitution: >(...)
]

_DANGEROUS_RE = re.compile('|'.join(_DANGEROUS_PATTERNS))


def extract_commands(cmd_string):
"""Extract all base commands from a shell command string.

Raises ValueError if the command string contains dangerous shell
constructs that could hide commands from validation.
"""
# Reject strings containing command/process substitution or backticks
# Check outside of single-quoted regions (single quotes prevent expansion)
# Simple approach: check the raw string — even quoted $() is suspicious
# in an auto-fix context and should be rejected
if _DANGEROUS_RE.search(cmd_string):
raise ValueError(
f"Command string contains dangerous shell construct: {cmd_string!r}"
Copy link

Choose a reason for hiding this comment

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

Legitimate auto-fix commands might contain $(...) in quoted strings that won't be expanded. Rejecting them outright prevents valid fixes like sed 's/$(pwd)/\/path/g'.

Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/parse_commands.py
Line: 31:35

Comment:
Legitimate auto-fix commands might contain `$(...)` in quoted strings that won't be expanded. Rejecting them outright prevents valid fixes like `sed 's/$(pwd)/\/path/g'`.

How can I resolve this? If you propose a fix, please make it concise.

)

commands = []

# Split by command separators: |, &&, ||, ;, &
# Use a simple state machine to handle quotes
in_single = False
in_double = False
current = ""
i = 0

while i < len(cmd_string):
c = cmd_string[i]

# Track quote state
if c == "'" and not in_double:
in_single = not in_single
current += c
elif c == '"' and not in_single:
in_double = not in_double
current += c
# Check for separators outside quotes
elif not in_single and not in_double:
if i < len(cmd_string) - 1 and cmd_string[i:i+2] in ['&&', '||']:
if current.strip():
commands.append(current.strip())
current = ""
i += 1 # skip second char
elif c in ['|', ';']:
if current.strip():
commands.append(current.strip())
current = ""
elif c == '&':
# Background operator — treat as separator
if current.strip():
commands.append(current.strip())
current = ""
elif c == '\n':
# Newline — treat as separator
if current.strip():
commands.append(current.strip())
current = ""
else:
current += c
else:
current += c

i += 1
Comment on lines +91 to +133

Choose a reason for hiding this comment

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

security-critical critical

The command parser fails to handle backslash-escaped quotes, allowing an attacker to bypass the command allowlist. By using an escaped quote (e.g., \"), an attacker can trick the state machine into incorrectly toggling the quote state, thereby hiding command separators like ; or && from the validation logic. This leads to arbitrary command execution when the string is later processed by eval in the bash scripts.

    while i < len(cmd_string):
        c = cmd_string[i]

        # Handle backslash escape (only outside single quotes)
        if c == "\\" and not in_single and i + 1 < len(cmd_string):
            current += c + cmd_string[i+1]
            i += 2
            continue

        # Track quote state
        if c == "'" and not in_double:
            in_single = not in_single
            current += c
        elif c == '"' and not in_single:
            in_double = not in_double
            current += c
        # Check for separators outside quotes
        elif not in_single and not in_double:
            if i < len(cmd_string) - 1 and cmd_string[i:i+2] in ['&&', '||']:
                if current.strip():
                    commands.append(current.strip())
                current = ""
                i += 1  # skip second char
            elif c in ['|', ';']:
                if current.strip():
                    commands.append(current.strip())
                current = ""
            elif c == '&':
                # Background operator — treat as separator
                if current.strip():
                    commands.append(current.strip())
                current = ""
            elif c == '\n':
                # Newline — treat as separator
                if current.strip():
                    commands.append(current.strip())
                current = ""
            else:
                current += c
        else:
            current += c

        i += 1


if current.strip():
commands.append(current.strip())

# Extract base command from each segment
base_commands = []
for cmd in commands:
try:
# Use shlex to properly parse the command
tokens = shlex.split(cmd)
if tokens:
base_commands.append(tokens[0])
except ValueError:
# If shlex fails, reject — don't fall back to insecure parsing
raise ValueError(f"Failed to parse command segment: {cmd!r}")

return base_commands

if __name__ == "__main__":
if len(sys.argv) > 1:
cmd = " ".join(sys.argv[1:])
else:
cmd = sys.stdin.read().strip()

try:
commands = extract_commands(cmd)
except ValueError as e:
print(f"ERROR: {e}", file=sys.stderr)
sys.exit(1)

for c in commands:
print(c)
Loading