mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
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).
This commit is contained in:
1 parent
4c485d6fa1
commit
484d434f6c
7 files changed
+76
-24
No files matched your search
@@ -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}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
+39
-7
@@ -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,22 +158,31 @@ 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) {
|
||||
for _, field := range strings.Fields(trimmed) {
|
||||
if strings.HasPrefix(field, ex) {
|
||||
exempt = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if exempt {
|
||||
break
|
||||
}
|
||||
}
|
||||
if exempt {
|
||||
continue
|
||||
}
|
||||
|
||||
// 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 {
|
||||
|
||||
@@ -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": {
|
||||
|
||||
Reference in new issue
Block a user