From 484d434f6c25950020e539494d1cfade95018b8d Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 2 Apr 2026 18:58:24 +0700 Subject: [PATCH] fix(security): harden sandbox, auth, and shell deny patterns Sandbox: add noexec/nosuid/nodev to tmpfs mounts, remove SETUID/SETGID/CHOWN caps, add PidsLimit 256 default, keep no-new-privileges from base. Auth: reject X-GoClaw-User-Id header spoofing in dev mode (no gateway token), use full 32-byte HMAC for file tokens instead of truncated 16-byte. Shell: add NFKC Unicode normalization + zero-width character stripping before deny pattern matching, add 5 export-prefixed env var deny patterns, fix exemption logic to check per-argument prefix instead of whole-command substring (prevents bypass via comments while preserving skill store access). --- docker-compose.sandbox.yml | 21 ++++++------- internal/http/auth.go | 9 ++++++ internal/http/file_token.go | 4 +-- internal/sandbox/docker.go | 13 ++++++-- internal/sandbox/sandbox.go | 1 + internal/tools/shell.go | 48 ++++++++++++++++++++++++----- internal/tools/shell_deny_groups.go | 6 ++++ 7 files changed, 77 insertions(+), 25 deletions(-) diff --git a/docker-compose.sandbox.yml b/docker-compose.sandbox.yml index 90a17153..f550a5cf 100644 --- a/docker-compose.sandbox.yml +++ b/docker-compose.sandbox.yml @@ -2,13 +2,14 @@ # # Prerequisites: # 1. Build the sandbox image: docker build -t goclaw-sandbox:bookworm-slim -f Dockerfile.sandbox . -# 2. Ensure Docker socket is accessible +# 2. Ensure Docker socket is accessible (required for container orchestration) # # Usage: -# docker compose -f docker-compose.yml -f docker-compose.postgres.yml -f docker-compose.sandbox.yml up +# docker compose -f docker-compose.yml -f docker-compose.sandbox.yml up # -# SECURITY NOTE: Mounting Docker socket gives the container control over host Docker. -# Only use in trusted environments where agent code execution isolation is required. +# SECURITY WARNING: This overlay mounts the Docker socket, giving the container +# control over host Docker. Deploy only behind a trusted network boundary. +# For maximum isolation, consider using Docker-in-Docker (dind) or Sysbox instead. services: goclaw: @@ -26,16 +27,12 @@ services: - GOCLAW_SANDBOX_CPUS=1.0 - GOCLAW_SANDBOX_TIMEOUT_SEC=300 - GOCLAW_SANDBOX_NETWORK=false - # Override base cap_drop to allow Docker socket access. - # Re-include base caps (SETUID/SETGID/CHOWN) lost when overriding cap_add. - # WARNING: SETUID/SETGID with security_opt cleared (no no-new-privileges) - # increases attack surface. Only use in trusted environments. + # Docker socket requires overriding base cap_drop to allow container orchestration. + # Keep security_opt (no-new-privileges) from base — do NOT clear it. + # Only add the minimum cap needed: NET_BIND_SERVICE for port binding. + # SETUID/SETGID/CHOWN removed to prevent privilege escalation. cap_drop: [] cap_add: - NET_BIND_SERVICE - - SETUID - - SETGID - - CHOWN - security_opt: [] group_add: - ${DOCKER_GID:-999} diff --git a/internal/http/auth.go b/internal/http/auth.go index 45ce10de..d6edcd5c 100644 --- a/internal/http/auth.go +++ b/internal/http/auth.go @@ -316,6 +316,15 @@ func enrichContext(ctx context.Context, r *http.Request, auth authResult) contex ctx = store.WithLocale(ctx, extractLocale(r)) ctx = store.WithRole(ctx, string(auth.Role)) userID := extractUserID(r) + // Security: In dev mode (no gateway token configured), do not trust the + // X-GoClaw-User-Id header — force "system" to prevent identity spoofing. + if pkgGatewayToken == "" && auth.KeyData == nil && userID != "" { + slog.Warn("security.user_id_header_ignored_no_auth", + "attempted_user_id", userID, + "ip", r.RemoteAddr, + ) + userID = "system" + } // If the API key has a bound owner, force user_id to owner regardless of header. if auth.KeyData != nil && auth.KeyData.OwnerID != "" { if userID != "" && userID != auth.KeyData.OwnerID { diff --git a/internal/http/file_token.go b/internal/http/file_token.go index 29c7e8ec..1227dee0 100644 --- a/internal/http/file_token.go +++ b/internal/http/file_token.go @@ -35,7 +35,7 @@ func FileSigningKey() string { } // SignFileToken creates a short-lived HMAC token for file access. -// Token format: {base64url_hmac_16bytes}.{unix_expiry} (~40 chars). +// Token format: {base64url_hmac_32bytes}.{unix_expiry}. // The path is bound into the signature so tokens can't be reused for other files. func SignFileToken(path, secret string, ttl time.Duration) string { expiry := time.Now().Add(ttl).Unix() @@ -62,7 +62,7 @@ func VerifyFileToken(token, path, secret string) bool { func fileTokenHMAC(path, secret string, expiry int64) string { mac := hmac.New(sha256.New, []byte(secret)) mac.Write(fmt.Appendf(nil, "%s:%d", path, expiry)) - return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)[:16]) + return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) } // SignMediaPath converts a media ref path to a signed /v1/files/ URL. diff --git a/internal/sandbox/docker.go b/internal/sandbox/docker.go index bb9e3bae..4347053d 100644 --- a/internal/sandbox/docker.go +++ b/internal/sandbox/docker.go @@ -49,9 +49,16 @@ func newDockerSandbox(ctx context.Context, name string, cfg Config, workspace st args = append(args, "--read-only") } for _, t := range cfg.Tmpfs { - // Append default size if not already specified and TmpfsSizeMB > 0 - if cfg.TmpfsSizeMB > 0 && !strings.Contains(t, ":") { - t = fmt.Sprintf("%s:size=%dm", t, cfg.TmpfsSizeMB) + if !strings.Contains(t, ":") { + // Always add security flags; optionally add size limit + opts := "noexec,nosuid,nodev" + if cfg.TmpfsSizeMB > 0 { + opts = fmt.Sprintf("size=%dm,%s", cfg.TmpfsSizeMB, opts) + } + t = fmt.Sprintf("%s:%s", t, opts) + } else if !strings.Contains(t, "noexec") { + // User-specified options but missing noexec — append security flags + t += ",noexec,nosuid,nodev" } args = append(args, "--tmpfs", t) } diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index c286260f..523f5f51 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -96,6 +96,7 @@ func DefaultConfig() Config { ReadOnlyRoot: true, CapDrop: []string{"ALL"}, Tmpfs: []string{"/tmp", "/var/tmp", "/run"}, + PidsLimit: 256, MaxOutputBytes: 1 << 20, // 1MB ContainerPrefix: "goclaw-sbx-", Workdir: "/workspace", diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 9b0cb9ae..a843a23b 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -13,6 +13,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/sandbox" "github.com/nextlevelbuilder/goclaw/internal/store" + "golang.org/x/text/unicode/norm" ) // Dangerous command patterns organized into configurable deny groups. @@ -72,9 +73,27 @@ func (t *ExecTool) DenyPaths(paths ...string) { } } -// AllowPathExemptions adds substrings that exempt a command from deny pattern matches. -func (t *ExecTool) AllowPathExemptions(substrings ...string) { - t.denyExemptions = append(t.denyExemptions, substrings...) +// AllowPathExemptions adds path prefixes that exempt a command from deny pattern matches. +// Each shell argument is checked individually — commands like "cat .goclaw/skills-store/tool.py" +// are exempt because the argument ".goclaw/skills-store/tool.py" starts with the prefix. +func (t *ExecTool) AllowPathExemptions(prefixes ...string) { + t.denyExemptions = append(t.denyExemptions, prefixes...) +} + +// normalizeCommand applies NFKC Unicode normalization and strips zero-width +// characters before deny pattern matching, preventing Unicode-based bypasses. +func normalizeCommand(s string) string { + // NFKC normalization: folds compatibility characters (e.g. fullwidth letters) + s = norm.NFKC.String(s) + // Strip zero-width characters that are invisible but can fragment tokens + s = strings.NewReplacer( + "\u200b", "", // zero-width space + "\u200c", "", // zero-width non-joiner + "\u200d", "", // zero-width joiner + "\u2060", "", // word joiner + "\ufeff", "", // BOM / zero-width no-break space + ).Replace(s) + return s } // SetApprovalManager sets the exec approval manager for this tool. @@ -118,6 +137,10 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { return ErrorResult("command contains invalid NUL byte") } + // Normalize command before all deny checks: NFKC + zero-width strip prevents + // Unicode-based pattern bypass while preserving functional command content. + normalizedCommand := normalizeCommand(command) + // Resolve deny patterns: per-agent overrides from context, fallback to all defaults. denyOverrides := store.ShellDenyGroupsFromContext(ctx) groupPatterns := ResolveDenyPatterns(denyOverrides) @@ -135,12 +158,21 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { // Check for dangerous commands (applies to both host and sandbox). for _, pattern := range allPatterns { - if pattern.MatchString(command) { - // Check if any exemption applies (e.g. skills-store within .goclaw) + if pattern.MatchString(normalizedCommand) { + // Check if any exemption applies (e.g. skills-store within .goclaw). + // Uses argument-level prefix matching to prevent bypass via comments + // (e.g. "echo pwned # .goclaw/skills-store/") while still allowing + // commands like "cat .goclaw/skills-store/tool.py". exempt := false + trimmed := strings.TrimSpace(normalizedCommand) for _, ex := range t.denyExemptions { - if strings.Contains(command, ex) { - exempt = true + for _, field := range strings.Fields(trimmed) { + if strings.HasPrefix(field, ex) { + exempt = true + break + } + } + if exempt { break } } @@ -150,7 +182,7 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { // Package install commands: route through approval flow instead of hard deny. // This lets agents "request permission" from admin to install packages. - if t.approvalMgr != nil && matchesAny(command, pkgInstallPatterns) { + if t.approvalMgr != nil && matchesAny(normalizedCommand, pkgInstallPatterns) { slog.Info("exec: package install requires approval", "command", truncateCmd(command, 100), "agent", t.agentID) decision, err := t.approvalMgr.RequestApproval(command, t.agentID, 2*time.Minute) if err != nil { diff --git a/internal/tools/shell_deny_groups.go b/internal/tools/shell_deny_groups.go index 56fe4282..d2adce35 100644 --- a/internal/tools/shell_deny_groups.go +++ b/internal/tools/shell_deny_groups.go @@ -118,6 +118,12 @@ var DenyGroupRegistry = map[string]*DenyGroup{ regexp.MustCompile(`\bGIT_DIFF_OPTS\s*=`), regexp.MustCompile(`\bBASH_ENV\s*=`), regexp.MustCompile(`\bENV\s*=.*\bsh\b`), + // export-prefixed variants: prevent setting dangerous env vars via export + regexp.MustCompile(`\bexport\s+LD_`), + regexp.MustCompile(`\bexport\s+DYLD_`), + regexp.MustCompile(`\bexport\s+BASH_ENV\b`), + regexp.MustCompile(`\bexport\s+ENV\s*=`), + regexp.MustCompile(`\bexport\s+PROMPT_COMMAND\b`), }, }, "container_escape": {