From be8504d721e71aa722e3e6643161b1a1a8c289dd Mon Sep 17 00:00:00 2001 From: xj Date: Tue, 24 Feb 2026 20:46:16 -0800 Subject: [PATCH] test(security): add regression tests for exec channel policy and SSRF hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add security regression tests proving each hardened boundary: - exec tool: remote channel blocked, internal channel allowed, empty channel fail-closed, allowRemote bypass - web_fetch SSRF: IPv4-mapped IPv6, cloud metadata IP, IPv6 unique local, 6to4, Teredo, redirect-to-private, comprehensive IP classification table - Trim context analysis doc (248→42 lines), remove 552-line plan doc --- .docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md | 263 +-------- ...026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md | 552 ------------------ pkg/tools/shell_test.go | 78 +++ pkg/tools/web_test.go | 125 ++++ 4 files changed, 230 insertions(+), 788 deletions(-) delete mode 100644 docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md diff --git a/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md b/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md index 303ddee38..7fd07af5c 100644 --- a/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md +++ b/.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md @@ -1,248 +1,39 @@ # GHSA-pv8c-p6jf-3fpp Security Context Analysis -Date: 2026-02-25 -Repository: `sipeed/picoclaw` -Advisory: `GHSA-pv8c-p6jf-3fpp` (draft) +Advisory: `GHSA-pv8c-p6jf-3fpp` (draft, severity: critical) +Vulnerable range: `0.1.1` -## 1) Advisory Snapshot (What Is Known) +## Trust Boundaries -From `gh api repos/sipeed/picoclaw/security-advisories/GHSA-pv8c-p6jf-3fpp`: +1. **Channel ingress** — webhook/channel authentication. +2. **Agent tool boundary** — LLM can invoke `exec`, `cron`, `web_fetch`, etc. +3. **Network egress** — `web_fetch` SSRF exposure. +4. **Persistence** — cron jobs, session/state file permissions. -- 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. +## Attack Chains Addressed -Source: -- https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp +| Chain | Path | Mitigation | +|-------|------|------------| +| A | Ingress → agent → `exec` → host RCE | `exec` blocked for remote channels by default (`AllowRemote: false`). Fail-closed: empty channel = blocked. | +| B | Prompted SSRF via `web_fetch` | Private/loopback/link-local/metadata IP blocked. Safe dial context (connect-time DNS check). Redirect-to-private blocked. IPv6 vectors: unique local, 6to4, Teredo. | +| C | RCE → persistent cron commands | `cron` command scheduling restricted to internal channels + `command_confirm` friction. | +| D | File permission leakage | Session files: 0o600. State dir: 0o700, state files: 0o600. Atomic write via temp+rename. | -Important caveat: -- The advisory is still draft and does not yet publish patched versions, CVSS vectors, or a complete vulnerability decomposition. +## Residual Risk (Not In This PR) -## 2) Threat Actors And Trust Boundaries +- `install_skill` suspicious content is warning-only (follow-up: block by default). +- Hardware `confirm` flags are not auth boundaries. +- Channel auth coverage outside WeCom not fully audited. +- Prompt injection via fetched web content remains an open vector. -### Threat actors +## CWE Mapping -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). +- CWE-306: Missing auth on critical function +- CWE-78: OS command injection +- CWE-918: SSRF +- CWE-276: Incorrect file permissions -### Core trust boundaries +## References -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 +- [Advisory](https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp) +- [OWASP SSRF Prevention](https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html) 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 deleted file mode 100644 index 8b0b87a8e..000000000 --- a/docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md +++ /dev/null @@ -1,552 +0,0 @@ -# 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/tools/shell_test.go b/pkg/tools/shell_test.go index 6d35815e8..dd0f79cde 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" "time" + + "github.com/sipeed/picoclaw/pkg/config" ) // TestShellTool_Success verifies successful command execution @@ -246,6 +248,82 @@ func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { } } +// TestShellTool_RemoteChannelBlockedByDefault verifies exec is blocked for remote channels +func TestShellTool_RemoteChannelBlockedByDefault(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.AllowRemote = false + + tool := NewExecToolWithConfig("", false, cfg) + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo hi", + "__channel": "telegram", + "__chat_id": "chat-1", + }) + + if !result.IsError { + t.Fatal("expected remote-channel exec to be blocked") + } + if !strings.Contains(result.ForLLM, "restricted to internal channels") { + t.Errorf("expected 'restricted to internal channels' message, got: %s", result.ForLLM) + } +} + +// TestShellTool_InternalChannelAllowed verifies exec is allowed for internal channels +func TestShellTool_InternalChannelAllowed(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.AllowRemote = false + + tool := NewExecToolWithConfig("", false, cfg) + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo hi", + "__channel": "cli", + "__chat_id": "direct", + }) + + if result.IsError { + t.Fatalf("expected internal channel exec to succeed, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "hi") { + t.Errorf("expected output to contain 'hi', got: %s", result.ForLLM) + } +} + +// TestShellTool_EmptyChannelBlockedWhenNotAllowRemote verifies fail-closed when no channel context +func TestShellTool_EmptyChannelBlockedWhenNotAllowRemote(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.AllowRemote = false + + tool := NewExecToolWithConfig("", false, cfg) + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo hi", + }) + + if !result.IsError { + t.Fatal("expected exec with empty channel to be blocked when allowRemote=false") + } +} + +// TestShellTool_AllowRemoteBypassesChannelCheck verifies allowRemote=true permits any channel +func TestShellTool_AllowRemoteBypassesChannelCheck(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.AllowRemote = true + + tool := NewExecToolWithConfig("", false, cfg) + result := tool.Execute(context.Background(), map[string]any{ + "command": "echo hi", + "__channel": "telegram", + "__chat_id": "chat-1", + }) + + if result.IsError { + t.Fatalf("expected allowRemote=true to permit remote channel, got: %s", result.ForLLM) + } +} + // TestShellTool_RestrictToWorkspace verifies workspace restriction func TestShellTool_RestrictToWorkspace(t *testing.T) { tmpDir := t.TempDir() diff --git a/pkg/tools/web_test.go b/pkg/tools/web_test.go index d07bad050..a44c277e3 100644 --- a/pkg/tools/web_test.go +++ b/pkg/tools/web_test.go @@ -3,6 +3,7 @@ package tools import ( "context" "encoding/json" + "net" "net/http" "net/http/httptest" "strings" @@ -365,6 +366,130 @@ func TestWebTool_WebFetch_PrivateHostAllowedForTests(t *testing.T) { } } +// TestWebFetch_BlocksIPv4MappedIPv6Loopback verifies ::ffff:127.0.0.1 is blocked +func TestWebFetch_BlocksIPv4MappedIPv6Loopback(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://[::ffff:127.0.0.1]:0", + }) + + if !result.IsError { + t.Error("expected error for IPv4-mapped IPv6 loopback URL, got success") + } +} + +// TestWebFetch_BlocksMetadataIP verifies 169.254.169.254 is blocked +func TestWebFetch_BlocksMetadataIP(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://169.254.169.254/latest/meta-data", + }) + + if !result.IsError { + t.Error("expected error for cloud metadata IP, got success") + } +} + +// TestWebFetch_BlocksIPv6UniqueLocal verifies fc00::/7 addresses are blocked +func TestWebFetch_BlocksIPv6UniqueLocal(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://[fd00::1]:0", + }) + + if !result.IsError { + t.Error("expected error for IPv6 unique local address, got success") + } +} + +// TestWebFetch_Blocks6to4 verifies 2002::/16 addresses are blocked (may embed private IPv4) +func TestWebFetch_Blocks6to4(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://[2002:7f00:0001::1]:0", + }) + + if !result.IsError { + t.Error("expected error for 6to4 address, got success") + } +} + +// TestWebFetch_BlocksTeredo verifies 2001:0000::/32 addresses are blocked +func TestWebFetch_BlocksTeredo(t *testing.T) { + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": "http://[2001:0000::1]:0", + }) + + if !result.IsError { + t.Error("expected error for Teredo tunnel address, got success") + } +} + +// TestWebFetch_RedirectToPrivateBlocked verifies redirects to private IPs are blocked +func TestWebFetch_RedirectToPrivateBlocked(t *testing.T) { + withPrivateWebFetchHostsAllowed(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Redirect to a private IP + http.Redirect(w, r, "http://10.0.0.1/secret", http.StatusFound) + })) + defer server.Close() + + // Temporarily disable private host allowance for the redirect check + allowPrivateWebFetchHosts.Store(false) + defer allowPrivateWebFetchHosts.Store(true) + + tool := NewWebFetchTool(50000) + result := tool.Execute(context.Background(), map[string]any{ + "url": server.URL, + }) + + if !result.IsError { + t.Error("expected error when redirecting to private IP, got success") + } +} + +// TestIsPrivateOrRestrictedIP_Table tests IP classification logic +func TestIsPrivateOrRestrictedIP_Table(t *testing.T) { + tests := []struct { + ip string + blocked bool + desc string + }{ + {"127.0.0.1", true, "IPv4 loopback"}, + {"10.0.0.1", true, "IPv4 private class A"}, + {"172.16.0.1", true, "IPv4 private class B"}, + {"192.168.1.1", true, "IPv4 private class C"}, + {"169.254.169.254", true, "link-local / cloud metadata"}, + {"100.64.0.1", true, "carrier-grade NAT"}, + {"0.0.0.0", true, "unspecified"}, + {"8.8.8.8", false, "public DNS"}, + {"1.1.1.1", false, "public DNS"}, + {"::1", true, "IPv6 loopback"}, + {"::ffff:127.0.0.1", true, "IPv4-mapped IPv6 loopback"}, + {"::ffff:10.0.0.1", true, "IPv4-mapped IPv6 private"}, + {"fc00::1", true, "IPv6 unique local"}, + {"fd00::1", true, "IPv6 unique local"}, + {"2002:7f00:0001::1", true, "6to4 with embedded 127.x"}, + {"2001:0000::1", true, "Teredo"}, + {"2607:f8b0:4004:800::200e", false, "public IPv6 (Google)"}, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + ip := net.ParseIP(tt.ip) + if ip == nil { + t.Fatalf("failed to parse IP: %s", tt.ip) + } + got := isPrivateOrRestrictedIP(ip) + if got != tt.blocked { + t.Errorf("isPrivateOrRestrictedIP(%s) = %v, want %v", tt.ip, got, tt.blocked) + } + }) + } +} + // TestWebTool_WebFetch_MissingDomain verifies error handling for URL without domain func TestWebTool_WebFetch_MissingDomain(t *testing.T) { tool := NewWebFetchTool(50000)