From 441824a0db664f956c1c849ec743525c4e2a30ab Mon Sep 17 00:00:00 2001 From: xj Date: Tue, 24 Feb 2026 20:27:14 -0800 Subject: [PATCH] fix(security): remove exec context race and harden ipv6 SSRF --- pkg/agent/loop.go | 10 ---------- pkg/tools/cron.go | 24 +++++------------------- pkg/tools/registry.go | 13 ++++++++++++- pkg/tools/shell.go | 10 ++-------- pkg/tools/web.go | 26 +++++++++++++++++++++----- pkg/tools/web_test.go | 6 +++--- 6 files changed, 43 insertions(+), 46 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index dbd7f7263..dbc4a9b87 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -747,16 +747,6 @@ func (al *AgentLoop) updateToolContexts(agent *AgentInstance, channel, chatID st st.SetContext(channel, chatID) } } - if tool, ok := al.tools.Get("exec"); ok { - if et, ok := tool.(tools.ContextualTool); ok { - et.SetContext(channel, chatID) - } - } - if tool, ok := al.tools.Get("cron"); ok { - if ct, ok := tool.(tools.ContextualTool); ok { - ct.SetContext(channel, chatID) - } - } } // maybeSummarize triggers summarization if the session history exceeds thresholds. diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 6cb275463..c847d2d7a 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -3,7 +3,6 @@ package tools import ( "context" "fmt" - "sync" "time" "github.com/sipeed/picoclaw/pkg/bus" @@ -24,9 +23,6 @@ type CronTool struct { executor JobExecutor msgBus *bus.MessageBus execTool *ExecTool - channel string - chatID string - mu sync.RWMutex } // NewCronTool creates a new CronTool @@ -102,14 +98,6 @@ func (t *CronTool) Parameters() map[string]any { } } -// SetContext sets the current session context for job creation -func (t *CronTool) SetContext(channel, chatID string) { - t.mu.Lock() - defer t.mu.Unlock() - t.channel = channel - t.chatID = chatID -} - // Execute runs the tool with the given arguments func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult { action, ok := args["action"].(string) @@ -134,10 +122,8 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult } func (t *CronTool) addJob(args map[string]any) *ToolResult { - t.mu.RLock() - channel := t.channel - chatID := t.chatID - t.mu.RUnlock() + channel, _ := args["__channel"].(string) + chatID, _ := args["__chat_id"].(string) if channel == "" || chatID == "" { return ErrorResult("no session context (channel/chat_id not set). Use this tool in an active conversation.") @@ -296,9 +282,9 @@ func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string { // Execute command if present if job.Payload.Command != "" { args := map[string]any{ - "command": job.Payload.Command, - "__channel": channel, - "__chat_id": chatID, + "command": job.Payload.Command, + "__channel": channel, + "__chat_id": chatID, } result := t.execTool.Execute(ctx, args) diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go index 6ecb8ae7c..e37098b98 100644 --- a/pkg/tools/registry.go +++ b/pkg/tools/registry.go @@ -77,8 +77,19 @@ func (r *ToolRegistry) ExecuteWithContext( }) } + toolArgs := args + if channel != "" || chatID != "" { + toolArgs = make(map[string]interface{}, len(args)+2) + for k, v := range args { + toolArgs[k] = v + } + // Internal runtime context for auth/policy checks in tools. + toolArgs["__channel"] = channel + toolArgs["__chat_id"] = chatID + } + start := time.Now() - result := tool.Execute(ctx, args) + result := tool.Execute(ctx, toolArgs) duration := time.Since(start) // Log based on result type diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index f724b61e5..a444d8535 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -24,8 +24,6 @@ type ExecTool struct { allowPatterns []*regexp.Regexp restrictToWorkspace bool allowRemote bool - channel string - chatID string } var defaultDenyPatterns = []*regexp.Regexp{ @@ -149,7 +147,8 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult } if !t.allowRemote { - channel := strings.TrimSpace(t.channel) + channel, _ := args["__channel"].(string) + channel = strings.TrimSpace(channel) if channel == "" || !constants.IsInternalChannel(channel) { return ErrorResult("exec is restricted to internal channels") } @@ -345,8 +344,3 @@ func (t *ExecTool) SetAllowPatterns(patterns []string) error { } return nil } - -func (t *ExecTool) SetContext(channel, chatID string) { - t.channel = channel - t.chatID = chatID -} diff --git a/pkg/tools/web.go b/pkg/tools/web.go index 62494cbd6..1670799c9 100644 --- a/pkg/tools/web.go +++ b/pkg/tools/web.go @@ -11,6 +11,7 @@ import ( "net/url" "regexp" "strings" + "sync/atomic" "time" ) @@ -498,7 +499,7 @@ type WebFetchTool struct { // allowPrivateWebFetchHosts controls whether loopback/private hosts are allowed. // This is false in normal runtime to reduce SSRF exposure, and tests can override it temporarily. -var allowPrivateWebFetchHosts = false +var allowPrivateWebFetchHosts atomic.Bool func NewWebFetchTool(maxChars int) *WebFetchTool { if maxChars <= 0 { @@ -711,7 +712,7 @@ func (t *WebFetchTool) extractText(htmlContent string) string { func newSafeDialContext(dialer *net.Dialer) func(context.Context, string, string) (net.Conn, error) { return func(ctx context.Context, network, address string) (net.Conn, error) { - if allowPrivateWebFetchHosts { + if allowPrivateWebFetchHosts.Load() { return dialer.DialContext(ctx, network, address) } @@ -760,11 +761,12 @@ func newSafeDialContext(dialer *net.Dialer) func(context.Context, string, string } func isPrivateFetchHost(host string) bool { - if allowPrivateWebFetchHosts { + if allowPrivateWebFetchHosts.Load() { return false } canonicalHost := strings.ToLower(strings.TrimSpace(host)) + canonicalHost = strings.TrimSuffix(canonicalHost, ".") if canonicalHost == "" { return true } @@ -816,6 +818,20 @@ func isPrivateOrRestrictedIP(ip net.IP) bool { return false } - // IPv6 unique local addresses (fc00::/7) - return len(ip) == net.IPv6len && (ip[0]&0xfe) == 0xfc + if len(ip) == net.IPv6len { + // IPv6 unique local addresses (fc00::/7) + if (ip[0] & 0xfe) == 0xfc { + return true + } + // 6to4 addresses (2002::/16) can embed private IPv4 targets. + if ip[0] == 0x20 && ip[1] == 0x02 { + return true + } + // Teredo tunneling addresses (2001:0000::/32) can encapsulate private endpoints. + if ip[0] == 0x20 && ip[1] == 0x01 && ip[2] == 0x00 && ip[3] == 0x00 { + return true + } + } + + return false } diff --git a/pkg/tools/web_test.go b/pkg/tools/web_test.go index 7dfb1dd91..d07bad050 100644 --- a/pkg/tools/web_test.go +++ b/pkg/tools/web_test.go @@ -324,10 +324,10 @@ func TestWebFetchTool_extractText(t *testing.T) { func withPrivateWebFetchHostsAllowed(t *testing.T) { t.Helper() - previous := allowPrivateWebFetchHosts - allowPrivateWebFetchHosts = true + previous := allowPrivateWebFetchHosts.Load() + allowPrivateWebFetchHosts.Store(true) t.Cleanup(func() { - allowPrivateWebFetchHosts = previous + allowPrivateWebFetchHosts.Store(previous) }) }