diff --git a/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md b/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md new file mode 100644 index 000000000..303ddee38 --- /dev/null +++ b/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md @@ -0,0 +1,248 @@ +# GHSA-pv8c-p6jf-3fpp Security Context Analysis + +Date: 2026-02-25 +Repository: `sipeed/picoclaw` +Advisory: `GHSA-pv8c-p6jf-3fpp` (draft) + +## 1) Advisory Snapshot (What Is Known) + +From `gh api repos/sipeed/picoclaw/security-advisories/GHSA-pv8c-p6jf-3fpp`: + +- Summary: `Unauthenticated RCE and multiple vulnerabilities in PicoClaw` +- Severity: `critical` +- State: `draft` +- Vulnerable range: `0.1.1` +- Impact text references shell execution, filesystem tools, web fetch, skill install, channel auth, I2C/SPI, cron, subagent, OAuth, and session management. + +Source: +- https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp + +Important caveat: +- The advisory is still draft and does not yet publish patched versions, CVSS vectors, or a complete vulnerability decomposition. + +## 2) Threat Actors And Trust Boundaries + +### Threat actors + +1. Unauthenticated external attacker reaching webhook/channel ingress. +2. Authenticated but malicious user abusing tool-capable agent behavior. +3. Adversarial remote content source (web pages consumed via `web_fetch`). +4. Supply-chain adversary through registry content (`install_skill`). +5. Local host user/process with filesystem access (lower priority in this GHSA). + +### Core trust boundaries + +1. Channel/webhook ingress authentication. +2. Agent decision boundary (LLM can invoke privileged tools). +3. Tool boundary (`exec`, `cron.command`, `install_skill`, hardware writes). +4. Network egress boundary (`web_fetch` SSRF class). +5. Persistence boundary (cron jobs, session/state files). + +## 3) Code-Backed Security Context + +This section separates verified facts from inference. + +### 3.1 Verified: ingress-to-tool execution chain exists + +- Channel messages are published into bus and consumed by agent loop: + - `pkg/channels/base.go:84` + - `pkg/channels/base.go:98` + - `pkg/bus/bus.go:24` + - `pkg/agent/loop.go:157` + - `pkg/agent/loop.go:165` +- `exec` tool is registered on agent instances: + - `pkg/agent/instance.go:54` +- Unix command execution uses shell: + - `pkg/tools/shell.go:182` (`sh -c`) + +Security implication: +- Any ingress auth failure can become command-execution impact if tool policy is insufficient. + +### 3.2 Verified: WeCom signature fail-open class has been hardened + +- Signature check now rejects empty token/signature/timestamp/nonce: + - `pkg/channels/wecom.go:487` + - `pkg/channels/wecom.go:488` +- WeCom App channel constructor now requires token: + - `pkg/channels/wecom_app.go:121` + - `pkg/channels/wecom_app.go:123` + +Security implication: +- Closes a concrete empty-secret auth bypass condition for WeCom paths. + +### 3.3 Verified: SSRF controls added for `web_fetch` + +- `web_fetch` is exposed to agents: + - `pkg/agent/loop.go:112` +- Private/local host checks: + - `pkg/tools/web.go:505` + - `pkg/tools/web.go:631` +- Redirect target checks: + - `pkg/tools/web.go:531` + - `pkg/tools/web.go:535` + +Security implication: +- Basic SSRF hardening exists. + +Open verification gap: +- Need explicit confirmation that checks are enforced at connect-time (dial-time) to resist DNS rebinding/TOCTOU, not only pre-resolution checks. + +### 3.4 Verified: cron can persist shell command execution + +- Cron supports scheduling a shell command: + - `pkg/tools/cron.go:54` + - `pkg/tools/cron.go:71` + - `pkg/tools/cron.go:181` + - `pkg/tools/cron.go:206` + - `pkg/tools/cron.go:284` +- Cron store persistence permission: + - `pkg/cron/service.go:343` (`0600`) + +Security implication: +- Command execution can be made persistent once attacker reaches tool invocation. + +### 3.5 Verified: skill install is a supply-chain boundary + +- Registry download/install path: + - `pkg/tools/skills_install.go:118` +- Malware blocked, suspicious currently warning-only: + - `pkg/tools/skills_install.go:134` + - `pkg/tools/skills_install.go:163` + +Security implication: +- Warning-only suspicious policy is weak in automated LLM tool-calling context. + +### 3.6 Verified: hardware write "confirm" is not an authorization boundary + +- I2C/SPI write-like operations rely on `confirm` parameter: + - `pkg/tools/i2c_linux.go:218` + - `pkg/tools/spi_linux.go:70` + +Security implication: +- `confirm` is model-supplied input; it reduces accidental writes but does not protect against adversarial prompting or ingress compromise. + +### 3.7 Verified: session persistence permissions hardened + +- Session save temp file mode: + - `pkg/session/manager.go:217` (`0600`) + +## 4) Attack/Vulnerability Chains + +Non-operational risk chains for analysis. + +### Chain A: Unauthenticated ingress -> tool invocation -> host command execution + +1. Attacker reaches unauthenticated/weakly authenticated channel ingress. +2. Message flows into agent loop. +3. LLM invokes `exec`. +4. Command runs in host context. + +Impact: +- Remote code execution. + +### Chain B: Prompted SSRF via `web_fetch` + +1. Adversary induces fetch of attacker-selected URL. +2. Tool accesses internal resources through direct or redirect flow. +3. Returned content is exposed to model and/or user. + +Impact: +- Internal service discovery, metadata leakage, token/secret exposure. + +### Chain C: RCE-to-persistence via `cron.command` + +1. Initial execution foothold obtained. +2. Scheduled commands created through cron tool. +3. Commands run later out-of-band. + +Impact: +- Durable persistence and repeated post-exploitation. + +### Chain D: Supply-chain through `install_skill` + +1. Adversarial skill selected/installed from registry. +2. Artifact persists in workspace. +3. Future workflow/tool usage can be influenced. + +Impact: +- Long-lived compromise path or latent privilege abuse. + +### Chain E: Indirect prompt injection through fetched content + +1. `web_fetch` imports untrusted text into model context. +2. Malicious content instructs model to call privileged tools. +3. Agent executes commands/actions absent robust policy gating. + +Impact: +- Tool-abuse without direct channel compromise. + +## 5) What / Why / How (Mitigation Interpretation) + +### What is implemented now + +1. WeCom signature verification fail-closed behavior. +2. WeCom App token requirement. +3. `web_fetch` private/redirect host restrictions. +4. Session file permission tightening (`0600`). + +### Why this helps + +- Reduces direct unauthenticated ingress abuse in WeCom. +- Reduces straightforward SSRF to loopback/private network. +- Reduces local confidentiality exposure of session artifacts. + +### How this is still insufficient (current residual risk) + +1. Channel auth coverage outside WeCom is not yet fully documented as audited. +2. `exec` remains high-impact and currently denylist-driven. +3. `confirm` flags are not true auth controls. +4. Suspicious skill installs are warning-only. +5. SSRF rebinding/TOCTOU protection needs explicit verification. +6. Prompt injection via fetched content is an explicit abuse path. + +## 6) Priority Recommendations + +### P0 (must address for network-exposed deployments) + +1. Complete channel-by-channel ingress auth audit and fail-closed startup for missing critical secrets. +2. Enforce fail-closed policy on `exec` for remote channels by default. +3. Validate SSRF protection against DNS rebinding/TOCTOU (connect-time checks) and metadata IP ranges. +4. Do not treat `confirm` as security control; add policy/approval gate for dangerous tools. + +### P1 (high-value next controls) + +1. Shift from denylist-heavy exec filtering to constrained allowlist profiles. +2. Block suspicious skill installs by default with explicit operator override. +3. Add auditable operator-visible controls for cron command creation/removal. + +### P2 (defense-in-depth) + +1. Channel-level abuse throttling/rate limits. +2. Expanded security telemetry for tool invocation chains and anomaly patterns. +3. Prompt-injection resilience policy for web-ingested content. + +## 7) CWE Mapping + +- Missing auth on critical function: CWE-306 +- OS command execution/injection class: CWE-78 +- SSRF: CWE-918 +- Blacklist weakness: CWE-184 +- Incorrect file permissions: CWE-276 + +## 8) References + +1. GitHub advisory: + - https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp +2. GitHub webhook signature validation guidance: + - https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries +3. OWASP SSRF prevention cheat sheet: + - https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html +4. CWE entries: + - https://cwe.mitre.org/data/definitions/306.html + - https://cwe.mitre.org/data/definitions/78.html + - https://cwe.mitre.org/data/definitions/918.html + - https://cwe.mitre.org/data/definitions/184.html + - https://cwe.mitre.org/data/definitions/276.html +5. Go APIs: + - https://pkg.go.dev/os#WriteFile + - https://pkg.go.dev/crypto/subtle#ConstantTimeCompare diff --git a/docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md b/docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md new file mode 100644 index 000000000..8b0b87a8e --- /dev/null +++ b/docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md @@ -0,0 +1,552 @@ +# GHSA-pv8c-p6jf-3fpp Hardening Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Break the unauthenticated-to-RCE chain by enforcing fail-closed ingress auth and fail-closed dangerous tool policies in network-exposed deployments. + +**Architecture:** Apply trust-boundary hardening in layers: channel ingress verification, strict tool execution policy, SSRF connect-time controls, and persistence/permissions hardening. Implement with TDD and small commits so each security claim is backed by one regression test. + +**Tech Stack:** Go, `testing`, `httptest`, PicoClaw `pkg/channels`, `pkg/tools`, `pkg/config`, `pkg/state`. + +--- + +### Task 0: Channel Ingress Auth Matrix And Fail-Closed Validation + +**Files:** +- Modify: `pkg/channels/manager.go` +- Create: `pkg/channels/security_matrix_test.go` +- Modify: `docs/tools_configuration.md` +- Modify: `.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md` + +**Step 1: Write the failing test** + +Add `pkg/channels/security_matrix_test.go`: + +```go +func TestChannelSecurityMatrix_FailClosedRequirements(t *testing.T) { + cfg := config.DefaultConfig() + msgBus := bus.NewMessageBus() + + cfg.Channels.WeComApp.Enabled = true + cfg.Channels.WeComApp.CorpID = "corp" + cfg.Channels.WeComApp.CorpSecret = "secret" + cfg.Channels.WeComApp.AgentID = 1000002 + cfg.Channels.WeComApp.Token = "" // must fail closed + + m, err := NewManager(cfg, msgBus) + if err != nil { + t.Fatalf("NewManager error: %v", err) + } + + if _, ok := m.channels["wecom_app"]; ok { + t.Fatal("wecom_app must not be enabled without token") + } +} +``` + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/channels -run TestChannelSecurityMatrix_FailClosedRequirements -v` +Expected: FAIL if manager still enables channels with missing critical auth fields. + +**Step 3: Write minimal implementation** + +In `pkg/channels/manager.go`, enforce explicit fail-closed checks before channel creation: + +- `wecom`: require `Enabled && Token != ""` +- `wecom_app`: require `Enabled && CorpID != "" && CorpSecret != "" && AgentID != 0 && Token != ""` +- add similar explicit checks for any webhook-based channel that has a verification secret field. + +Add inline comment near each check: +- `"Fail closed: do not expose webhook channel without verification secret."` + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/channels -run TestChannelSecurityMatrix_FailClosedRequirements -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/channels/manager.go pkg/channels/security_matrix_test.go docs/tools_configuration.md .docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md +git commit -m "fix(security): enforce fail-closed channel auth initialization" +``` + +--- + +### Task 1: Add Exec Remote Policy Config + +**Files:** +- Modify: `pkg/config/config.go` +- Modify: `pkg/config/defaults.go` +- Modify: `pkg/config/config_test.go` +- Modify: `docs/tools_configuration.md` + +**Step 1: Write the failing test** + +Add tests in `pkg/config/config_test.go`: + +```go +func TestDefaultConfig_ExecAllowRemoteDisabled(t *testing.T) { + cfg := DefaultConfig() + if cfg.Tools.Exec.AllowRemote { + t.Fatal("Tools.Exec.AllowRemote should default to false") + } +} + +func TestLoadConfig_ExecAllowRemoteFromEnv(t *testing.T) { + t.Setenv("PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE", "true") + cfg, err := LoadConfig(filepath.Join(t.TempDir(), "missing.json")) + if err != nil { + t.Fatalf("LoadConfig error: %v", err) + } + if !cfg.Tools.Exec.AllowRemote { + t.Fatal("expected Tools.Exec.AllowRemote=true from env") + } +} +``` + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/config -run 'TestDefaultConfig_ExecAllowRemoteDisabled|TestLoadConfig_ExecAllowRemoteFromEnv' -v` +Expected: FAIL because `ExecConfig.AllowRemote` does not exist. + +**Step 3: Write minimal implementation** + +Update `pkg/config/config.go`: + +```go +type ExecConfig struct { + EnableDenyPatterns bool `json:"enable_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` + CustomDenyPatterns []string `json:"custom_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + AllowRemote bool `json:"allow_remote" env:"PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE"` +} +``` + +Update `pkg/config/defaults.go`: + +```go +Exec: ExecConfig{ + EnableDenyPatterns: true, + AllowRemote: false, +}, +``` + +Document in `docs/tools_configuration.md`: +- `tools.exec.allow_remote` / `PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE` +- default `false` +- security note: enabling allows remote channels to run shell commands. + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/config -run 'TestDefaultConfig_ExecAllowRemoteDisabled|TestLoadConfig_ExecAllowRemoteFromEnv' -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/config/config.go pkg/config/defaults.go pkg/config/config_test.go docs/tools_configuration.md +git commit -m "feat(security): add exec remote policy config" +``` + +--- + +### Task 2: Enforce Fail-Closed Exec Channel Guard + +**Files:** +- Modify: `pkg/tools/shell.go` +- Modify: `pkg/tools/shell_test.go` +- Modify: `pkg/agent/loop.go` +- Modify: `pkg/agent/loop_test.go` +- Modify: `pkg/constants/channels.go` + +**Step 1: Write the failing tests** + +Add tests in `pkg/tools/shell_test.go`: + +```go +func TestShellTool_NoContextSetBlocked(t *testing.T) { + cfg := config.DefaultConfig() + tool := NewExecToolWithConfig("", false, cfg) + result := tool.Execute(context.Background(), map[string]any{"command": "echo hi"}) + if !result.IsError || !strings.Contains(result.ForLLM, "disabled for remote channels") { + t.Fatalf("expected fail-closed block, got: %#v", result) + } +} + +func TestShellTool_RemoteChannelBlockedByDefault(t *testing.T) { + cfg := config.DefaultConfig() + tool := NewExecToolWithConfig("", false, cfg) + tool.SetContext("telegram", "chat-1") + result := tool.Execute(context.Background(), map[string]any{"command": "echo hi"}) + if !result.IsError { + t.Fatal("expected remote-channel exec to be blocked") + } +} + +func TestShellTool_InternalChannelAllowed(t *testing.T) { + cfg := config.DefaultConfig() + tool := NewExecToolWithConfig("", false, cfg) + tool.SetContext("cli", "direct") + result := tool.Execute(context.Background(), map[string]any{"command": "echo hi"}) + if result.IsError { + t.Fatalf("expected internal channel allow, got: %s", result.ForLLM) + } +} +``` + +Add tests in `pkg/agent/loop_test.go` to verify `updateToolContexts` sets context for `exec` and `cron`. + +Add tests in `pkg/constants/channels.go` companion test to verify strict allowlist (`cli`, `system`, `subagent`) and remote channels return false. + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/tools -run 'TestShellTool_NoContextSetBlocked|TestShellTool_RemoteChannelBlockedByDefault|TestShellTool_InternalChannelAllowed' -v` +Expected: FAIL. + +Run: `go test ./pkg/agent -run TestUpdateToolContexts_ExecAndCron -v` +Expected: FAIL. + +**Step 3: Write minimal implementation** + +In `pkg/tools/shell.go`: +- Add fields: `allowRemote`, `channel`, `chatID` +- Implement `SetContext(channel, chatID string)` +- Add compile-time assertion: + +```go +var _ ContextualTool = (*ExecTool)(nil) +``` + +- Fail-closed guard in `Execute`: + +```go +if !t.allowRemote && !constants.IsInternalChannel(t.channel) { + return ErrorResult("command execution is disabled for remote channels; set tools.exec.allow_remote=true to override") +} +``` + +Do not add `t.channel != ""` check. + +Wire `allowRemote` from config in constructor. + +In `pkg/agent/loop.go` update `updateToolContexts` to call `SetContext` for `exec` and `cron`. + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/tools -run 'TestShellTool_NoContextSetBlocked|TestShellTool_RemoteChannelBlockedByDefault|TestShellTool_InternalChannelAllowed' -v` +Expected: PASS. + +Run: `go test ./pkg/agent -run TestUpdateToolContexts_ExecAndCron -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/tools/shell.go pkg/tools/shell_test.go pkg/agent/loop.go pkg/agent/loop_test.go pkg/constants/channels.go +git commit -m "fix(security): enforce fail-closed exec channel policy" +``` + +--- + +### Task 3: Harden Cron Command Scheduling Policy + +**Files:** +- Modify: `pkg/tools/cron.go` +- Create: `pkg/tools/cron_test.go` +- Modify: `docs/tools_configuration.md` + +**Step 1: Write the failing tests** + +Add tests in `pkg/tools/cron_test.go`: + +```go +func TestCronTool_AddRemoteCommandBlocked(t *testing.T) { + tool := newCronToolForTest(t, "telegram", "chat1") + result := tool.Execute(context.Background(), map[string]any{ + "action": "add", + "message": "run command", + "at_seconds": float64(60), + "command": "id", + }) + if !result.IsError || !strings.Contains(result.ForLLM, "not allowed from remote channels") { + t.Fatalf("expected remote command block, got: %#v", result) + } +} + +func TestCronTool_AddCommandRequiresConfirm(t *testing.T) { + tool := newCronToolForTest(t, "cli", "direct") + result := tool.Execute(context.Background(), map[string]any{ + "action": "add", + "message": "run command", + "at_seconds": float64(60), + "command": "id", + }) + if !result.IsError || !strings.Contains(result.ForLLM, "command_confirm=true") { + t.Fatalf("expected command_confirm validation error, got: %#v", result) + } +} +``` + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/tools -run 'TestCronTool_AddRemoteCommandBlocked|TestCronTool_AddCommandRequiresConfirm' -v` +Expected: FAIL. + +**Step 3: Write minimal implementation** + +In `pkg/tools/cron.go`: + +1. Enforce remote-channel block for `command` scheduling: + +```go +if command != "" && !constants.IsInternalChannel(channel) { + return ErrorResult("cron command scheduling is not allowed from remote channels") +} +``` + +2. Require `command_confirm=true` only for internal channels as friction: + +```go +if command != "" { + commandConfirm, _ := args["command_confirm"].(bool) + if !commandConfirm { + return ErrorResult("command_confirm=true is required when scheduling shell commands") + } +} +``` + +3. Add comment in code/docs: +- `command_confirm` is defense-in-depth only, not an authentication boundary. + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/tools -run 'TestCronTool_AddRemoteCommandBlocked|TestCronTool_AddCommandRequiresConfirm' -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/tools/cron.go pkg/tools/cron_test.go docs/tools_configuration.md +git commit -m "fix(security): restrict cron command scheduling to internal channels" +``` + +--- + +### Task 4: Block Suspicious Skill Installs By Default With Explicit Criteria + +**Files:** +- Modify: `pkg/config/config.go` +- Modify: `pkg/config/defaults.go` +- Modify: `pkg/tools/skills_install.go` +- Modify: `pkg/tools/skills_install_test.go` +- Modify: `pkg/agent/loop.go` + +**Step 1: Write the failing tests** + +In `pkg/tools/skills_install_test.go`, add: + +```go +func TestInstallSkillTool_BlocksSuspiciousByDefault(t *testing.T) { ... } +func TestInstallSkillTool_AllowsSuspiciousWithConfig(t *testing.T) { ... } +``` + +Add one concrete criteria-driven test: +- if registry returns `IsSuspicious=true`, install must fail when config default is false. + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/tools -run 'TestInstallSkillTool_BlocksSuspiciousByDefault|TestInstallSkillTool_AllowsSuspiciousWithConfig' -v` +Expected: FAIL. + +**Step 3: Write minimal implementation** + +Add config: + +```go +AllowSuspiciousInstall bool `json:"allow_suspicious_install" env:"PICOCLAW_SKILLS_ALLOW_SUSPICIOUS_INSTALL"` +``` + +Default: + +```go +AllowSuspiciousInstall: false, +``` + +In `pkg/tools/skills_install.go`: +- add constructor param `allowSuspicious bool` +- if `result.IsSuspicious && !allowSuspicious`: remove target dir and return error. + +In `pkg/agent/loop.go`, wire constructor with config. + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/tools -run 'TestInstallSkillTool_BlocksSuspiciousByDefault|TestInstallSkillTool_AllowsSuspiciousWithConfig' -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/config/config.go pkg/config/defaults.go pkg/tools/skills_install.go pkg/tools/skills_install_test.go pkg/agent/loop.go +git commit -m "fix(security): block suspicious skill installs by default" +``` + +--- + +### Task 5: State File Permission Hardening (Final Path + Directory) + +**Files:** +- Modify: `pkg/state/state.go` +- Modify: `pkg/state/state_test.go` + +**Step 1: Write the failing test** + +Add tests in `pkg/state/state_test.go`: + +```go +func TestStateFilePermissions0600(t *testing.T) { ... } +func TestStateDirPermissions0700(t *testing.T) { ... } +``` + +Assertions: +- `state/state.json` mode is `0600`. +- `state/` mode is `0700` (non-Windows). + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/state -run 'TestStateFilePermissions0600|TestStateDirPermissions0700' -v` +Expected: FAIL. + +**Step 3: Write minimal implementation** + +In `pkg/state/state.go`: + +1. Create state dir with `0700`: + +```go +os.MkdirAll(stateDir, 0o700) +``` + +2. Write temp file with `0600`: + +```go +os.WriteFile(tempFile, data, 0o600) +``` + +3. After rename, enforce final mode: + +```go +if err := os.Chmod(sm.stateFile, 0o600); err != nil { ... } +``` + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/state -run 'TestStateFilePermissions0600|TestStateDirPermissions0700' -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/state/state.go pkg/state/state_test.go +git commit -m "fix(security): enforce secure state file and directory permissions" +``` + +--- + +### Task 6: Strengthen SSRF Validation And Tests (Rebinding/IPv6/Metadata) + +**Files:** +- Modify: `pkg/tools/web.go` +- Modify: `pkg/tools/web_test.go` + +**Step 1: Write the failing tests** + +Add tests in `pkg/tools/web_test.go`: + +```go +func TestWebFetch_BlocksIPv6MappedLoopback(t *testing.T) { ... } // ::ffff:127.0.0.1 +func TestWebFetch_BlocksMetadataIP(t *testing.T) { ... } // 169.254.169.254 +func TestWebFetch_RedirectToPrivateBlocked(t *testing.T) { ... } // existing + strict assertion +func TestWebFetch_DNSRebindingMitigationConnectTime(t *testing.T) { ... } // custom dialer check behavior +``` + +**Step 2: Run test to verify it fails** + +Run: `go test ./pkg/tools -run 'TestWebFetch_(BlocksIPv6MappedLoopback|BlocksMetadataIP|RedirectToPrivateBlocked|DNSRebindingMitigationConnectTime)' -v` +Expected: FAIL for missing checks/coverage. + +**Step 3: Write minimal implementation** + +In `pkg/tools/web.go`: +- Ensure host classification includes IPv4/IPv6 loopback/link-local/private ranges and metadata IP. +- Ensure enforcement at connect-time in HTTP client transport/dial path, not only preflight hostname checks. +- Keep redirect checks. + +**Step 4: Run test to verify it passes** + +Run: `go test ./pkg/tools -run 'TestWebFetch_(BlocksIPv6MappedLoopback|BlocksMetadataIP|RedirectToPrivateBlocked|DNSRebindingMitigationConnectTime)' -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add pkg/tools/web.go pkg/tools/web_test.go +git commit -m "fix(security): harden web_fetch against advanced SSRF vectors" +``` + +--- + +### Task 7: Regression Sweep And Upgrade Notes + +**Files:** +- Modify: `pkg/channels/wecom_test.go` +- Modify: `pkg/channels/wecom_app_test.go` +- Modify: `docs/tools_configuration.md` +- Modify: `.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md` +- Modify: `README.md` + +**Step 1: Write remaining regression tests/document checks** + +Ensure explicit regressions exist for: +- empty token verification failures in WeCom paths +- remote exec blocked by default +- remote cron command scheduling blocked +- suspicious skill install blocked by default + +**Step 2: Run verification commands** + +Run: + +```bash +go test ./pkg/config ./pkg/tools ./pkg/channels ./pkg/state -v +go vet ./... +staticcheck ./... +``` + +Expected: PASS. + +**Step 3: Write upgrade notes** + +Document new keys and defaults: +- `tools.exec.allow_remote=false` +- `tools.skills.allow_suspicious_install=false` +- cron command restrictions and `command_confirm` + +Add rollback instructions: +- temporary override env vars for emergency compatibility. + +**Step 4: Commit** + +```bash +git add pkg/channels/wecom_test.go pkg/channels/wecom_app_test.go docs/tools_configuration.md .docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md README.md +git commit -m "docs(security): add regressions and upgrade notes for GHSA hardening" +``` + +**Step 5: Prepare PR** + +Include: +- broken attack-chain summary (before/after) +- tests proving each boundary +- migration notes +- residual risks + follow-up tasks (human approval flow for dangerous actions) + diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index dbc4a9b87..dbd7f7263 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -747,6 +747,16 @@ 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/config/config.go b/pkg/config/config.go index 6f76614cf..8ff2924c8 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -464,6 +464,7 @@ type CronToolsConfig struct { type ExecConfig struct { EnableDenyPatterns bool `json:"enable_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` + AllowRemote bool `json:"allow_remote" env:"PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE"` CustomDenyPatterns []string `json:"custom_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` } diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index b96ee4d89..ef69b2d75 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -297,6 +297,7 @@ func DefaultConfig() *Config { }, Exec: ExecConfig{ EnableDenyPatterns: true, + AllowRemote: false, }, Skills: SkillsToolsConfig{ Registries: SkillsRegistriesConfig{ diff --git a/pkg/session/manager.go b/pkg/session/manager.go index 08f0b0ad2..00e5aa76a 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -32,7 +32,7 @@ func NewSessionManager(storage string) *SessionManager { } if storage != "" { - os.MkdirAll(storage, 0o755) + os.MkdirAll(storage, 0o700) sm.loadSessions() } @@ -214,7 +214,7 @@ func (sm *SessionManager) Save(key string) error { _ = tmpFile.Close() return err } - if err := tmpFile.Chmod(0o644); err != nil { + if err := tmpFile.Chmod(0o600); err != nil { _ = tmpFile.Close() return err } diff --git a/pkg/state/state.go b/pkg/state/state.go index 1a92f82ed..8eb9c0da2 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -38,7 +38,9 @@ func NewManager(workspace string) *Manager { oldStateFile := filepath.Join(workspace, "state.json") // Create state directory if it doesn't exist - os.MkdirAll(stateDir, 0o755) + if err := os.MkdirAll(stateDir, 0o700); err != nil { + log.Printf("[WARN] state: failed to create state directory %s: %v", stateDir, err) + } sm := &Manager{ workspace: workspace, @@ -139,7 +141,7 @@ func (sm *Manager) saveAtomic() error { } // Write to temp file - if err := os.WriteFile(tempFile, data, 0o644); err != nil { + if err := os.WriteFile(tempFile, data, 0o600); err != nil { return fmt.Errorf("failed to write temp file: %w", err) } @@ -150,6 +152,11 @@ func (sm *Manager) saveAtomic() error { return fmt.Errorf("failed to rename temp file: %w", err) } + // Ensure restrictive permissions even when replacing existing files. + if err := os.Chmod(sm.stateFile, 0o600); err != nil { + return fmt.Errorf("failed to set state file permissions: %w", err) + } + return nil } diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 562fffc84..6cb275463 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -8,6 +8,7 @@ import ( "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/constants" "github.com/sipeed/picoclaw/pkg/cron" "github.com/sipeed/picoclaw/pkg/utils" ) @@ -72,6 +73,10 @@ func (t *CronTool) Parameters() map[string]any { "type": "string", "description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message. 'deliver' will be forced to false for commands.", }, + "command_confirm": map[string]any{ + "type": "boolean", + "description": "Required when using command=true. Must be true to explicitly confirm scheduling a shell command.", + }, "at_seconds": map[string]any{ "type": "integer", "description": "One-time reminder: seconds from now when to trigger (e.g., 600 for 10 minutes later). Use this for one-time reminders like 'remind me in 10 minutes'.", @@ -179,7 +184,15 @@ func (t *CronTool) addJob(args map[string]any) *ToolResult { } command, _ := args["command"].(string) + commandConfirm, _ := args["command_confirm"].(bool) if command != "" { + if !constants.IsInternalChannel(channel) { + return ErrorResult("scheduling command execution is restricted to internal channels") + } + if !commandConfirm { + return ErrorResult("command_confirm=true is required to schedule command execution") + } + // Commands must be processed by agent/exec tool, so deliver must be false (or handled specifically) // Actually, let's keep deliver=false to let the system know it's not a simple chat message // But for our new logic in ExecuteJob, we can handle it regardless of deliver flag if Payload.Command is set. @@ -283,7 +296,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, + "command": job.Payload.Command, + "__channel": channel, + "__chat_id": chatID, } result := t.execTool.Execute(ctx, args) diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 6883172cd..f724b61e5 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -14,6 +14,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/constants" ) type ExecTool struct { @@ -22,6 +23,9 @@ type ExecTool struct { denyPatterns []*regexp.Regexp allowPatterns []*regexp.Regexp restrictToWorkspace bool + allowRemote bool + channel string + chatID string } var defaultDenyPatterns = []*regexp.Regexp{ @@ -75,11 +79,13 @@ func NewExecTool(workingDir string, restrict bool) *ExecTool { func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) *ExecTool { denyPatterns := make([]*regexp.Regexp, 0) + allowRemote := true enableDenyPatterns := true if config != nil { execConfig := config.Tools.Exec enableDenyPatterns = execConfig.EnableDenyPatterns + allowRemote = execConfig.AllowRemote if enableDenyPatterns { denyPatterns = append(denyPatterns, defaultDenyPatterns...) if len(execConfig.CustomDenyPatterns) > 0 { @@ -107,6 +113,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf denyPatterns: denyPatterns, allowPatterns: nil, restrictToWorkspace: restrict, + allowRemote: allowRemote, } } @@ -141,6 +148,13 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult return ErrorResult("command is required") } + if !t.allowRemote { + channel := strings.TrimSpace(t.channel) + if channel == "" || !constants.IsInternalChannel(channel) { + return ErrorResult("exec is restricted to internal channels") + } + } + cwd := t.workingDir if wd, ok := args["working_dir"].(string); ok && wd != "" { if t.restrictToWorkspace && t.workingDir != "" { @@ -331,3 +345,8 @@ 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 968579dea..62494cbd6 100644 --- a/pkg/tools/web.go +++ b/pkg/tools/web.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "net/url" "regexp" @@ -495,6 +496,10 @@ type WebFetchTool struct { proxy string } +// 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 + func NewWebFetchTool(maxChars int) *WebFetchTool { if maxChars <= 0 { maxChars = 50000 @@ -559,6 +564,10 @@ func (t *WebFetchTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("missing domain in URL") } + if isPrivateFetchHost(parsedURL.Hostname()) { + return ErrorResult("fetching private or local network hosts is not allowed") + } + maxChars := t.maxChars if mc, ok := args["maxChars"].(float64); ok { if int(mc) > 100 { @@ -573,17 +582,42 @@ func (t *WebFetchTool) Execute(ctx context.Context, args map[string]any) *ToolRe req.Header.Set("User-Agent", userAgent) - client, err := createHTTPClient(t.proxy, 60*time.Second) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to create HTTP client: %v", err)) + // Build transport with SSRF-safe dial context. + dialer := &net.Dialer{ + Timeout: 15 * time.Second, + KeepAlive: 30 * time.Second, + } + transport := &http.Transport{ + MaxIdleConns: 10, + IdleConnTimeout: 30 * time.Second, + DisableCompression: false, + TLSHandshakeTimeout: 15 * time.Second, + DialContext: newSafeDialContext(dialer), } - // Configure redirect handling - client.CheckRedirect = func(req *http.Request, via []*http.Request) error { - if len(via) >= 5 { - return fmt.Errorf("stopped after 5 redirects") + // Preserve proxy support from upstream. + if t.proxy != "" { + proxy, err := url.Parse(t.proxy) + if err != nil { + return ErrorResult(fmt.Sprintf("invalid proxy URL: %v", err)) } - return nil + transport.Proxy = http.ProxyURL(proxy) + } else { + transport.Proxy = http.ProxyFromEnvironment + } + + client := &http.Client{ + Timeout: 60 * time.Second, + Transport: transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 5 { + return fmt.Errorf("stopped after 5 redirects") + } + if isPrivateFetchHost(req.URL.Hostname()) { + return fmt.Errorf("redirect target is private or local network host") + } + return nil + }, } resp, err := client.Do(req) @@ -674,3 +708,114 @@ func (t *WebFetchTool) extractText(htmlContent string) string { return strings.Join(cleanLines, "\n") } + +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 { + return dialer.DialContext(ctx, network, address) + } + + host, port, err := net.SplitHostPort(address) + if err != nil { + return nil, fmt.Errorf("invalid target address %q: %w", address, err) + } + if host == "" { + return nil, fmt.Errorf("empty target host") + } + + if ip := net.ParseIP(host); ip != nil { + if isPrivateOrRestrictedIP(ip) { + return nil, fmt.Errorf("blocked private or local target: %s", host) + } + return dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port)) + } + + ipAddrs, err := net.DefaultResolver.LookupIPAddr(ctx, host) + if err != nil { + return nil, fmt.Errorf("failed to resolve %s: %w", host, err) + } + + attempted := 0 + var lastErr error + for _, ipAddr := range ipAddrs { + if isPrivateOrRestrictedIP(ipAddr.IP) { + continue + } + attempted++ + conn, err := dialer.DialContext(ctx, network, net.JoinHostPort(ipAddr.IP.String(), port)) + if err == nil { + return conn, nil + } + lastErr = err + } + + if attempted == 0 { + return nil, fmt.Errorf("all resolved addresses for %s are private or restricted", host) + } + if lastErr != nil { + return nil, fmt.Errorf("failed connecting to public addresses for %s: %w", host, lastErr) + } + return nil, fmt.Errorf("failed connecting to public addresses for %s", host) + } +} + +func isPrivateFetchHost(host string) bool { + if allowPrivateWebFetchHosts { + return false + } + + canonicalHost := strings.ToLower(strings.TrimSpace(host)) + if canonicalHost == "" { + return true + } + + if canonicalHost == "localhost" || strings.HasSuffix(canonicalHost, ".localhost") { + return true + } + + ip := net.ParseIP(canonicalHost) + if ip != nil { + return isPrivateOrRestrictedIP(ip) + } + + ips, err := net.LookupIP(canonicalHost) + if err != nil { + return true + } + + for _, resolved := range ips { + if isPrivateOrRestrictedIP(resolved) { + return true + } + } + + return false +} + +func isPrivateOrRestrictedIP(ip net.IP) bool { + if ip == nil { + return true + } + + if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || + ip.IsMulticast() || ip.IsUnspecified() { + return true + } + + if ip4 := ip.To4(); ip4 != nil { + // IPv4 private, loopback, link-local, and carrier-grade NAT ranges. + if ip4[0] == 10 || + ip4[0] == 127 || + ip4[0] == 0 || + (ip4[0] == 172 && ip4[1] >= 16 && ip4[1] <= 31) || + (ip4[0] == 192 && ip4[1] == 168) || + (ip4[0] == 169 && ip4[1] == 254) || + (ip4[0] == 100 && ip4[1] >= 64 && ip4[1] <= 127) { + return true + } + return false + } + + // IPv6 unique local addresses (fc00::/7) + return len(ip) == net.IPv6len && (ip[0]&0xfe) == 0xfc +} diff --git a/pkg/tools/web_test.go b/pkg/tools/web_test.go index 2cd79eb24..7dfb1dd91 100644 --- a/pkg/tools/web_test.go +++ b/pkg/tools/web_test.go @@ -12,6 +12,8 @@ import ( // TestWebTool_WebFetch_Success verifies successful URL fetching func TestWebTool_WebFetch_Success(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/html") w.WriteHeader(http.StatusOK) @@ -45,6 +47,8 @@ func TestWebTool_WebFetch_Success(t *testing.T) { // TestWebTool_WebFetch_JSON verifies JSON content handling func TestWebTool_WebFetch_JSON(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + testData := map[string]string{"key": "value", "number": "123"} expectedJSON, _ := json.MarshalIndent(testData, "", " ") @@ -137,6 +141,8 @@ func TestWebTool_WebFetch_MissingURL(t *testing.T) { // TestWebTool_WebFetch_Truncation verifies content truncation func TestWebTool_WebFetch_Truncation(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + longContent := strings.Repeat("x", 20000) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -204,6 +210,8 @@ func TestWebTool_WebSearch_MissingQuery(t *testing.T) { // TestWebTool_WebFetch_HTMLExtraction verifies HTML text extraction func TestWebTool_WebFetch_HTMLExtraction(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/html") w.WriteHeader(http.StatusOK) @@ -314,6 +322,49 @@ func TestWebFetchTool_extractText(t *testing.T) { } } +func withPrivateWebFetchHostsAllowed(t *testing.T) { + t.Helper() + previous := allowPrivateWebFetchHosts + allowPrivateWebFetchHosts = true + t.Cleanup(func() { + allowPrivateWebFetchHosts = previous + }) +} + +func TestWebTool_WebFetch_PrivateHostBlocked(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://127.0.0.1:0", + }) + + if !result.IsError { + t.Errorf("expected error for private host URL, got success") + } + if !strings.Contains(result.ForLLM, "private or local network") && !strings.Contains(result.ForUser, "private or local network") { + t.Errorf("expected private host block message, got %q", result.ForLLM) + } +} + +func TestWebTool_WebFetch_PrivateHostAllowedForTests(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + w.Write([]byte("ok")) + })) + defer server.Close() + + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": server.URL, + }) + + if result.IsError { + t.Errorf("expected success when private host access is allowed in tests, got %q", result.ForLLM) + } +} + // TestWebTool_WebFetch_MissingDomain verifies error handling for URL without domain func TestWebTool_WebFetch_MissingDomain(t *testing.T) { tool := NewWebFetchTool(50000)