[Security Review] Daily Security Review and Threat Modeling – 2026-03-15 #1318
Closed
Replies: 1 comment
-
|
This discussion was automatically closed because it expired on 2026-03-22T13:46:45.976Z.
|
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
📊 Executive Summary
This report presents a comprehensive security review of the gh-aw-firewall (AWF) codebase conducted on 2026-03-15. The review covered network security architecture, container hardening, domain pattern validation, input handling, environment variable management, and dependency security.
Overall posture: Strongly designed with multiple defence-in-depth layers. The architecture demonstrates intentional security engineering. Three findings warrant attention: one High (overly-broad wildcard domain patterns), one High (broad
--env-allpassthrough), and one Medium (AppArmor unconfined + mount syscall enabled). The remaining findings are informational.🔍 Findings from Firewall Escape Test
The "Firewall Escape Test" workflow was not found in the configured workflow list. The complementary escape-test workflow appears not to exist in this repository. The security review below is therefore based solely on static analysis of the codebase.
🛡️ Architecture Security Analysis
Network Security Assessment
Evidence gathered:
# Command run: cat containers/agent/setup-iptables.sh cat src/host-iptables.tsTwo-layer iptables architecture:
Layer 1 — Host (DOCKER-USER chain,
src/host-iptables.ts):FW_WRAPPERchain inserted intoDOCKER-USERLayer 2 — Container NAT (
containers/agent/setup-iptables.sh):OUTPUTchain DNAT rules redirect TCP 80/443 to SquidRETURNthen falls to DROPRETURNiptables -A OUTPUT -p tcp -j DROP; iptables -A OUTPUT -p udp -j DROPAssessment: Firewall rule ordering is correct (specific allows before general deny). No obvious NAT bypass opportunities. IPv4 and IPv6 both addressed. UDP is blocked by default (prevents non-DNS exfiltration). DNS exfiltration is well-mitigated.
Squid Configuration (
src/squid-config.ts):acl dst_ipv4 dstdom_regex ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$Safe_portsACL levelhttp_access deny allas final ruleContainer Security Assessment
Evidence gathered:
AppArmor is disabled to allow
mount -t procinentrypoint.sh. The stated justification is that SYS_ADMIN is dropped before user code runs. This is architecturally sound but removes an entire mandatory access control layer. AppArmor could restrict file paths, mount operations, and syscalls even for root. Alternatives: create a custom AppArmor profile that allows onlyprocmounting and restricts everything else.Finding — Medium:
mountandunshareallowed in seccomp without argument filteringThe
mount,unshare, andclonesyscalls are allowed without argument-level filtering. While SYS_ADMIN and SYS_CHROOT are dropped before user code runs, the seccomp policy does not enforce this. If capability dropping fails or is bypassed (e.g., a race condition),unshare+clonewithCLONE_NEWNS+mountcould be used to create a new mount namespace and remount filesystems. Argument-level seccomp filtering (SCMP_CMP_MASKED_EQwithCLONE_NEWNSbit) would add defence-in-depth.Domain Validation Assessment
Evidence gathered:
Finding — High: Single-segment TLD wildcards (
*.com,*.io) are allowedThe validation logic in
src/domain-patterns.ts:185-196:This pattern matches every
.comdomain, effectively disabling the firewall for any user who passes--allow-domains '*.com'. An attacker who can control the domain allowlist (e.g., via a compromised workflow) could trivially bypass all domain filtering.Recommendation: Validate that wildcard patterns have a minimum number of non-wildcard labels after the last wildcard. At minimum, patterns like
*.comand*.io(wildcard as the most-specific label) should require at least 2 non-wildcard labels:*.github.com(1 wildcard, 2 non-wildcard) is fine;*.com(1 wildcard, 1 non-wildcard) should be blocked.Positive findings in domain validation:
*alone blocked*.*blocked*.*.comblocked[a-zA-Z0-9.-]*(not.*)(redacted)https://`) correctly parsedInput Validation Assessment
Evidence gathered:
Positive findings:
escapeShellArg()insrc/cli.ts:860properly wraps arguments in single quotes with internal single-quote escapingjoinShellArgs(), safely escaping each argdocker execas a JSON array (['/bin/bash', '-c', cmd]), not a shell stringconfig.agentCommand.replace(/\$/g, '$$$$')prevents YAML variable expansion in Docker Composeport.replace(/[^0-9-]/g, '')strips non-numeric chars as defence-in-depthFinding — High:
--env-allpasses arbitrary host environment variablesEXCLUDED_ENV_VARSonly contains:PATH,PWD,OLDPWD,SHLVL,_,SUDO_COMMAND,SUDO_USER,SUDO_UID,SUDO_GID.With
--env-all, variables such asAWS_SECRET_ACCESS_KEY,AZURE_CLIENT_SECRET,DATABASE_URL,POSTGRES_PASSWORD, any.npmrcauth tokens in the environment, CI provider tokens, etc. are forwarded to the container. TheAWF_ONE_SHOT_TOKENSprotection only covers a hardcoded list:COPILOT_GITHUB_TOKEN, GITHUB_TOKEN, GH_TOKEN, GITHUB_API_TOKEN, GITHUB_PAT, GH_ACCESS_TOKEN, OPENAI_API_KEY, OPENAI_KEY, ANTHROPIC_API_KEY, CLAUDE_API_KEY, CODEX_API_KEY.Arbitrary secrets not in this list (e.g.,
AWS_SECRET_ACCESS_KEY,AZURE_CLIENT_SECRET) remain readable via/proc/self/environfor the entire process lifetime when--env-allis used.Recommendation: Add common cloud credential env vars to
EXCLUDED_ENV_VARSwhen--env-allis used. Alternatively, implement an opt-in pattern (--env KEY=VALUE) or warn users clearly when--env-allis used that it forwards all secrets.Finding — Low:
~/.configmounted read-write broadlyThe entire
~/.configis mounted read-write. Credential files inside are hidden via/dev/nulloverlays, but only for explicitly known paths (e.g.,~/.config/gh/hosts.yml). New credential files added to~/.configin the future (or unknown tools' credential files) would be exposed. The comment acknowledges this: "Specific credential files within ~/.config (like ~/.config/gh/hosts.yml) are still blocked via /dev/null overlays applied later in the code."*.com) bypasses firewallsrc/domain-patterns.ts:185-196--env-allexposes cloud/DB credentials in containersrc/docker-manager.ts:491-495src/docker-manager.ts:1028, seccomp-profile.json~/.configRW mount exposes unknown credential filessrc/docker-manager.ts:680npm auditoutput/proc/1/environreadableentrypoint.sh:680-690--allow-domainssrc/domain-patterns.tsvalidationsquid-config.ts:549-554dst_ipv4/dst_ipv6 ACLs🎯 Attack Surface Map
--allow-domainssrc/cli.ts:739validateDomainOrPattern(), ReDoS-safe regex--env-allsrc/docker-manager.ts:491EXCLUDED_ENV_VARSset, one-shot-tokensrc/cli.ts:1347escapeShellArg(), JSON array execsrc/ssl-bump.ts:337parseUrlPatterns()escapingsrc/docker-manager.ts:1049$$dollar-sign escapingsrc/docker-manager.ts:592+/tmpmount (rw)src/docker-manager.ts:603/tmpwritablecontainers/agent/setup-iptables.shsrc/squid-config.ts:248containers/agent/one-shot-token/one-shot-token.c📋 Evidence Collection
Domain validation testing output
Seccomp profile analysis
npm audit output
EXCLUDED_ENV_VARS set (--env-all)
AppArmor and capability settings
✅ Recommendations
🔴 High Priority
H1: Block single-segment TLD wildcard patterns (
*.com,*.io)File:
src/domain-patterns.ts:185Require that wildcard patterns have at least 2 non-wildcard segments following the wildcard. This would allow
*.github.com(2 non-wildcard segments after wildcard) but block*.com(only 1).H2: Expand
EXCLUDED_ENV_VARSfor--env-allmode or add a warningFile:
src/docker-manager.ts:375When
--env-allis used, add common cloud/CI credential variables to the exclusion set, or emit a prominent warning listing the classes of variables that will be forwarded. Consider adding:AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN,AZURE_CLIENT_SECRET,GOOGLE_APPLICATION_CREDENTIALS,DATABASE_URL,*_PASSWORD,*_SECRET(using pattern matching).🟡 Medium Priority
M1: Add AppArmor profile instead of
unconfinedFile:
src/docker-manager.ts:1028Create a minimal custom AppArmor profile that permits only the required
procmount operation and denies all other mounts. This restores the AppArmor defence layer while preserving the procfs mounting capability.M2: Add argument-level seccomp filtering for
unshareandcloneFile:
containers/agent/seccomp-profile.jsonAdd a
SCMP_CMP_MASKED_EQargument filter to blockCLONE_NEWNS(0x20000),CLONE_NEWNET(0x40000000),CLONE_NEWUSER(0x10000000) flags inclone/clone3, and restrictunshareto only allow flags needed for development tools. This provides defence-in-depth if capability dropping is bypassed.M3: Scope
~/.configmount more narrowlyFile:
src/docker-manager.ts:680Instead of mounting all of
~/.config, mount only explicitly required subdirectories (e.g.,~/.config/ghis already overlaid with/dev/null). This reduces the blast radius for unknown credential files.M4: Upgrade or patch js-yaml (prototype pollution)
File:
package.jsonThe moderate js-yaml vulnerability (prototype pollution via YAML merge
<<) affects the Docker Compose YAML generation path. While exploitation requires attacker-controlled YAML input (unlikely in this context), upgrading to a patched version eliminates the risk entirely.🟢 Low Priority
L1: Add explicit warning for
--env-allin documentation and help textThe current help text should warn that
--env-allforwards ALL environment variables including arbitrary secrets, and recommend using specific--env KEY=VALUEflags for individual variables instead.L2: Consider extending
AWF_ONE_SHOT_TOKENSlistFile:
src/docker-manager.ts:421Add commonly used cloud credential variable names (
AWS_SECRET_ACCESS_KEY,AZURE_CLIENT_SECRET) to the default one-shot-token protection list, even when--env-allis not used. This adds defence-in-depth for deliberate secrets forwarded via--env.📈 Security Metrics
Review conducted via static analysis of source code, shell scripts, seccomp profile, and npm audit output. No dynamic testing was performed. All evidence is verifiable by re-running the commands cited in the Evidence Collection section.
Beta Was this translation helpful? Give feedback.
All reactions