fix: inject NAT rules into child containers to prevent proxy bypass#138
fix: inject NAT rules into child containers to prevent proxy bypass#138
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
Smoke Test Results (Run ID: 20378118736)Last 2 merged PRs:
Tests:
Status: PASS
|
Test Coverage Report
Coverage ThresholdsThe project has the following coverage thresholds configured:
Coverage report generated by `npm run test:coverage` |
|
@copilot figure out why https://github.com/githubnext/gh-aw-firewall/actions/runs/20378118743/job/58561726010?pr=138 is failing |
The CI failure is not related to my PR changes. The failure is in the "Smoke Claude" workflow where the The agent logs show:
This is an infrastructure/configuration issue with the playwright MCP setup in CI, not related to the NAT rules injection for child containers that I implemented. My changes only affect |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability where child containers spawned by the agent could bypass proxy-based egress filtering by ignoring HTTP_PROXY environment variables. The fix injects iptables NAT rules directly into child containers to force all HTTP/HTTPS traffic through the Squid proxy, regardless of application proxy settings.
Changes:
- Enhanced
docker-wrapper.shto inject NAT rules into child containers using inline iptables setup - Added
--cap-add NET_ADMINto all spawned containers to enable NAT rule configuration - Implemented robust shell argument escaping using
printf %qto safely wrap user commands - Added comprehensive integration tests verifying NAT rule injection and proxy bypass prevention
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| containers/agent/docker-wrapper.sh | Implemented NAT rule injection via entrypoint wrapping, argument parsing functions, and shell escaping utilities |
| tests/integration/docker-egress.test.ts | Added test suite validating NAT rules are applied and prevent proxy bypass even when environment variables are ignored |
Comments suppressed due to low confidence (3)
containers/agent/docker-wrapper.sh:112
- The special handling of
--networkand--netoptions based on theinclude_networkparameter is confusing. Wheninclude_networkis false, these options are silently dropped fromPARSED_DOCKER_OPTS, which could be unexpected behavior. Consider documenting this behavior or refactoring to make the intent clearer.
if [ "$include_network" = "true" ]; then
if [ "$arg" = "--network" ] || [ "$arg" = "--net" ]; then
PARSED_DOCKER_OPTS+=("$arg")
skip_next=true
continue
fi
fi
containers/agent/docker-wrapper.sh:211
- Overriding the entrypoint to
sh(line 235, 284) will break containers that rely on specific entrypoints (e.g., containers with initialization scripts, signal handlers, or custom entrypoints). This is a breaking change that could affect existing workflows. Users who explicitly set--entrypointin their docker run commands will have it silently overridden. Consider detecting existing--entrypointflags in the arguments and either preserving them with a wrapper or warning the user.
if [ -z "$PARSED_IMAGE" ]; then
tests/integration/docker-egress.test.ts:400
- This test verifies NAT rules are present by checking for 'DNAT' and '172.30.0.10:3128' in iptables output. However, it doesn't verify that the specific HTTP (port 80) and HTTPS (port 443) rules are present, or that the RETURN rules for DNS and localhost are correctly configured. Consider adding more specific assertions to validate the complete NAT rule set.
`docker run --rm alpine:latest sh -c 'apk add --no-cache iptables >/dev/null 2>&1 && iptables -t nat -L OUTPUT -n 2>/dev/null | grep DNAT || echo "No NAT rules"'`,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
containers/agent/docker-wrapper.sh
Outdated
| local arg="$1" | ||
| case "$arg" in | ||
| -e|--env|-l|--label|-v|--volume|-p|--publish|--name|--hostname|\ | ||
| --user|-u|--workdir|-w|--mount|--network-alias|--dns|--dns-search|\ |
There was a problem hiding this comment.
The is_docker_option_with_value function is missing --network and --net in its list of options that take values. This causes the parsing logic in parse_docker_run_args to need special handling for network options (lines 105-112). Add --network and --net to the case statement for consistency, or document why they need special handling.
This issue also appears in the following locations of the same file:
- line 106
| --user|-u|--workdir|-w|--mount|--network-alias|--dns|--dns-search|\ | |
| --user|-u|--workdir|-w|--mount|--network|--net|--network-alias|--dns|--dns-search|\ |
containers/agent/docker-wrapper.sh
Outdated
| # It's designed to be minimal and work with busybox/alpine shells | ||
| # Note: The script ends with a semicolon to ensure proper command separation | ||
| NAT_SETUP_SCRIPT='if command -v iptables >/dev/null 2>&1; then iptables -t nat -F OUTPUT 2>/dev/null || true; iptables -t nat -A OUTPUT -o lo -j RETURN; iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; iptables -t nat -A OUTPUT -d '"$SQUID_IP"' -j RETURN; iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; fi; ' | ||
|
|
||
| # Function to escape a command argument for use in sh -c | ||
| # Uses printf %q for robust escaping of all shell metacharacters | ||
| escape_for_shell() { | ||
| local arg="$1" | ||
| # Use printf %q for proper shell escaping |
There was a problem hiding this comment.
printf %q is a bash-specific feature that is not available in POSIX sh or busybox ash (used in Alpine containers). While this function runs in the bash wrapper script (not in the child containers), the comment on line 17 states the NAT setup script is 'designed to be minimal and work with busybox/alpine shells', which could be misleading. Consider clarifying that printf %q is only used in the wrapper bash script for escaping, not in the injected sh script.
| # It's designed to be minimal and work with busybox/alpine shells | |
| # Note: The script ends with a semicolon to ensure proper command separation | |
| NAT_SETUP_SCRIPT='if command -v iptables >/dev/null 2>&1; then iptables -t nat -F OUTPUT 2>/dev/null || true; iptables -t nat -A OUTPUT -o lo -j RETURN; iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; iptables -t nat -A OUTPUT -d '"$SQUID_IP"' -j RETURN; iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; fi; ' | |
| # Function to escape a command argument for use in sh -c | |
| # Uses printf %q for robust escaping of all shell metacharacters | |
| escape_for_shell() { | |
| local arg="$1" | |
| # Use printf %q for proper shell escaping | |
| # The injected script itself is POSIX-sh compatible and designed to work with busybox/alpine shells. | |
| # Note: The script ends with a semicolon to ensure proper command separation | |
| NAT_SETUP_SCRIPT='if command -v iptables >/dev/null 2>&1; then iptables -t nat -F OUTPUT 2>/dev/null || true; iptables -t nat -A OUTPUT -o lo -j RETURN; iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; iptables -t nat -A OUTPUT -d '"$SQUID_IP"' -j RETURN; iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; fi; ' | |
| # Function to escape a command argument for use in sh -c inside this bash wrapper | |
| # Note: Uses bash-specific "printf %q" for robust escaping; this runs only in the wrapper, | |
| # and is not part of the injected NAT setup script executed in child containers. | |
| escape_for_shell() { | |
| local arg="$1" | |
| # Use printf %q for proper shell escaping in this bash script |
containers/agent/docker-wrapper.sh
Outdated
| # This script redirects HTTP/HTTPS traffic to Squid proxy using iptables | ||
| # It's designed to be minimal and work with busybox/alpine shells | ||
| # Note: The script ends with a semicolon to ensure proper command separation | ||
| NAT_SETUP_SCRIPT='if command -v iptables >/dev/null 2>&1; then iptables -t nat -F OUTPUT 2>/dev/null || true; iptables -t nat -A OUTPUT -o lo -j RETURN; iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; iptables -t nat -A OUTPUT -d '"$SQUID_IP"' -j RETURN; iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; fi; ' |
There was a problem hiding this comment.
This 600+ character single-line script is extremely difficult to read and maintain. Consider using a heredoc or multi-line string with line continuation for better readability. The script could also be extracted to a separate file that gets mounted or copied into containers for easier maintenance and testing.
| NAT_SETUP_SCRIPT='if command -v iptables >/dev/null 2>&1; then iptables -t nat -F OUTPUT 2>/dev/null || true; iptables -t nat -A OUTPUT -o lo -j RETURN; iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; iptables -t nat -A OUTPUT -d '"$SQUID_IP"' -j RETURN; iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination '"$SQUID_IP:$SQUID_PORT"'; fi; ' | |
| NAT_SETUP_SCRIPT="if command -v iptables >/dev/null 2>&1; then \ | |
| iptables -t nat -F OUTPUT 2>/dev/null || true; \ | |
| iptables -t nat -A OUTPUT -o lo -j RETURN; \ | |
| iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p udp -d 127.0.0.11 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p tcp -d 127.0.0.11 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p udp -d 8.8.8.8 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p tcp -d 8.8.8.8 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p udp -d 8.8.4.4 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p tcp -d 8.8.4.4 --dport 53 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -d 8.8.8.8 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -d 8.8.4.4 -j RETURN; \ | |
| iptables -t nat -A OUTPUT -d $SQUID_IP -j RETURN; \ | |
| iptables -t nat -A OUTPUT -p tcp --dport 80 -j DNAT --to-destination $SQUID_IP:$SQUID_PORT; \ | |
| iptables -t nat -A OUTPUT -p tcp --dport 443 -j DNAT --to-destination $SQUID_IP:$SQUID_PORT; \ | |
| fi; " |
containers/agent/docker-wrapper.sh
Outdated
| if [ ${#PARSED_USER_CMD[@]} -gt 0 ]; then | ||
| # User specified a command - wrap it with NAT setup | ||
| escaped_cmd=$(build_escaped_cmd "${PARSED_USER_CMD[@]}") | ||
| wrapped_cmd="${NAT_SETUP_SCRIPT}exec ${escaped_cmd}" |
There was a problem hiding this comment.
The NAT_SETUP_SCRIPT contains variable expansions for $SQUID_IP and $SQUID_PORT that occur when the wrapper script runs, but the expanded values are then embedded into a string that gets passed to sh -c in the child container. If these variables ever contain shell metacharacters, this could lead to injection vulnerabilities. While unlikely with IP addresses and port numbers, consider using the SQUID_PROXY_IP and SQUID_PROXY_PORT environment variables that are already being passed to the container (lines 232-233, 281-282) instead of embedding them in the script string.
containers/agent/docker-wrapper.sh
Outdated
| # No user command - can't easily wrap the default entrypoint | ||
| echo "[$(date -Iseconds)] WARNING: No command specified, NAT rules may not apply to default entrypoint" >> "$LOG_FILE" | ||
| exec /usr/bin/docker-real run \ | ||
| --network "$NETWORK_NAME" \ | ||
| --cap-add NET_ADMIN \ | ||
| -e HTTP_PROXY="$SQUID_PROXY" \ | ||
| -e HTTPS_PROXY="$SQUID_PROXY" \ | ||
| -e http_proxy="$SQUID_PROXY" \ | ||
| -e https_proxy="$SQUID_PROXY" \ | ||
| -e SQUID_PROXY_IP="$SQUID_IP" \ | ||
| -e SQUID_PROXY_PORT="$SQUID_PORT" \ | ||
| "${PARSED_DOCKER_OPTS[@]}" \ | ||
| "$PARSED_IMAGE" |
There was a problem hiding this comment.
When no user command is specified (lines 238-252, 287-299), the container runs with its default entrypoint but NET_ADMIN capability is still added. However, NAT rules are not injected because there's no command to wrap. This means containers with default entrypoints remain vulnerable to proxy bypass. Consider either: (1) documenting this limitation clearly, (2) always wrapping even default entrypoints by inspecting the image metadata, or (3) failing with an error when no command is provided.
This issue also appears in the following locations of the same file:
- line 211
| # No user command - can't easily wrap the default entrypoint | |
| echo "[$(date -Iseconds)] WARNING: No command specified, NAT rules may not apply to default entrypoint" >> "$LOG_FILE" | |
| exec /usr/bin/docker-real run \ | |
| --network "$NETWORK_NAME" \ | |
| --cap-add NET_ADMIN \ | |
| -e HTTP_PROXY="$SQUID_PROXY" \ | |
| -e HTTPS_PROXY="$SQUID_PROXY" \ | |
| -e http_proxy="$SQUID_PROXY" \ | |
| -e https_proxy="$SQUID_PROXY" \ | |
| -e SQUID_PROXY_IP="$SQUID_IP" \ | |
| -e SQUID_PROXY_PORT="$SQUID_PORT" \ | |
| "${PARSED_DOCKER_OPTS[@]}" \ | |
| "$PARSED_IMAGE" | |
| # No user command - cannot safely enforce NAT rules on the default entrypoint | |
| echo "[$(date -Iseconds)] ERROR: No command specified for image '$PARSED_IMAGE'; cannot enforce proxy NAT rules on default entrypoint. Aborting container start." >> "$LOG_FILE" | |
| echo "docker-wrapper: security error: no command specified for image '$PARSED_IMAGE'; cannot enforce proxy NAT rules on default entrypoint." >&2 | |
| exit 1 |
| // This test verifies that child containers have NAT rules applied via docker-wrapper.sh | ||
| // Even if an application ignores HTTP_PROXY env vars, traffic is still redirected to Squid | ||
| const result = await runner.runWithSudo( | ||
| `docker run --rm alpine:latest sh -c 'apk add --no-cache curl iptables >/dev/null 2>&1 && unset HTTP_PROXY HTTPS_PROXY http_proxy https_proxy && curl -f https://example.com --max-time 10'`, |
There was a problem hiding this comment.
The test verifies NAT rules work when curl and iptables are installed in the container, but doesn't test the scenario where the command runs in a container that already has both tools installed. The test result could fail if apk add fails rather than if NAT rules aren't working. Consider using a pre-built image with curl and iptables, or adding explicit error handling for the apk command to distinguish installation failures from proxy bypass.
This issue also appears in the following locations of the same file:
- line 400
…tainers-nat-rules # Conflicts: # containers/agent/docker-wrapper.sh # tests/integration/docker-egress.test.ts
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
Smoke Test Results - Claude Last 2 merged PRs:
✅ GitHub MCP - Retrieved PR data Status: PASS
|
Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall: PASS Author: @copilot | Assignees: @Mossaka, @copilot
|
Security Review - Critical Issues FoundThis PR introduces several changes that significantly weaken the security boundaries of the AWF firewall. The changes remove multiple layers of defense-in-depth protections. 🔴 CRITICAL: CAP_NET_ADMIN No Longer DroppedFile: Before: exec capsh --drop=cap_net_admin -- -c "exec gosu awfuser $(printf '%q ' "$@")"After: exec gosu awfuser "$@"Security Impact:
Recommended Action: Restore the 🔴 CRITICAL: OUTPUT Filter Chain Rules RemovedFile: Removed Code: # OUTPUT filter chain rules (defense-in-depth with NAT rules)
iptables -A OUTPUT -o lo -j ACCEPT
iptables -A OUTPUT -p udp -d "$dns_server" --dport 53 -j ACCEPT
iptables -A OUTPUT -p tcp -d "$dns_server" --dport 53 -j ACCEPT
# ... (multiple DNS rules)
iptables -A OUTPUT -p tcp -d "$SQUID_IP" -j ACCEPT
# Drop all other TCP traffic (default deny policy)
iptables -A OUTPUT -p tcp -j DROPSecurity Impact:
Recommended Action: Restore the OUTPUT filter chain rules. Defense-in-depth is a critical security principle - removing one layer of protection makes the system more vulnerable to bypass techniques. 🟡 MODERATE: Seccomp Profile WeakenedFile: Removed: {
"names": ["ptrace", "process_vm_readv", "process_vm_writev"],
"action": "SCMP_ACT_ERRNO",
"comment": "Block process inspection/modification"
}Security Impact:
Recommended Action: Consider restoring these syscall blocks unless there's a specific, documented need for process inspection capabilities. 🟡 MODERATE: libcap2-bin Package RemovedFile: Removed: libcap2-bin && \Security Impact:
Recommended Action: Restore ℹ️ INFO: Docker Socket Access for awfuserFile: Added: usermod -aG docker awfuserSecurity Impact:
Recommendation: This change appears intentional for MCP server support. Ensure documentation clearly warns users about this privilege escalation and recommends setting SummaryThe removal of CAP_NET_ADMIN dropping and OUTPUT filter chain rules are critical security regressions that undermine the core purpose of this firewall. These protections were explicitly designed to prevent firewall bypass and should be restored before merging. Suggested Actions:
|
Child containers spawned by the agent lack NAT rules that redirect HTTP/HTTPS traffic to Squid. Applications ignoring
HTTP_PROXYenv vars could bypass the firewall entirely.Changes
docker-wrapper.sh
--cap-add NET_ADMINinto all spawned containersexec--networkis specifiedprintf %qfor robust shell metacharacter escapingparse_docker_run_args,build_escaped_cmd, etc.)NAT Setup Script
Containers without
iptablesfall back to HTTP_PROXY (previous behavior).Integration Tests
unset HTTP_PROXYwget --no-proxyblocked by NATOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.