Fix security vulnerabilities from review (CRIT-1 through MED-6)

CRIT-1: Remove hardcoded Google OAuth client secret; credentials are now
read from GOOGLE_CLIENT_ID / GOOGLE_CLIENT_SECRET env vars. Removed the
decodeBase64 helper that existed only to obfuscate the embedded secret.

CRIT-2 + MED-3: Add RequireLocalOrigin middleware to the launcher API
server. All /api/ and /auth/ routes now reject requests whose Origin
header is present but doesn't match http://localhost:18800 or
http://127.0.0.1:18800, blocking browser-based CSRF attacks.

CRIT-3 + HIGH-5: Split shell deny patterns into absoluteDenyPatterns
(always enforced, cannot be overridden) and defaultDenyPatterns
(overridable by customAllowPatterns). Added missing patterns for nc/ncat,
python -c, perl -e, ruby -e, nohup, crontab, authorized_keys writes,
shell startup file writes, and base64-piped execution. |/bin/sh and
|/bin/bash are now also covered.

HIGH-1: Replace cmd /c start <url> with rundll32 url.dll,FileProtocolHandler
on Windows in OpenBrowser to prevent cmd.exe shell metacharacter injection.
Added URL scheme validation before opening any browser URL.

HIGH-2: Apply io.LimitReader(resp.Body, 1<<20) to all io.ReadAll calls
on OAuth HTTP response bodies (RequestDeviceCode, LoginDeviceCode,
pollDeviceCode, RefreshAccessToken, ExchangeCodeForTokens,
fetchGoogleUserEmail) to prevent OOM from malicious token endpoints.

HIGH-3: Add maxOAuthSessions = 50 cap; handleGoogleAntigravityLogin now
returns 429 when the limit is reached, preventing unbounded map growth.

HIGH-4: Escape all attacker-controlled strings written into HTML responses
in handleOAuthCallback using html.EscapeString, fixing reflected XSS via
the ?error= query parameter.

HIGH-6: Replace math/rand with crypto/rand in randomString() in
antigravity_provider.go using crypto/rand.Int + math/big.Int.

MED-1: Add validGitHubRepo regex validation in InstallFromGitHub to
reject repo names containing path traversal, query strings, fragments,
or characters outside [A-Za-z0-9_.-].

MED-2: Add rejectSSRFTarget() in WebFetchTool that resolves the hostname
and rejects loopback, link-local, private, and cloud metadata (169.254.x)
addresses, preventing prompt-injected SSRF to the launcher API or cloud
instance metadata endpoints.

MED-6: Canonicalize path with filepath.Clean before running pattern
matching in whitelistFs.matches() to prevent regex bypass via embedded
traversal sequences like /allowed/path/../../../etc/shadow.

https://claude.ai/code/session_01AerxPcDs78ntotTwbyYiVd
This commit is contained in:
Claude 2026-03-05 00:24:50 +00:00
parent cbd5c4c45d
commit 9540e63fa5
No known key found for this signature in database
9 changed files with 209 additions and 51 deletions

View file

@ -3,6 +3,7 @@ package server
import (
"encoding/json"
"fmt"
"html"
"io"
"log"
"net/http"
@ -13,6 +14,11 @@ import (
"github.com/sipeed/picoclaw/pkg/providers"
)
// maxOAuthSessions limits the number of concurrent in-flight OAuth sessions to
// prevent an unauthenticated caller from growing the in-memory session map without
// bound (HIGH-3: DoS via unlimited session creation).
const maxOAuthSessions = 50
// oauthSession stores in-flight OAuth state for browser-based flows.
type oauthSession struct {
Provider string
@ -186,8 +192,13 @@ func handleGoogleAntigravityLogin(w http.ResponseWriter, r *http.Request, config
authURL := auth.BuildAuthorizeURL(oauthCfg, pkce, state, redirectURI)
// Store session for callback
// Enforce session count limit before storing the new session.
oauthSessionsMu.Lock()
if len(oauthSessions) >= maxOAuthSessions {
oauthSessionsMu.Unlock()
http.Error(w, "Too many pending OAuth sessions; please try again later", http.StatusTooManyRequests)
return
}
oauthSessions[state] = &oauthSession{
Provider: "google-antigravity",
PKCE: pkce,
@ -232,8 +243,10 @@ func handleOAuthCallback(w http.ResponseWriter, r *http.Request) {
}
if code == "" {
errMsg := r.URL.Query().Get("error")
w.Header().Set("Content-Type", "text/html")
// Escape the attacker-controlled query parameter before writing it into HTML
// to prevent reflected XSS (HIGH-4).
errMsg := html.EscapeString(r.URL.Query().Get("error"))
w.Header().Set("Content-Type", "text/html; charset=utf-8")
fmt.Fprintf(
w,
`<html><body><h2>Authentication failed</h2><p>%s</p><p>You can close this window.</p></body></html>`,
@ -244,11 +257,11 @@ func handleOAuthCallback(w http.ResponseWriter, r *http.Request) {
cred, err := auth.ExchangeCodeForTokens(session.OAuthCfg, code, session.PKCE.CodeVerifier, session.RedirectURI)
if err != nil {
w.Header().Set("Content-Type", "text/html")
w.Header().Set("Content-Type", "text/html; charset=utf-8")
fmt.Fprintf(
w,
`<html><body><h2>Authentication failed</h2><p>%s</p><p>You can close this window.</p></body></html>`,
err.Error(),
html.EscapeString(err.Error()),
)
return
}
@ -266,8 +279,8 @@ func handleOAuthCallback(w http.ResponseWriter, r *http.Request) {
}
if err := auth.SetCredential(session.Provider, cred); err != nil {
w.Header().Set("Content-Type", "text/html")
fmt.Fprintf(w, `<html><body><h2>Failed to save credentials</h2><p>%s</p></body></html>`, err.Error())
w.Header().Set("Content-Type", "text/html; charset=utf-8")
fmt.Fprintf(w, `<html><body><h2>Failed to save credentials</h2><p>%s</p></body></html>`, html.EscapeString(err.Error()))
return
}
@ -297,7 +310,7 @@ func fetchGoogleUserEmail(accessToken string) (string, error) {
}
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
if resp.StatusCode != http.StatusOK {
return "", fmt.Errorf("userinfo request failed: %s", string(body))
}

View file

@ -6,6 +6,7 @@ import (
"io"
"log"
"net/http"
"strings"
"time"
"github.com/sipeed/picoclaw/pkg/auth"
@ -14,6 +15,47 @@ import (
const DefaultPort = "18800"
// allowedOrigins are the origins from which browser requests to the API are accepted.
// Any request whose Origin header is set but doesn't match one of these is rejected.
var allowedOrigins = []string{
"http://localhost:" + DefaultPort,
"http://127.0.0.1:" + DefaultPort,
}
// RequireLocalOrigin is HTTP middleware that rejects cross-origin requests to
// the launcher API. It blocks browser-based CSRF attacks (any browser tab can
// attempt a cross-origin fetch, but the browser always sends the real Origin
// header which we validate here). Requests without an Origin header (direct
// curl/tool access from localhost) are permitted.
func RequireLocalOrigin(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
origin := r.Header.Get("Origin")
if origin != "" {
allowed := false
for _, o := range allowedOrigins {
if strings.EqualFold(origin, o) {
allowed = true
break
}
}
if !allowed {
http.Error(w, "Forbidden: cross-origin request rejected", http.StatusForbidden)
return
}
}
// Set CORS headers so the UI (same origin) works correctly and other
// origins are explicitly denied by the browser.
w.Header().Set("Access-Control-Allow-Origin", "http://localhost:"+DefaultPort)
w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, OPTIONS")
w.Header().Set("Access-Control-Allow-Headers", "Content-Type")
if r.Method == http.MethodOptions {
w.WriteHeader(http.StatusNoContent)
return
}
next.ServeHTTP(w, r)
})
}
// providerStatus represents the auth status of a single provider in API responses.
type providerStatus struct {
Provider string `json:"provider"`

View file

@ -67,15 +67,20 @@ func main() {
addr = "127.0.0.1:" + server.DefaultPort
}
mux := http.NewServeMux()
server.RegisterConfigAPI(mux, absPath)
server.RegisterAuthAPI(mux, absPath)
server.RegisterProcessAPI(mux, absPath)
apiMux := http.NewServeMux()
server.RegisterConfigAPI(apiMux, absPath)
server.RegisterAuthAPI(apiMux, absPath)
server.RegisterProcessAPI(apiMux, absPath)
staticFS, err := fs.Sub(staticFiles, "internal/ui")
if err != nil {
log.Fatalf("Failed to create sub filesystem: %v", err)
}
mux := http.NewServeMux()
// All /api/ and /auth/ routes are protected by the local-origin CSRF guard.
mux.Handle("/api/", server.RequireLocalOrigin(apiMux))
mux.Handle("/auth/", server.RequireLocalOrigin(apiMux))
mux.Handle("/", http.FileServer(http.FS(staticFS)))
// Print startup banner

View file

@ -41,13 +41,18 @@ func OpenAIOAuthConfig() OAuthProviderConfig {
}
// GoogleAntigravityOAuthConfig returns the OAuth configuration for Google Cloud Code Assist (Antigravity).
// Client credentials are the same ones used by OpenCode/pi-ai for Cloud Code Assist access.
// Credentials are read from GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET environment variables.
// You must register your own OAuth 2.0 credentials at https://console.cloud.google.com/apis/credentials
// and set these environment variables before using Google authentication.
func GoogleAntigravityOAuthConfig() OAuthProviderConfig {
// These are the same client credentials used by the OpenCode antigravity plugin.
clientID := decodeBase64(
"MTA3MTAwNjA2MDU5MS10bWhzc2luMmgyMWxjcmUyMzV2dG9sb2poNGc0MDNlcC5hcHBzLmdvb2dsZXVzZXJjb250ZW50LmNvbQ==",
)
clientSecret := decodeBase64("R09DU1BYLUs1OEZXUjQ4NkxkTEoxbUxCOHNYQzR6NnFEQWY=")
clientID := os.Getenv("GOOGLE_CLIENT_ID")
clientSecret := os.Getenv("GOOGLE_CLIENT_SECRET")
if clientID == "" || clientSecret == "" {
// Warn loudly — shared credentials should not be used in production.
fmt.Fprintln(os.Stderr, "WARNING: GOOGLE_CLIENT_ID and/or GOOGLE_CLIENT_SECRET are not set.")
fmt.Fprintln(os.Stderr, " Google OAuth will not function without valid credentials.")
fmt.Fprintln(os.Stderr, " Register your own at https://console.cloud.google.com/apis/credentials")
}
return OAuthProviderConfig{
Issuer: "https://accounts.google.com/o/oauth2/v2",
TokenURL: "https://oauth2.googleapis.com/token",
@ -58,14 +63,6 @@ func GoogleAntigravityOAuthConfig() OAuthProviderConfig {
}
}
func decodeBase64(s string) string {
data, err := base64.StdEncoding.DecodeString(s)
if err != nil {
return s
}
return string(data)
}
// GenerateState generates a random state string for OAuth CSRF protection.
func GenerateState() (string, error) {
buf := make([]byte, 32)
@ -212,7 +209,7 @@ func RequestDeviceCode(cfg OAuthProviderConfig) (*DeviceCodeInfo, error) {
}
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("device code request failed: %s", string(body))
}
@ -300,7 +297,7 @@ func LoginDeviceCode(cfg OAuthProviderConfig) (*AuthCredential, error) {
}
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("device code request failed: %s", string(body))
}
@ -360,7 +357,7 @@ func pollDeviceCode(cfg OAuthProviderConfig, deviceAuthID, userCode string) (*Au
return nil, fmt.Errorf("pending")
}
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
var tokenResp struct {
AuthorizationCode string `json:"authorization_code"`
@ -401,7 +398,7 @@ func RefreshAccessToken(cred *AuthCredential, cfg OAuthProviderConfig) (*AuthCre
}
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("token refresh failed: %s", string(body))
}
@ -494,7 +491,7 @@ func ExchangeCodeForTokens(cfg OAuthProviderConfig, code, codeVerifier, redirect
}
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("token exchange failed: %s", string(body))
}
@ -608,14 +605,25 @@ func base64URLDecode(s string) ([]byte, error) {
}
// OpenBrowser opens the given URL in the user's default browser.
func OpenBrowser(url string) error {
// Each platform call passes the URL as a discrete argument to avoid shell interpretation.
func OpenBrowser(rawURL string) error {
// Validate the URL before passing it to a browser launcher.
parsed, err := url.Parse(rawURL)
if err != nil {
return fmt.Errorf("invalid URL: %w", err)
}
if parsed.Scheme != "http" && parsed.Scheme != "https" {
return fmt.Errorf("refusing to open non-http(s) URL: %s", parsed.Scheme)
}
switch runtime.GOOS {
case "darwin":
return exec.Command("open", url).Start()
return exec.Command("open", rawURL).Start()
case "linux":
return exec.Command("xdg-open", url).Start()
return exec.Command("xdg-open", rawURL).Start()
case "windows":
return exec.Command("cmd", "/c", "start", url).Start()
// Use rundll32 instead of "cmd /c start" to avoid cmd.exe shell metacharacter
// interpretation (e.g., '&', '|', '^' in URLs would execute commands via cmd.exe).
return exec.Command("rundll32", "url.dll,FileProtocolHandler", rawURL).Start()
default:
return fmt.Errorf("unsupported platform: %s", runtime.GOOS)
}

View file

@ -4,10 +4,11 @@ import (
"bufio"
"bytes"
"context"
cryptorand "crypto/rand"
"encoding/json"
"fmt"
"io"
"math/rand"
"math/big"
"net/http"
"strings"
"time"
@ -758,8 +759,17 @@ func truncateString(s string, maxLen int) string {
func randomString(n int) string {
const letters = "abcdefghijklmnopqrstuvwxyz0123456789"
b := make([]byte, n)
max := big.NewInt(int64(len(letters)))
for i := range b {
b[i] = letters[rand.Intn(len(letters))]
idx, err := cryptorand.Int(cryptorand.Reader, max)
if err != nil {
// Fall back to a fixed character rather than panicking; callers use
// this only for non-secret request IDs, so degraded randomness is
// acceptable in the unlikely event crypto/rand fails.
b[i] = letters[0]
continue
}
b[i] = letters[idx.Int64()]
}
return string(b)
}

View file

@ -7,12 +7,18 @@ import (
"net/http"
"os"
"path/filepath"
"regexp"
"time"
"github.com/sipeed/picoclaw/pkg/fileutil"
"github.com/sipeed/picoclaw/pkg/utils"
)
// validGitHubRepo matches "owner/repo" where owner and repo contain only
// alphanumeric characters, hyphens, underscores, and dots — no path traversal,
// query strings, fragments, or null bytes.
var validGitHubRepo = regexp.MustCompile(`^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`)
type SkillInstaller struct {
workspace string
}
@ -24,6 +30,9 @@ func NewSkillInstaller(workspace string) *SkillInstaller {
}
func (si *SkillInstaller) InstallFromGitHub(ctx context.Context, repo string) error {
if !validGitHubRepo.MatchString(repo) {
return fmt.Errorf("invalid GitHub repo name %q: must be in 'owner/repo' format using only alphanumeric characters, hyphens, underscores, and dots", repo)
}
skillDir := filepath.Join(si.workspace, "skills", filepath.Base(repo))
if _, err := os.Stat(skillDir); err == nil {

View file

@ -398,8 +398,12 @@ type whitelistFs struct {
}
func (w *whitelistFs) matches(path string) bool {
// Canonicalize the path before pattern matching to prevent bypass via
// embedded traversal sequences (e.g. /allowed/path/../../../etc/shadow
// would match the "/allowed/path" prefix but resolve elsewhere).
canonical := filepath.Clean(path)
for _, p := range w.patterns {
if p.MatchString(path) {
if p.MatchString(canonical) {
return true
}
}

View file

@ -26,35 +26,48 @@ type ExecTool struct {
}
var (
defaultDenyPatterns = []*regexp.Regexp{
// absoluteDenyPatterns are always blocked regardless of customAllowPatterns.
// These cover the most dangerous command patterns that should never be executed.
absoluteDenyPatterns = []*regexp.Regexp{
regexp.MustCompile(`\brm\s+-[rf]{1,2}\b`),
regexp.MustCompile(`\bdel\s+/[fq]\b`),
regexp.MustCompile(`\brmdir\s+/s\b`),
// Match disk wiping commands (must be followed by space/args)
regexp.MustCompile(
`\b(format|mkfs|diskpart)\b\s`,
),
regexp.MustCompile(`\b(format|mkfs|diskpart)\b\s`),
regexp.MustCompile(`\bdd\s+if=`),
// Block writes to block devices (all common naming schemes).
regexp.MustCompile(
`>\s*/dev/(sd[a-z]|hd[a-z]|vd[a-z]|xvd[a-z]|nvme\d|mmcblk\d|loop\d|dm-\d|md\d|sr\d|nbd\d)`,
),
regexp.MustCompile(`\b(shutdown|reboot|poweroff)\b`),
regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`),
regexp.MustCompile(`\$\([^)]+\)`),
regexp.MustCompile(`\$\{[^}]+\}`),
regexp.MustCompile("`[^`]+`"),
regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`), // fork bomb
regexp.MustCompile(`\$\([^)]+\)`), // command substitution $()
regexp.MustCompile(`\$\{[^}]+\}`), // variable substitution ${}
regexp.MustCompile("`[^`]+`"), // backtick substitution
regexp.MustCompile(`\|\s*sh\b`),
regexp.MustCompile(`\|\s*bash\b`),
regexp.MustCompile(`\|\s*(\/bin\/)?sh\b`),
regexp.MustCompile(`\|\s*(\/bin\/)?bash\b`),
regexp.MustCompile(`\bbase64\b.*\|\s*(sh|bash|(\/bin\/)?(sh|bash))\b`),
regexp.MustCompile(`;\s*rm\s+-[rf]`),
regexp.MustCompile(`&&\s*rm\s+-[rf]`),
regexp.MustCompile(`\|\|\s*rm\s+-[rf]`),
regexp.MustCompile(`\bsudo\b`),
regexp.MustCompile(`\beval\b`),
// Persist via cron/at/startup files
regexp.MustCompile(`\bcrontab\b`),
regexp.MustCompile(`\bat\s+now\b`),
regexp.MustCompile(`\.ssh[/\\]authorized_keys\b`),
regexp.MustCompile(`\.(bashrc|bash_profile|profile|zshrc|zprofile)\b`),
// Network exfiltration
regexp.MustCompile(`\b(nc|ncat|netcat)\b`),
}
defaultDenyPatterns = []*regexp.Regexp{
// Additional patterns that can be overridden by customAllowPatterns
regexp.MustCompile(`<<\s*EOF`),
regexp.MustCompile(`\$\(\s*cat\s+`),
regexp.MustCompile(`\$\(\s*curl\s+`),
regexp.MustCompile(`\$\(\s*wget\s+`),
regexp.MustCompile(`\$\(\s*which\s+`),
regexp.MustCompile(`\bsudo\b`),
regexp.MustCompile(`\bchmod\s+[0-7]{3,4}\b`),
regexp.MustCompile(`\bchown\b`),
regexp.MustCompile(`\bpkill\b`),
@ -72,8 +85,12 @@ var (
regexp.MustCompile(`\bgit\s+push\b`),
regexp.MustCompile(`\bgit\s+force\b`),
regexp.MustCompile(`\bssh\b.*@`),
regexp.MustCompile(`\beval\b`),
regexp.MustCompile(`\bsource\s+.*\.sh\b`),
// Script interpreters running inline code
regexp.MustCompile(`\bpython[23]?\s+-c\b`),
regexp.MustCompile(`\bperl\s+-e\b`),
regexp.MustCompile(`\bruby\s+-e\b`),
regexp.MustCompile(`\bnohup\b`),
}
// absolutePathPattern matches absolute file paths in commands (Unix and Windows).
@ -291,7 +308,14 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
cmd := strings.TrimSpace(command)
lower := strings.ToLower(cmd)
// Custom allow patterns exempt a command from deny checks.
// Absolute deny patterns are always enforced — customAllowPatterns cannot bypass them.
for _, pattern := range absoluteDenyPatterns {
if pattern.MatchString(lower) {
return "Command blocked by safety guard (dangerous pattern detected)"
}
}
// Custom allow patterns can exempt a command from the configurable deny checks below.
explicitlyAllowed := false
for _, pattern := range t.customAllowPatterns {
if pattern.MatchString(lower) {

View file

@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"io"
"net"
"net/http"
"net/url"
"regexp"
@ -666,6 +667,44 @@ func NewWebFetchToolWithProxy(maxChars int, proxy string, fetchLimitBytes int64)
}, nil
}
// rejectSSRFTarget returns an error if the URL targets an address that should
// not be reachable from an agent tool: loopback, link-local, private ranges,
// and cloud metadata endpoints. This prevents prompt-injected instructions from
// exfiltrating data via the local launcher API or cloud instance metadata.
func rejectSSRFTarget(u *url.URL) error {
hostname := u.Hostname()
// Resolve the hostname to catch DNS-based SSRF.
// Use the literal hostname for IP checks before DNS resolution too.
ips, err := net.LookupHost(hostname)
if err != nil {
// If we can't resolve it, let the HTTP client fail naturally.
// The important case is when we CAN resolve it and it's private.
ips = []string{hostname}
}
for _, ipStr := range ips {
ip := net.ParseIP(ipStr)
if ip == nil {
continue
}
if ip.IsLoopback() {
return fmt.Errorf("SSRF protection: requests to loopback addresses are not allowed")
}
if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() {
return fmt.Errorf("SSRF protection: requests to link-local addresses are not allowed")
}
if ip.IsPrivate() {
return fmt.Errorf("SSRF protection: requests to private network addresses are not allowed")
}
// Block cloud metadata endpoints (AWS, GCP, Azure all use 169.254.169.254).
if ip.Equal(net.ParseIP("169.254.169.254")) {
return fmt.Errorf("SSRF protection: requests to cloud metadata endpoints are not allowed")
}
}
return nil
}
func (t *WebFetchTool) Name() string {
return "web_fetch"
}
@ -711,6 +750,10 @@ func (t *WebFetchTool) Execute(ctx context.Context, args map[string]any) *ToolRe
return ErrorResult("missing domain in URL")
}
if err := rejectSSRFTarget(parsedURL); err != nil {
return ErrorResult(err.Error())
}
maxChars := t.maxChars
if mc, ok := args["maxChars"].(float64); ok {
if int(mc) > 100 {