[Security Review] Daily Security Review and Threat Modeling — 2026-03-18 #1352
Closed
Replies: 1 comment
-
|
This discussion was automatically closed because it expired on 2026-03-25T14:03:22.087Z.
|
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
A comprehensive security review of the
gh-aw-firewallcodebase was performed on 2026-03-18, covering 6,092 lines of security-critical code across the network firewall, container orchestration, and domain validation layers. The overall security posture is strong, with defence-in-depth implemented at multiple layers (host iptables → container NAT → Squid L7 proxy). Four focused findings are documented below, ranging from medium to low severity.🔍 Findings from Escape Test Workflow
The
firewall-escape-testworkflow was not found by its shorthand name in the agenticworkflows registry (workflows visible:security-review,secret-digger-*,security-guard). The dailysecurity-reviewlog retrieval was also blocked due to missinggitbinary in the runner path. This review therefore proceeds entirely on static codebase analysis.🛡️ Architecture Security Analysis
Network Security Assessment — Good
The firewall implements two independent filtering layers:
Host layer (
src/host-iptables.ts) — A dedicatedFW_WRAPPERchain is inserted at position 1 inDOCKER-USER, ensuring all container-origin traffic is filtered before Docker's own rules. Rules are well-ordered: Squid IP is explicitly allowed first, then DNS upstreams, then established/related state, then REJECT for everything else (including multicast and link-local169.254.0.0/16). IPv6 is handled viaip6tableswhen available; if not, IPv6 is disabled system-wide viasysctl.Container NAT layer (
containers/agent/setup-iptables.sh) — All port-80/443 TCP is DNAT'd to Squid. Dangerous ports (SSH:22, SMTP:25, databases 1433/3306/5432/6379/27017, RDP:3389) are explicitly RETURN'd from NAT and then DROP'd by filter chain. Final rules drop all TCP and UDP not previously allowed.Finding M-1 (Medium): ICMP not blocked in agent container OUTPUT chain
The comment explains this is required to allow
mount(2)for procfs inside the chroot (Docker's default AppArmor profiledocker-defaultblocks themountsyscall). While this is mitigated by:SYS_ADMINbeing dropped viacapshbefore user code runsno-new-privileges:truebeing setpivot_rootandkexec_load...disabling AppArmor removes a kernel-enforced security layer. If a future refactor inadvertently fails to drop
SYS_ADMINviacapsh, or ifcapshitself is exploited, there is no AppArmor backstop preventing filesystem manipulations.Recommendation: Investigate creating a custom AppArmor profile that allows only the specific
mountoperation needed for procfs (e.g.,mount fstype=proc), rather than going fullyunconfined. This would restore AppArmor protection while allowing the necessary mounts.Domain Validation Assessment — Strong
The domain validation is carefully implemented. Wildcard patterns use a character-class based regex (
[a-zA-Z0-9.-]*) instead of.*to prevent ReDoS. Over-broad patterns are rejected with meaningful errors. Squid independently blocks direct IP connections, providing defence-in-depth against domain filter bypass via numeric IPs.Input Validation Assessment — Good
All
execacalls use array-based arguments (noshell: true), preventing command injection:The
escapeShellArgfunction insrc/cli.ts:864-876correctly uses single-quote wrapping with proper'\\''escaping for single quotes within arguments. UID/GID values passed via environment are validated as numeric and non-zero inentrypoint.sh.Finding L-1 (Low):
disableIpv6ViaSysctlfailure is only a warning, not a hard errorWhen
ip6tablesis unavailable, the fallback is to disable IPv6 viasysctl. Ifsysctlfails (e.g., kernel restrictions, read-only/proc/sys), the firewall continues operating but IPv6 remains active and unfiltered. An agent could exfiltrate data or reach blocked services over IPv6.Recommendation: Treat a
sysctlfailure here as a hard error — either abort with a clear message or at minimum escalate tologger.errorand expose a--allow-ipv6-unfilteredopt-in flag.Dependency Vulnerability Assessment
Finding L-2 (Low): js-yaml prototype pollution in
markdownlint-cli2The
js-yamlvulnerability (prototype pollution via YAML<<merge key) affectsmarkdownlint-cli2, which is used in the documentation/linting pipeline. Thejs-yamllibrary is also used at runtime (src/docker-manager.tsgenerates Docker Compose YAML viajs-yaml). However,js-yamlin the main dependency tree should be separately checked:The
markdown-itReDoS only applies to documentation tooling, not runtime code.Recommendation: Run
npm ls js-yamlto confirm the runtime dependency resolves tojs-yaml@4.x. Updatemarkdownlint-cli2to resolve the moderate advisories.setup-iptables.shL319-320: only TCP/UDP droppeddocker-manager.ts:1028--enable-dindmounts real Docker socket, agent creates unfiltered containersdocker-manager.ts:788-793+ log warninghost-iptables.ts:69-78setup-iptables.shorhost-iptables.tsdocker-manager.ts:1031-1033awf-netrestrictionsdocker-manager.tslog warning on enableDinddst_ipv4/dst_ipv6ACLs block this🎯 Attack Surface Map
--allow-domainsinputsrc/cli.ts:1117,src/domain-patterns.ts--enable-dindDocker socketsrc/docker-manager.ts:788-793--enable-host-accessbypasssrc/docker-manager.ts:1062,setup-iptables.sh:175-214--allow-host-portscontainers/agent/setup-iptables.sh:319-320src/host-iptables.ts:69-78src/docker-manager.ts:1028,entrypoint.shcapsh dropcontainers/api-proxy/server.jscontainers/agent/entrypoint.sh:20-30📋 Evidence Collection
Commands run and key outputs
✅ Recommendations
🔴 Medium — Should fix soon
[M-1] Block ICMP in agent container OUTPUT chain
containers/agent/setup-iptables.sh, after line 320iptables -A OUTPUT -p icmp -j DROP[M-2] Replace
apparmor:unconfinedwith a minimal custom AppArmor profilesrc/docker-manager.ts:1028, new filecontainers/agent/apparmor-profilemount fstype=procand denies everything else the default profile would denycapshdrop fails🟡 Low — Plan to address
[L-1] Treat IPv6 sysctl failure as hard error
src/host-iptables.ts:69-78--allow-ipv6-unfilteredopt-in flag[L-2] Update dev dependencies to resolve moderate npm advisories
package.jsonnpm update markdownlint-cli2; confirm runtimejs-yamlis v4.x vianpm ls js-yamljs-yaml@3.xis transitively present in runtime pathℹ️ Informational
[I-1]
--enable-dindis an inherent firewall bypass — documented with a warning log, but should also appear prominently in user-facing help text and README. Spawned containers will not be subject toawf-netACLs.[I-2] Agent-level iptables LOG for ICMP — even if ICMP is eventually blocked, adding a LOG rule before the DROP (
iptables -A OUTPUT -p icmp -j LOG --log-prefix "[FW_BLOCKED_ICMP] ") improves forensic visibility, consistent with how UDP and TCP are already logged.[I-3] sysctl IPv6 disable also applies in container-level
setup-iptables.sh— the same silent-failure risk exists there (sysctl ... || echo "[iptables] WARNING:..."rather thanexit 1). Consider failing hard when the IPv6 disable is required but fails.📈 Security Metrics
Beta Was this translation helpful? Give feedback.
All reactions