122 lines
4.8 KiB
Markdown
122 lines
4.8 KiB
Markdown
---
|
|
phase: 03-sandbox-aware-prompting
|
|
reviewed: 2026-04-09T12:00:00Z
|
|
depth: standard
|
|
files_reviewed: 1
|
|
files_reviewed_list:
|
|
- claudebox.sh
|
|
findings:
|
|
critical: 1
|
|
warning: 3
|
|
info: 1
|
|
total: 5
|
|
status: issues_found
|
|
---
|
|
|
|
# Phase 3: Code Review Report
|
|
|
|
**Reviewed:** 2026-04-09
|
|
**Depth:** standard
|
|
**Files Reviewed:** 1
|
|
**Status:** issues_found
|
|
|
|
## Summary
|
|
|
|
Reviewed `claudebox.sh`, the main wrapper script for sandboxed Claude Code execution. The script is well-structured with a clear security model (allowlist-based env, bwrap isolation, secret masking). Found one critical resource leak caused by `exec` bypassing the EXIT trap, two security-adjacent warnings around gitconfig trust and the env escape hatch, and a masking logic issue that can leak most of a short secret.
|
|
|
|
## Critical Issues
|
|
|
|
### CR-01: EXIT trap never fires -- temp gitconfig leaks on every invocation
|
|
|
|
**File:** `claudebox.sh:160-161`
|
|
**Issue:** The script sets `trap 'rm -f "$GITCONFIG_TMP"' EXIT` at line 160, but the script always terminates via `exec bwrap` at line 326. `exec` replaces the current process, so the EXIT trap never executes. A new temp file is leaked in `/tmp` on every claudebox invocation.
|
|
**Fix:**
|
|
The gitconfig tmpfile is bind-mounted read-only into the sandbox, so it must exist for the lifetime of the bwrap process. Clean it up by running bwrap in the background, waiting, then cleaning up -- or write it to a deterministic path that gets overwritten each launch:
|
|
```bash
|
|
# Option A: Use a fixed path instead of mktemp (simplest)
|
|
GITCONFIG_TMP="$HOME/.claudebox/.gitconfig.tmp"
|
|
# Remove the trap entirely -- file is overwritten each launch
|
|
|
|
# Option B: Fork instead of exec, clean up after
|
|
bwrap ... &
|
|
BWRAP_PID=$!
|
|
wait "$BWRAP_PID"
|
|
EXIT_CODE=$?
|
|
rm -f "$GITCONFIG_TMP"
|
|
exit "$EXIT_CODE"
|
|
```
|
|
Option A is recommended -- it eliminates the leak with no complexity cost and the file lives in the user-owned `.claudebox` directory.
|
|
|
|
## Warnings
|
|
|
|
### WR-01: `safe.directory = *` trusts all git directories
|
|
|
|
**File:** `claudebox.sh:167`
|
|
**Issue:** The generated gitconfig sets `safe.directory = *`, which tells git to trust ownership of any directory. While the sandbox limits mounted paths, this is broader than necessary -- only `$CWD` needs to be trusted. If mount scope changes in the future, this becomes a real risk.
|
|
**Fix:**
|
|
```bash
|
|
cat > "$GITCONFIG_TMP" <<GITEOF
|
|
[user]
|
|
name = $GIT_NAME
|
|
email = $GIT_EMAIL
|
|
[safe]
|
|
directory = $CWD
|
|
GITEOF
|
|
```
|
|
|
|
### WR-02: CLAUDEBOX_EXTRA_ENV can smuggle secrets past the allowlist
|
|
|
|
**File:** `claudebox.sh:213-223`
|
|
**Issue:** The `CLAUDEBOX_EXTRA_ENV` escape hatch passes any named host variable into the sandbox without validation. A parent process (or shell profile) setting `CLAUDEBOX_EXTRA_ENV=AWS_SECRET_ACCESS_KEY,GH_TOKEN` would inject secrets that the core allowlist intentionally excludes. The audit display provides visibility but not prevention.
|
|
**Fix:** Consider a denylist check for known secret-bearing variable names:
|
|
```bash
|
|
DENY_PATTERN="^(AWS_|GH_TOKEN|GITHUB_TOKEN|SSH_|GPG_|AGE_)"
|
|
for var in "${EXTRAS[@]}"; do
|
|
var="${var// /}"
|
|
if [[ "$var" =~ $DENY_PATTERN ]]; then
|
|
echo "${RED}Blocked${RESET} $var -- secret variable cannot be passed via CLAUDEBOX_EXTRA_ENV" >&2
|
|
continue
|
|
fi
|
|
# ... existing logic
|
|
done
|
|
```
|
|
|
|
### WR-03: mask_value leaks most of short secrets
|
|
|
|
**File:** `claudebox.sh:83-86`
|
|
**Issue:** The masking threshold is 11 characters. For a 12-character secret, `mask_value` displays the first 7 and last 4 characters (11 of 12 visible). This effectively leaks the entire value. The issue scales: a 15-char secret shows 11 of 15 characters.
|
|
**Fix:** Raise the threshold so the masked portion is always the majority of the value:
|
|
```bash
|
|
mask_value() {
|
|
local name="$1" value="$2"
|
|
local upper="${name^^}"
|
|
if [[ "$upper" == *KEY* || "$upper" == *TOKEN* || "$upper" == *SECRET* || "$upper" == *PASSWORD* || "$upper" == *CREDENTIAL* ]]; then
|
|
if (( ${#value} > 20 )); then
|
|
echo "${value:0:4}...${value: -4}"
|
|
else
|
|
echo "***"
|
|
fi
|
|
else
|
|
echo "$value"
|
|
fi
|
|
}
|
|
```
|
|
|
|
## Info
|
|
|
|
### IN-01: --dangerously-skip-permissions is always passed to claude
|
|
|
|
**File:** `claudebox.sh:287`
|
|
**Issue:** The `--dangerously-skip-permissions` flag is unconditionally added to the Claude command. This is intentional per the design (bwrap IS the permission boundary), but it means the sandbox's security depends entirely on the bwrap mount list being correct. Worth documenting this invariant prominently.
|
|
**Fix:** Add a comment explaining the security invariant:
|
|
```bash
|
|
# Security invariant: --dangerously-skip-permissions is safe here because
|
|
# bwrap's mount list is the security boundary, not Claude's permission system.
|
|
# If you change the mount list, audit what Claude can access.
|
|
```
|
|
|
|
---
|
|
|
|
_Reviewed: 2026-04-09_
|
|
_Reviewer: Claude (gsd-code-reviewer)_
|
|
_Depth: standard_
|