opencode/picoclaw
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

test(security): add regression tests for exec channel policy and SSRF hardening

Browse source 
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
This commit is contained in:
xj 2026-02-24 20:46:16 -08:00
parent 441824a0db
commit be8504d721
4 changed files with 230 additions and 788 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

263
.docs/GHSA-pv8c-p6jf-3fpp-context-analysis.md
View file

@ -1,248 +1,39 @@
# GHSA-pv8c-p6jf-3fpp Security Context Analysis # GHSA-pv8c-p6jf-3fpp Security Context Analysis
Date: 2026-02-25 Advisory: `GHSA-pv8c-p6jf-3fpp` (draft, severity: critical)
Repository: `sipeed/picoclaw` Vulnerable range: `0.1.1`
Advisory: `GHSA-pv8c-p6jf-3fpp` (draft)
## 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` ## Attack Chains Addressed
- 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: | Chain | Path | Mitigation |
- https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp |-------|------|------------|
| 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: ## Residual Risk (Not In This PR)
- 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 - `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. - CWE-306: Missing auth on critical function
2. Authenticated but malicious user abusing tool-capable agent behavior. - CWE-78: OS command injection
3. Adversarial remote content source (web pages consumed via `web_fetch`). - CWE-918: SSRF
4. Supply-chain adversary through registry content (`install_skill`). - CWE-276: Incorrect file permissions
5. Local host user/process with filesystem access (lower priority in this GHSA).
### Core trust boundaries ## References
1. Channel/webhook ingress authentication. - [Advisory](https://github.com/sipeed/picoclaw/security/advisories/GHSA-pv8c-p6jf-3fpp)
2. Agent decision boundary (LLM can invoke privileged tools). - [OWASP SSRF Prevention](https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html)
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

552
docs/plans/2026-02-25-ghsa-pv8c-p6jf-3fpp-hardening.md
View file

@ -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)

78
pkg/tools/shell_test.go
View file

@ -7,6 +7,8 @@ import (
"strings" "strings"
"testing" "testing"
"time" "time"
"github.com/sipeed/picoclaw/pkg/config"
) )
// TestShellTool_Success verifies successful command execution // 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 // TestShellTool_RestrictToWorkspace verifies workspace restriction
func TestShellTool_RestrictToWorkspace(t *testing.T) { func TestShellTool_RestrictToWorkspace(t *testing.T) {
tmpDir := t.TempDir() tmpDir := t.TempDir()

125
pkg/tools/web_test.go
View file

@ -3,6 +3,7 @@ package tools
import ( import (
"context" "context"
"encoding/json" "encoding/json"
"net"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"strings" "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 // TestWebTool_WebFetch_MissingDomain verifies error handling for URL without domain
func TestWebTool_WebFetch_MissingDomain(t *testing.T) { func TestWebTool_WebFetch_MissingDomain(t *testing.T) {
tool := NewWebFetchTool(50000) tool := NewWebFetchTool(50000)