mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-16 10:15:12 +00:00
feat(security): apply upstream TS OpenClaw security and core engine fixes
Port 8 fixes from upstream TypeScript OpenClaw (4 CRITICAL + 4 HIGH): CRITICAL: 1. Tool call name trimming — add strings.TrimSpace() to all provider response parsers (Anthropic 2 locations, OpenAI 3 locations) to prevent registry lookup failures from LLM whitespace-padded names 2. Shell env injection deny patterns — block GIT_EXTERNAL_DIFF, GIT_DIFF_OPTS, BASH_ENV, and ENV=.*sh to prevent code execution via environment variable injection during git/shell operations 3. Broken symlink escape — recursive target resolution via resolveThroughExistingAncestors() to catch chained symlinks that escape workspace (e.g. link1→link2→/etc/passwd) 4. Mutable parent-symlink TOCTOU check — hasMutableSymlinkParent() detects symlinks in writable parent dirs that could be rebound between path validation and file operation HIGH: 5. Model fallback thinking preservation — add ThinkingCapable interface to providers/types.go, implement on Anthropic/OpenAI/DashScope, check before injecting thinking_level in agent loop, warn on fallback when thinking is configured 6. Cron session-key double-prefix guard — prevent agent:X:cron:agent:X duplication in BuildCronSessionKey() 7. Webhook rate limiter — bounded WebhookRateLimiter (4096 keys max, 60s window, 30 hits/window) to prevent memory exhaustion DoS 8. DM policy allowlist validation — warn at startup when dmPolicy=allowlist with empty allowFrom (silent message drop) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
1 parent
d3323352a1
commit
73b46c3634
17 files changed
+213
-16
No files matched your search
@@ -655,7 +655,7 @@ func makeCronJobHandler(agents *agent.Router, msgBus *bus.MessageBus, cfg *confi
|
||||
return nil, fmt.Errorf("agent %s not found: %w", agentID, err)
|
||||
}
|
||||
|
||||
sessionKey := sessions.BuildCronSessionKey(agentID, job.ID, fmt.Sprintf("cron-%s", job.ID))
|
||||
sessionKey := sessions.BuildCronSessionKey(agentID, job.ID, job.ID)
|
||||
channel := job.Payload.Channel
|
||||
if channel == "" {
|
||||
channel = "cron"
|
||||
@@ -667,7 +667,7 @@ func makeCronJobHandler(agents *agent.Router, msgBus *bus.MessageBus, cfg *confi
|
||||
Channel: channel,
|
||||
ChatID: job.Payload.To,
|
||||
UserID: job.UserID,
|
||||
RunID: fmt.Sprintf("cron-%s", job.ID),
|
||||
RunID: fmt.Sprintf("cron:%s", job.ID),
|
||||
Stream: false,
|
||||
TraceName: fmt.Sprintf("Cron [%s] - %s", job.Name, agentID),
|
||||
TraceTags: []string{"cron"},
|
||||
|
||||
@@ -552,7 +552,12 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error)
|
||||
},
|
||||
}
|
||||
if l.thinkingLevel != "" && l.thinkingLevel != "off" {
|
||||
chatReq.Options[providers.OptThinkingLevel] = l.thinkingLevel
|
||||
if tc, ok := l.provider.(providers.ThinkingCapable); ok && tc.SupportsThinking() {
|
||||
chatReq.Options[providers.OptThinkingLevel] = l.thinkingLevel
|
||||
} else {
|
||||
slog.Debug("thinking_level ignored: provider does not support thinking",
|
||||
"provider", l.provider.Name(), "level", l.thinkingLevel)
|
||||
}
|
||||
}
|
||||
|
||||
// Call LLM (streaming or non-streaming)
|
||||
|
||||
@@ -90,6 +90,11 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc {
|
||||
provider, _ = deps.ProviderReg.Get(names[0])
|
||||
slog.Warn("agent provider not found, using fallback",
|
||||
"agent", agentKey, "wanted", ag.Provider, "using", names[0])
|
||||
if tl := ag.ParseThinkingLevel(); tl != "" && tl != "off" {
|
||||
slog.Warn("agent thinking may not be supported by fallback provider",
|
||||
"agent", agentKey, "thinking_level", tl,
|
||||
"wanted_provider", ag.Provider, "fallback_provider", names[0])
|
||||
}
|
||||
}
|
||||
|
||||
if provider == nil {
|
||||
|
||||
@@ -10,6 +10,7 @@ package channels
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"strings"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
||||
@@ -203,6 +204,19 @@ func (c *BaseChannel) CheckPolicy(peerKind, dmPolicy, groupPolicy, senderID stri
|
||||
}
|
||||
}
|
||||
|
||||
// ValidatePolicy logs warnings for common policy misconfigurations.
|
||||
// Should be called during channel initialization.
|
||||
func (c *BaseChannel) ValidatePolicy(dmPolicy, groupPolicy string) {
|
||||
if dmPolicy == "allowlist" && !c.HasAllowList() {
|
||||
slog.Warn("channel policy misconfiguration: dmPolicy=allowlist but allowFrom is empty — all DMs will be rejected",
|
||||
"channel", c.name)
|
||||
}
|
||||
if groupPolicy == "allowlist" && !c.HasAllowList() {
|
||||
slog.Warn("channel policy misconfiguration: groupPolicy=allowlist but allowFrom is empty — all group messages will be rejected",
|
||||
"channel", c.name)
|
||||
}
|
||||
}
|
||||
|
||||
// HandleMessage creates an InboundMessage and publishes it to the bus.
|
||||
// This is the standard way for channels to forward received messages.
|
||||
// peerKind should be "direct" or "group" (see sessions.PeerDirect, sessions.PeerGroup).
|
||||
|
||||
@@ -47,6 +47,7 @@ func New(cfg config.DiscordConfig, msgBus *bus.MessageBus, pairingSvc store.Pair
|
||||
discordgo.IntentsMessageContent
|
||||
|
||||
base := channels.NewBaseChannel("discord", msgBus, cfg.AllowFrom)
|
||||
base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy)
|
||||
|
||||
requireMention := true
|
||||
if cfg.RequireMention != nil {
|
||||
|
||||
@@ -63,6 +63,7 @@ func New(cfg config.FeishuConfig, msgBus *bus.MessageBus, pairingSvc store.Pairi
|
||||
client := NewLarkClient(cfg.AppID, cfg.AppSecret, domain)
|
||||
|
||||
base := channels.NewBaseChannel("feishu", msgBus, cfg.AllowFrom)
|
||||
base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy)
|
||||
|
||||
historyLimit := cfg.HistoryLimit
|
||||
if historyLimit == 0 {
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
package channels
|
||||
|
||||
import (
|
||||
"sync"
|
||||
"time"
|
||||
)
|
||||
|
||||
const (
|
||||
// maxTrackedKeys caps the number of tracked rate-limit keys to prevent
|
||||
// memory exhaustion from attackers rotating source IPs/keys.
|
||||
maxTrackedKeys = 4096
|
||||
|
||||
// rateLimitWindow is the sliding window duration for rate counting.
|
||||
rateLimitWindow = 60 * time.Second
|
||||
|
||||
// rateLimitMaxHits is the max requests per key within a window.
|
||||
rateLimitMaxHits = 30
|
||||
)
|
||||
|
||||
type rateLimitEntry struct {
|
||||
windowStart time.Time
|
||||
count int
|
||||
}
|
||||
|
||||
// WebhookRateLimiter bounds the number of tracked rate-limit keys
|
||||
// to prevent memory exhaustion from rotating source keys (DoS).
|
||||
// Safe for concurrent use.
|
||||
type WebhookRateLimiter struct {
|
||||
mu sync.Mutex
|
||||
entries map[string]*rateLimitEntry
|
||||
}
|
||||
|
||||
// NewWebhookRateLimiter creates a bounded webhook rate limiter.
|
||||
func NewWebhookRateLimiter() *WebhookRateLimiter {
|
||||
return &WebhookRateLimiter{entries: make(map[string]*rateLimitEntry)}
|
||||
}
|
||||
|
||||
// Allow returns true if the key is within rate limits.
|
||||
// Automatically prunes stale entries and enforces a hard cap on tracked keys.
|
||||
func (r *WebhookRateLimiter) Allow(key string) bool {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
now := time.Now()
|
||||
|
||||
// Prune stale entries when approaching the cap
|
||||
if len(r.entries) >= maxTrackedKeys {
|
||||
for k, e := range r.entries {
|
||||
if now.Sub(e.windowStart) >= rateLimitWindow {
|
||||
delete(r.entries, k)
|
||||
}
|
||||
}
|
||||
// Hard eviction if still at cap (FIFO-ish via map iteration)
|
||||
for len(r.entries) >= maxTrackedKeys {
|
||||
for k := range r.entries {
|
||||
delete(r.entries, k)
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
e, ok := r.entries[key]
|
||||
if !ok || now.Sub(e.windowStart) >= rateLimitWindow {
|
||||
r.entries[key] = &rateLimitEntry{windowStart: now, count: 1}
|
||||
return true
|
||||
}
|
||||
|
||||
e.count++
|
||||
return e.count <= rateLimitMaxHits
|
||||
}
|
||||
@@ -76,6 +76,7 @@ func New(cfg config.TelegramConfig, msgBus *bus.MessageBus, pairingSvc store.Pai
|
||||
}
|
||||
|
||||
base := channels.NewBaseChannel("telegram", msgBus, cfg.AllowFrom)
|
||||
base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy)
|
||||
|
||||
requireMention := true
|
||||
if cfg.RequireMention != nil {
|
||||
|
||||
@@ -41,6 +41,7 @@ func New(cfg config.WhatsAppConfig, msgBus *bus.MessageBus, pairingSvc store.Pai
|
||||
}
|
||||
|
||||
base := channels.NewBaseChannel("whatsapp", msgBus, cfg.AllowFrom)
|
||||
base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy)
|
||||
|
||||
return &Channel{
|
||||
BaseChannel: base,
|
||||
|
||||
@@ -51,6 +51,7 @@ func New(cfg config.ZaloConfig, msgBus *bus.MessageBus, pairingSvc store.Pairing
|
||||
}
|
||||
|
||||
base := channels.NewBaseChannel("zalo", msgBus, cfg.AllowFrom)
|
||||
base.ValidatePolicy(cfg.DMPolicy, "")
|
||||
|
||||
dmPolicy := cfg.DMPolicy
|
||||
if dmPolicy == "" {
|
||||
|
||||
@@ -56,8 +56,9 @@ func WithAnthropicBaseURL(baseURL string) AnthropicOption {
|
||||
}
|
||||
}
|
||||
|
||||
func (p *AnthropicProvider) Name() string { return "anthropic" }
|
||||
func (p *AnthropicProvider) DefaultModel() string { return p.defaultModel }
|
||||
func (p *AnthropicProvider) Name() string { return "anthropic" }
|
||||
func (p *AnthropicProvider) DefaultModel() string { return p.defaultModel }
|
||||
func (p *AnthropicProvider) SupportsThinking() bool { return true }
|
||||
|
||||
func (p *AnthropicProvider) Chat(ctx context.Context, req ChatRequest) (*ChatResponse, error) {
|
||||
model := req.Model
|
||||
@@ -150,7 +151,7 @@ func (p *AnthropicProvider) ChatStream(ctx context.Context, req ChatRequest, onC
|
||||
if ev.ContentBlock.Type == "tool_use" {
|
||||
result.ToolCalls = append(result.ToolCalls, ToolCall{
|
||||
ID: ev.ContentBlock.ID,
|
||||
Name: ev.ContentBlock.Name,
|
||||
Name: strings.TrimSpace(ev.ContentBlock.Name),
|
||||
Arguments: make(map[string]interface{}),
|
||||
})
|
||||
}
|
||||
@@ -529,7 +530,7 @@ func (p *AnthropicProvider) parseResponse(resp *anthropicResponse) *ChatResponse
|
||||
_ = json.Unmarshal(block.Input, &args)
|
||||
result.ToolCalls = append(result.ToolCalls, ToolCall{
|
||||
ID: block.ID,
|
||||
Name: block.Name,
|
||||
Name: strings.TrimSpace(block.Name),
|
||||
Arguments: args,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -29,7 +29,8 @@ func NewDashScopeProvider(apiKey, apiBase, defaultModel string) *DashScopeProvid
|
||||
}
|
||||
}
|
||||
|
||||
func (p *DashScopeProvider) Name() string { return "dashscope" }
|
||||
func (p *DashScopeProvider) Name() string { return "dashscope" }
|
||||
func (p *DashScopeProvider) SupportsThinking() bool { return true }
|
||||
|
||||
// ChatStream handles DashScope's limitation: tools + streaming cannot coexist.
|
||||
// When tools are present, falls back to non-streaming Chat() and synthesizes
|
||||
|
||||
@@ -47,8 +47,9 @@ func (p *OpenAIProvider) WithChatPath(path string) *OpenAIProvider {
|
||||
return p
|
||||
}
|
||||
|
||||
func (p *OpenAIProvider) Name() string { return p.name }
|
||||
func (p *OpenAIProvider) DefaultModel() string { return p.defaultModel }
|
||||
func (p *OpenAIProvider) Name() string { return p.name }
|
||||
func (p *OpenAIProvider) DefaultModel() string { return p.defaultModel }
|
||||
func (p *OpenAIProvider) SupportsThinking() bool { return true }
|
||||
func (p *OpenAIProvider) APIKey() string { return p.apiKey }
|
||||
func (p *OpenAIProvider) APIBase() string { return p.apiBase }
|
||||
|
||||
@@ -140,12 +141,12 @@ func (p *OpenAIProvider) ChatStream(ctx context.Context, req ChatRequest, onChun
|
||||
acc, ok := accumulators[tc.Index]
|
||||
if !ok {
|
||||
acc = &toolCallAccumulator{
|
||||
ToolCall: ToolCall{ID: tc.ID, Name: tc.Function.Name},
|
||||
ToolCall: ToolCall{ID: tc.ID, Name: strings.TrimSpace(tc.Function.Name)},
|
||||
}
|
||||
accumulators[tc.Index] = acc
|
||||
}
|
||||
if tc.Function.Name != "" {
|
||||
acc.Name = tc.Function.Name
|
||||
acc.Name = strings.TrimSpace(tc.Function.Name)
|
||||
}
|
||||
acc.rawArgs += tc.Function.Arguments
|
||||
if tc.Function.ThoughtSignature != "" {
|
||||
@@ -356,7 +357,7 @@ func (p *OpenAIProvider) parseResponse(resp *openAIResponse) *ChatResponse {
|
||||
_ = json.Unmarshal([]byte(tc.Function.Arguments), &args)
|
||||
call := ToolCall{
|
||||
ID: tc.ID,
|
||||
Name: tc.Function.Name,
|
||||
Name: strings.TrimSpace(tc.Function.Name),
|
||||
Arguments: args,
|
||||
}
|
||||
if tc.Function.ThoughtSignature != "" {
|
||||
|
||||
@@ -32,6 +32,12 @@ type Provider interface {
|
||||
Name() string
|
||||
}
|
||||
|
||||
// ThinkingCapable is optionally implemented by providers that support extended thinking.
|
||||
// Used to gate thinking_level injection so it's not sent to providers that ignore it.
|
||||
type ThinkingCapable interface {
|
||||
SupportsThinking() bool
|
||||
}
|
||||
|
||||
// ChatRequest contains the input for a Chat/ChatStream call.
|
||||
type ChatRequest struct {
|
||||
Messages []Message `json:"messages"`
|
||||
|
||||
@@ -60,7 +60,14 @@ func BuildSubagentSessionKey(agentID, label string) string {
|
||||
// BuildCronSessionKey builds the session key for a cron job run.
|
||||
//
|
||||
// agent:{agentId}:cron:{jobID}:run:{runID}
|
||||
//
|
||||
// Guards against double-prefixing: if jobID is already a canonical session key
|
||||
// (e.g. "agent:X:..."), only the rest part is used to prevent
|
||||
// "agent:X:cron:agent:X:cron:..." duplication.
|
||||
func BuildCronSessionKey(agentID, jobID, runID string) string {
|
||||
if _, rest := ParseSessionKey(jobID); rest != "" {
|
||||
jobID = rest
|
||||
}
|
||||
return fmt.Sprintf("agent:%s:cron:%s:run:%s", agentID, jobID, runID)
|
||||
}
|
||||
|
||||
|
||||
@@ -247,11 +247,19 @@ func resolvePath(path, workspace string, restrict bool) (string, error) {
|
||||
target = filepath.Join(filepath.Dir(absResolved), target)
|
||||
}
|
||||
target = filepath.Clean(target)
|
||||
if !isPathInside(target, wsReal) {
|
||||
slog.Warn("security.broken_symlink_escape", "path", path, "target", target, "workspace", wsReal)
|
||||
|
||||
// Resolve through existing ancestors to catch chained symlinks
|
||||
// (e.g. link1 → link2 → /outside) where intermediate targets escape.
|
||||
resolved, resolveErr := resolveThroughExistingAncestors(target)
|
||||
if resolveErr != nil {
|
||||
slog.Warn("security.broken_symlink_resolve_failed", "path", path, "target", target)
|
||||
return "", fmt.Errorf("access denied: cannot resolve broken symlink target")
|
||||
}
|
||||
if !isPathInside(resolved, wsReal) {
|
||||
slog.Warn("security.broken_symlink_escape", "path", path, "target", resolved, "workspace", wsReal)
|
||||
return "", fmt.Errorf("access denied: broken symlink target outside workspace")
|
||||
}
|
||||
real = target
|
||||
real = resolved
|
||||
} else {
|
||||
// Truly non-existent file (not a symlink): resolve parent and re-validate.
|
||||
parentReal, parentErr := filepath.EvalSymlinks(filepath.Dir(absResolved))
|
||||
@@ -273,6 +281,14 @@ func resolvePath(path, workspace string, restrict bool) (string, error) {
|
||||
return "", fmt.Errorf("access denied: path outside workspace")
|
||||
}
|
||||
|
||||
// Reject paths with mutable symlink components (TOCTOU symlink rebind risk).
|
||||
// A symlink in the path whose parent directory is writable could be replaced
|
||||
// between resolution time and actual file operation.
|
||||
if hasMutableSymlinkParent(real) {
|
||||
slog.Warn("security.mutable_symlink_parent", "path", path, "resolved", real)
|
||||
return "", fmt.Errorf("access denied: path contains mutable symlink component")
|
||||
}
|
||||
|
||||
// Reject hardlinked files (nlink > 1) to prevent hardlink-based escapes.
|
||||
if err := checkHardlink(real); err != nil {
|
||||
return "", err
|
||||
@@ -289,6 +305,68 @@ func isPathInside(child, parent string) bool {
|
||||
return strings.HasPrefix(child, parent+string(filepath.Separator))
|
||||
}
|
||||
|
||||
// resolveThroughExistingAncestors resolves a path by finding the deepest
|
||||
// existing ancestor, canonicalizing it with EvalSymlinks, then appending
|
||||
// the remaining non-existent components. This handles broken symlinks
|
||||
// whose targets contain intermediate symlinks that escape the workspace.
|
||||
func resolveThroughExistingAncestors(target string) (string, error) {
|
||||
// Try full resolution first (target exists and all symlinks resolve)
|
||||
if real, err := filepath.EvalSymlinks(target); err == nil {
|
||||
return real, nil
|
||||
}
|
||||
|
||||
// Walk up to find the deepest existing ancestor
|
||||
current := target
|
||||
var tail []string
|
||||
for {
|
||||
parent := filepath.Dir(current)
|
||||
if parent == current {
|
||||
// Reached filesystem root without finding existing dir
|
||||
break
|
||||
}
|
||||
tail = append([]string{filepath.Base(current)}, tail...)
|
||||
current = parent
|
||||
|
||||
if realParent, err := filepath.EvalSymlinks(current); err == nil {
|
||||
// Found existing ancestor — canonicalize and rebuild
|
||||
result := realParent
|
||||
for _, component := range tail {
|
||||
result = filepath.Join(result, component)
|
||||
}
|
||||
return result, nil
|
||||
}
|
||||
}
|
||||
return filepath.Clean(target), nil
|
||||
}
|
||||
|
||||
// hasMutableSymlinkParent checks if any component of the resolved path is a symlink
|
||||
// whose parent directory is writable by the current process. A writable parent means
|
||||
// the symlink could be replaced between path resolution and actual file operation
|
||||
// (TOCTOU symlink rebind attack).
|
||||
func hasMutableSymlinkParent(path string) bool {
|
||||
clean := filepath.Clean(path)
|
||||
components := strings.Split(clean, string(filepath.Separator))
|
||||
current := string(filepath.Separator)
|
||||
for _, comp := range components {
|
||||
if comp == "" {
|
||||
continue
|
||||
}
|
||||
current = filepath.Join(current, comp)
|
||||
info, err := os.Lstat(current)
|
||||
if err != nil {
|
||||
break // non-existent — stop checking
|
||||
}
|
||||
if info.Mode()&os.ModeSymlink != 0 {
|
||||
// Symlink found — check if its parent dir is writable
|
||||
parentDir := filepath.Dir(current)
|
||||
if syscall.Access(parentDir, 0x2 /* W_OK */) == nil {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// checkHardlink rejects regular files with nlink > 1 (hardlink attack prevention).
|
||||
// Directories naturally have nlink > 1 and are exempt.
|
||||
func checkHardlink(path string) error {
|
||||
|
||||
@@ -75,6 +75,10 @@ var defaultDenyPatterns = []*regexp.Regexp{
|
||||
regexp.MustCompile(`\bDYLD_INSERT_LIBRARIES\s*=`),
|
||||
regexp.MustCompile(`\bLD_LIBRARY_PATH\s*=`),
|
||||
regexp.MustCompile(`/etc/ld\.so\.preload`),
|
||||
regexp.MustCompile(`\bGIT_EXTERNAL_DIFF\s*=`), // git diff arbitrary code exec
|
||||
regexp.MustCompile(`\bGIT_DIFF_OPTS\s*=`), // git diff behavior injection
|
||||
regexp.MustCompile(`\bBASH_ENV\s*=`), // shell init injection
|
||||
regexp.MustCompile(`\bENV\s*=.*\bsh\b`), // sh init injection
|
||||
|
||||
// ── Container escape ──
|
||||
regexp.MustCompile(`/var/run/docker\.sock|docker\.(sock|socket)`),
|
||||
|
||||
Reference in new issue
Block a user