Fix a security bug: block SSRF in WebFetchTool by validating resolved IP addresses

[chengpeng-wang]

Summary

WebFetchTool.Execute allowed fetching arbitrary URLs without validating the resolved IP address. This made it trivially possible to reach localhost services, private network hosts, and cloud metadata endpoints — a classic Server-Side Request Forgery (SSRF) vulnerability.

---

Problem

The original code only checked:

• URL scheme is http or https
• Host field is not empty

But it never checked what IP the hostname actually resolved to, meaning requests like the following were silently allowed:

http://127.0.0.1:8888/   → Jupyter Notebook (potential RCE)
http://192.168.1.1/      → Local router admin panel
http://10.0.0.x/         → Internal network services
http://169.254.169.254/  → Cloud instance metadata (AWS/GCP/Aliyun)

For users running picoclaw locally, this is especially relevant in a Prompt Injection + SSRF combined attack scenario:

1. Attacker embeds hidden instructions in a web page
2. User asks picoclaw to summarize that page
3. LLM gets hijacked and calls web_fetch against internal addresses
4. Internal data is exfiltrated to attacker-controlled server

---

Fix

Added blockPrivateTarget() which runs before the HTTP request is made:

func blockPrivateTarget(ctx context.Context, parsedURL *url.URL) error {
    hostname := parsedURL.Hostname()
    addrs, err := net.DefaultResolver.LookupHost(ctx, hostname)
    if err != nil {
        return fmt.Errorf("could not resolve host %q", hostname)
    }
    for _, addr := range addrs {
        ip := net.ParseIP(addr)
        if ip == nil {
            continue
        }
        if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() ||
            ip.IsLinkLocalMulticast() || ip.IsUnspecified() {
            return fmt.Errorf("requests to private/internal addresses are not allowed")
        }
    }
    return nil
}

This blocks:

• 127.0.0.0/8 — loopback
• 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 — private networks
• 169.254.0.0/16 — link-local / cloud metadata
• ::1 — IPv6 loopback
• fc00::/7, fe80::/10 — IPv6 private and link-local
• Unspecified addresses (0.0.0.0)

---

Security Impact

Attack Vector	Before	After
Direct private IP access	❌ Unblocked	✅ Blocked
Cloud metadata endpoints	❌ Unblocked	✅ Blocked
Hostname resolving to private IP	❌ Unblocked	✅ Blocked
IPv6 internal addresses	❌ Unblocked	✅ Blocked

[yinwm]

@chengpeng-wang please fix Linter and tests bug

[chengpeng-wang]

Five tests in pkg/tools/web_test.go were failing after SSRF protection (blockPrivateTarget) was introduced:

• TestWebTool_WebFetch_Success
• TestWebTool_WebFetch_JSON
• TestWebTool_WebFetch_Truncation
• TestWebFetchTool_PayloadTooLarge
• TestWebTool_WebFetch_HTMLExtraction

All five used httptest.NewServer, which binds to 127.0.0.1 (loopback). The SSRF check runs before any HTTP request is dispatched — it resolves the hostname and rejects any loopback or RFC 1918 address. Because Go's net.DefaultResolver returns IP literals directly (bypassing the Dial hook), the resolver cannot be overridden to work around this in tests.

Solution

Replaced httptest.NewServer with a mockRoundTripper that intercepts requests at the http.RoundTripper layer and returns a hand-crafted *http.Response — no real network call is made. Test URLs are switched to the RFC 5737 documentation range (192.0.2.0/24), which is neither loopback nor private, so the SSRF check passes cleanly.

192.0.2.0/24    — TEST-NET-1, reserved for documentation (RFC 5737)
198.51.100.0/24 — TEST-NET-2
203.0.113.0/24  — TEST-NET-3

Changes

File	Change
pkg/tools/web_test.go	Added mockRoundTripper + newMockFetchTool helper; rewrote 5 tests

No production code was modified.

[yinwm]

Code review

Found 4 issues:

1. TOCTOU race condition - DNS rebinding attack can bypass SSRF protection

The blockPrivateTarget function resolves DNS and checks IP addresses before making the HTTP request. However, between the DNS check and the actual HTTP request, an attacker could change the DNS response (DNS rebinding attack). The HTTP client will re-resolve the DNS when making the request, potentially getting a different private IP.

picoclaw/pkg/tools/web.go

Lines 586 to 604 in 094c6cf

	func blockPrivateTarget(ctx context.Context, parsedURL *url.URL) error {
	hostname := parsedURL.Hostname() // strips port and IPv6 brackets
	addrs, err := net.DefaultResolver.LookupHost(ctx, hostname)
	if err != nil {
	return fmt.Errorf("could not resolve host %q", hostname)
	}
	for _, addr := range addrs {
	ip := net.ParseIP(addr)
	if ip == nil {
	continue
	}
	if ip.IsLoopback() || ip.IsPrivate() ||
	ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() {
	return fmt.Errorf("requests to private/internal addresses are not allowed")
	}
	}
	return nil
	}

2. All existing tests will fail because they use localhost/127.0.0.1

The PR adds blockPrivateTarget() which blocks all requests to private/internal IP addresses including 127.0.0.1 (localhost). However, all existing tests in web_test.go use httptest.NewServer() which binds to 127.0.0.1 by default. This will cause all tests to fail. (Note: I see the PR has added mockRoundTripper to work around this, but need to verify all tests are properly migrated.)

picoclaw/pkg/tools/web.go

Lines 596 to 600 in 094c6cf

	}
	if ip.IsLoopback() || ip.IsPrivate() ||
	ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() {
	return fmt.Errorf("requests to private/internal addresses are not allowed")
	}

3. Missing unit tests for the new blockPrivateTarget function

The PR adds a new security-critical function blockPrivateTarget to prevent SSRF attacks, but there are no dedicated unit tests to verify: various private IP ranges are correctly blocked, public IPs are allowed, IPv4 and IPv6 are correctly handled, DNS resolution failures are handled properly.

picoclaw/pkg/tools/web.go

Lines 586 to 604 in 094c6cf

	func blockPrivateTarget(ctx context.Context, parsedURL *url.URL) error {
	hostname := parsedURL.Hostname() // strips port and IPv6 brackets
	addrs, err := net.DefaultResolver.LookupHost(ctx, hostname)
	if err != nil {
	return fmt.Errorf("could not resolve host %q", hostname)
	}
	for _, addr := range addrs {
	ip := net.ParseIP(addr)
	if ip == nil {
	continue
	}
	if ip.IsLoopback() || ip.IsPrivate() ||
	ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() {
	return fmt.Errorf("requests to private/internal addresses are not allowed")
	}
	}
	return nil
	}

4. DNS resolution failure blocks all legitimate requests

When DNS resolution fails (e.g., temporary network issues), the function returns an error and blocks the request. This means temporary DNS issues would prevent users from accessing any website, even public ones. Consider differentiating between "DNS failed" vs "resolved to private IP".

picoclaw/pkg/tools/web.go

Lines 589 to 591 in 094c6cf

	if err != nil {
	return fmt.Errorf("could not resolve host %q", hostname)
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[chengpeng-wang]

Issue 1 — TOCTOU / DNS Rebinding Attack

Root cause: blockPrivateTarget resolved DNS and checked IPs as a pre-flight step, but http.Transport performed its own independent DNS resolution when actually dialing. An attacker could swap the DNS record in the window between the check and the connection.

Fix: Added newSafeDialer (web.go:612), which merges DNS resolution, IP validation, and TCP dialing into a single atomic step. It resolves the hostname once, rejects private/internal IPs, then connects directly to the resolved IP — bypassing any re-resolution at dial time. The dialer is wired into WebFetchTool's http.Transport.DialContext when no proxy is configured (web.go:550-555).

---

Issue 2 — Tests Failing Due to Localhost

Already resolved in a prior change via mockRoundTripper + RFC 5737 documentation IPs (192.0.2.0/24). No further changes needed.

---

Issue 3 — Missing Unit Tests for blockPrivateTarget

Fix: Added two dedicated test functions in web_test.go:

• TestBlockPrivateTarget — 10 sub-cases covering every category of blocked address (IPv4 loopback, RFC 1918 ranges, link-local, unspecified, IPv6 loopback, IPv6 link-local) and two allowed public IPs (8.8.8.8, 192.0.2.1). Uses IP literals so no real DNS lookup is required.
• TestSafeDialer — Exercises newSafeDialer directly: verifies private addresses are rejected with an SSRF error before any connection attempt, and confirms that a public RFC 5737 address passes the check (failing at the network level, not with an SSRF error).

---

Issue 4 — DNS Failure Indistinguishable from SSRF Block

Fix: Both blockPrivateTarget (web.go:593) and newSafeDialer (web.go:612) now return a wrapped error with a distinct prefix:

Situation	Error message
DNS lookup failed	DNS resolution failed for host "example.com": <underlying error>
IP is private/internal	requests to private/internal addresses are not allowed

Callers and users can now distinguish a transient network problem from a deliberate SSRF block.

[CLAassistant]

All committers have signed the CLA.

[chengpeng-wang]

@yinwm I have signed the CLA. Could you please review the change and approve the workflow to run the test?

[sipeed-bot]

@chengpeng-wang Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue.