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

fix(security): harden unauthenticated tool-exec paths

Browse source
This commit is contained in:
xj 2026-02-24 19:59:11 -08:00
parent ec6da7a530
commit e009611f88
11 changed files with 1062 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

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

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

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

10
pkg/agent/loop.go
View file

@ -747,6 +747,16 @@ func (al *AgentLoop) updateToolContexts(agent *AgentInstance, channel, chatID st
st.SetContext(channel, chatID) 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. // maybeSummarize triggers summarization if the session history exceeds thresholds.

1
pkg/config/config.go
View file

@ -464,6 +464,7 @@ type CronToolsConfig struct {
type ExecConfig struct { type ExecConfig struct {
EnableDenyPatterns bool `json:"enable_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` 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"` CustomDenyPatterns []string `json:"custom_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"`
} }

1
pkg/config/defaults.go
View file

@ -297,6 +297,7 @@ func DefaultConfig() *Config {
}, },
Exec: ExecConfig{ Exec: ExecConfig{
EnableDenyPatterns: true, EnableDenyPatterns: true,
AllowRemote: false,
}, },
Skills: SkillsToolsConfig{ Skills: SkillsToolsConfig{
Registries: SkillsRegistriesConfig{ Registries: SkillsRegistriesConfig{

4
pkg/session/manager.go
View file

@ -32,7 +32,7 @@ func NewSessionManager(storage string) *SessionManager {
} }
if storage != "" { if storage != "" {
os.MkdirAll(storage, 0o755) os.MkdirAll(storage, 0o700)
sm.loadSessions() sm.loadSessions()
} }
@ -214,7 +214,7 @@ func (sm *SessionManager) Save(key string) error {
_ = tmpFile.Close() _ = tmpFile.Close()
return err return err
} }
if err := tmpFile.Chmod(0o644); err != nil { if err := tmpFile.Chmod(0o600); err != nil {
_ = tmpFile.Close() _ = tmpFile.Close()
return err return err
} }

11
pkg/state/state.go
View file

@ -38,7 +38,9 @@ func NewManager(workspace string) *Manager {
oldStateFile := filepath.Join(workspace, "state.json") oldStateFile := filepath.Join(workspace, "state.json")
// Create state directory if it doesn't exist // 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{ sm := &Manager{
workspace: workspace, workspace: workspace,
@ -139,7 +141,7 @@ func (sm *Manager) saveAtomic() error {
} }
// Write to temp file // 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) 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) 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 return nil
} }

17
pkg/tools/cron.go
View file

@ -8,6 +8,7 @@ import (
"github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/bus"
"github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/config"
"github.com/sipeed/picoclaw/pkg/constants"
"github.com/sipeed/picoclaw/pkg/cron" "github.com/sipeed/picoclaw/pkg/cron"
"github.com/sipeed/picoclaw/pkg/utils" "github.com/sipeed/picoclaw/pkg/utils"
) )
@ -72,6 +73,10 @@ func (t *CronTool) Parameters() map[string]any {
"type": "string", "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.", "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{ "at_seconds": map[string]any{
"type": "integer", "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'.", "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) command, _ := args["command"].(string)
commandConfirm, _ := args["command_confirm"].(bool)
if command != "" { 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) // 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 // 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. // 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 // Execute command if present
if job.Payload.Command != "" { if job.Payload.Command != "" {
args := map[string]any{ args := map[string]any{
"command": job.Payload.Command, "command": job.Payload.Command,
"__channel": channel,
"__chat_id": chatID,
} }
result := t.execTool.Execute(ctx, args) result := t.execTool.Execute(ctx, args)

19
pkg/tools/shell.go
View file

@ -14,6 +14,7 @@ import (
"time" "time"
"github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/config"
"github.com/sipeed/picoclaw/pkg/constants"
) )
type ExecTool struct { type ExecTool struct {
@ -22,6 +23,9 @@ type ExecTool struct {
denyPatterns []*regexp.Regexp denyPatterns []*regexp.Regexp
allowPatterns []*regexp.Regexp allowPatterns []*regexp.Regexp
restrictToWorkspace bool restrictToWorkspace bool
allowRemote bool
channel string
chatID string
} }
var defaultDenyPatterns = []*regexp.Regexp{ 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 { func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) *ExecTool {
denyPatterns := make([]*regexp.Regexp, 0) denyPatterns := make([]*regexp.Regexp, 0)
allowRemote := true
enableDenyPatterns := true enableDenyPatterns := true
if config != nil { if config != nil {
execConfig := config.Tools.Exec execConfig := config.Tools.Exec
enableDenyPatterns = execConfig.EnableDenyPatterns enableDenyPatterns = execConfig.EnableDenyPatterns
allowRemote = execConfig.AllowRemote
if enableDenyPatterns { if enableDenyPatterns {
denyPatterns = append(denyPatterns, defaultDenyPatterns...) denyPatterns = append(denyPatterns, defaultDenyPatterns...)
if len(execConfig.CustomDenyPatterns) > 0 { if len(execConfig.CustomDenyPatterns) > 0 {
@ -107,6 +113,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf
denyPatterns: denyPatterns, denyPatterns: denyPatterns,
allowPatterns: nil, allowPatterns: nil,
restrictToWorkspace: restrict, 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") 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 cwd := t.workingDir
if wd, ok := args["working_dir"].(string); ok && wd != "" { if wd, ok := args["working_dir"].(string); ok && wd != "" {
if t.restrictToWorkspace && t.workingDir != "" { if t.restrictToWorkspace && t.workingDir != "" {
@ -331,3 +345,8 @@ func (t *ExecTool) SetAllowPatterns(patterns []string) error {
} }
return nil return nil
} }
func (t *ExecTool) SetContext(channel, chatID string) {
t.channel = channel
t.chatID = chatID
}

161
pkg/tools/web.go
View file

@ -6,6 +6,7 @@ import (
"encoding/json" "encoding/json"
"fmt" "fmt"
"io" "io"
"net"
"net/http" "net/http"
"net/url" "net/url"
"regexp" "regexp"
@ -495,6 +496,10 @@ type WebFetchTool struct {
proxy string 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 { func NewWebFetchTool(maxChars int) *WebFetchTool {
if maxChars <= 0 { if maxChars <= 0 {
maxChars = 50000 maxChars = 50000
@ -559,6 +564,10 @@ func (t *WebFetchTool) Execute(ctx context.Context, args map[string]any) *ToolRe
return ErrorResult("missing domain in URL") return ErrorResult("missing domain in URL")
} }
if isPrivateFetchHost(parsedURL.Hostname()) {
return ErrorResult("fetching private or local network hosts is not allowed")
}
maxChars := t.maxChars maxChars := t.maxChars
if mc, ok := args["maxChars"].(float64); ok { if mc, ok := args["maxChars"].(float64); ok {
if int(mc) > 100 { 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) req.Header.Set("User-Agent", userAgent)
client, err := createHTTPClient(t.proxy, 60*time.Second) // Build transport with SSRF-safe dial context.
if err != nil { dialer := &net.Dialer{
return ErrorResult(fmt.Sprintf("failed to create HTTP client: %v", err)) 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 // Preserve proxy support from upstream.
client.CheckRedirect = func(req *http.Request, via []*http.Request) error { if t.proxy != "" {
if len(via) >= 5 { proxy, err := url.Parse(t.proxy)
return fmt.Errorf("stopped after 5 redirects") 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) resp, err := client.Do(req)
@ -674,3 +708,114 @@ func (t *WebFetchTool) extractText(htmlContent string) string {
return strings.Join(cleanLines, "\n") 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
}

51
pkg/tools/web_test.go
View file

@ -12,6 +12,8 @@ import (
// TestWebTool_WebFetch_Success verifies successful URL fetching // TestWebTool_WebFetch_Success verifies successful URL fetching
func TestWebTool_WebFetch_Success(t *testing.T) { func TestWebTool_WebFetch_Success(t *testing.T) {
withPrivateWebFetchHostsAllowed(t)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/html") w.Header().Set("Content-Type", "text/html")
w.WriteHeader(http.StatusOK) w.WriteHeader(http.StatusOK)
@ -45,6 +47,8 @@ func TestWebTool_WebFetch_Success(t *testing.T) {
// TestWebTool_WebFetch_JSON verifies JSON content handling // TestWebTool_WebFetch_JSON verifies JSON content handling
func TestWebTool_WebFetch_JSON(t *testing.T) { func TestWebTool_WebFetch_JSON(t *testing.T) {
withPrivateWebFetchHostsAllowed(t)
testData := map[string]string{"key": "value", "number": "123"} testData := map[string]string{"key": "value", "number": "123"}
expectedJSON, _ := json.MarshalIndent(testData, "", " ") expectedJSON, _ := json.MarshalIndent(testData, "", " ")
@ -137,6 +141,8 @@ func TestWebTool_WebFetch_MissingURL(t *testing.T) {
// TestWebTool_WebFetch_Truncation verifies content truncation // TestWebTool_WebFetch_Truncation verifies content truncation
func TestWebTool_WebFetch_Truncation(t *testing.T) { func TestWebTool_WebFetch_Truncation(t *testing.T) {
withPrivateWebFetchHostsAllowed(t)
longContent := strings.Repeat("x", 20000) longContent := strings.Repeat("x", 20000)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 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 // TestWebTool_WebFetch_HTMLExtraction verifies HTML text extraction
func TestWebTool_WebFetch_HTMLExtraction(t *testing.T) { func TestWebTool_WebFetch_HTMLExtraction(t *testing.T) {
withPrivateWebFetchHostsAllowed(t)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "text/html") w.Header().Set("Content-Type", "text/html")
w.WriteHeader(http.StatusOK) 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 // 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)