From 93fac4b48e3e3b51c84daf0156120064aef0cea0 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Thu, 16 Jul 2026 17:10:30 +0400 Subject: [PATCH] fix(openclaw): escape untrusted import fields before writing overlay YAML Root cause: generateOverlayValues/TranslateToOverlayYAML hand-formatted fields imported from ~/.openclaw/openclaw.json (agentModel, provider name/baseUrl/apiKeyEnvVar, model id/name) straight into values-obol.yaml via fmt.Fprintf("...: %s\n", untrusted). A value like x\nimage:\n repository: evil breaks out of its field, injects new top-level YAML keys, and survives the line-based openclaw:/models: block stripper (the raw newline flips a line from "skipped" back to "kept" mid-block). Since values-obol.yaml is applied to the cluster via helmfile sync, this is arbitrary Helm value injection, including overriding the container image for RCE. Fix: add yamlScalar(), a small helper that renders a Go string as a forced double-quoted YAML scalar via gopkg.in/yaml.v3 (already a dependency), and route every untrusted Fprintf site in generateOverlayValues/TranslateToOverlayYAML through it. Quoting is applied at the source (import.go) so the downstream block-stripping in generateOverlayValues never sees a raw injected newline in the first place. Internally-derived values (litellm master key, chart image tag, hostname, agentBaseURL, whitelisted provider API type) are left as-is. Confirmed Canary402 full-surface audit finding. Verified the added regression test fails on the pre-fix code (injected image/rbac keys appear as live top-level YAML) and passes after the fix. Claude-Session: https://claude.ai/code/session_014YjPMViNrZ7zBVgUQzwEKk --- internal/openclaw/import.go | 33 +++++++++-- internal/openclaw/import_test.go | 96 ++++++++++++++++++++++++++++--- internal/openclaw/openclaw.go | 2 +- internal/openclaw/overlay_test.go | 16 +++--- 4 files changed, 123 insertions(+), 24 deletions(-) diff --git a/internal/openclaw/import.go b/internal/openclaw/import.go index f08e9966..e7a93d8c 100644 --- a/internal/openclaw/import.go +++ b/internal/openclaw/import.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/ObolNetwork/obol-stack/internal/model" + "gopkg.in/yaml.v3" ) // API key environment variable names for known providers. @@ -217,14 +218,14 @@ func TranslateToOverlayYAML(result *ImportResult) string { var b strings.Builder if result.AgentModel != "" { - fmt.Fprintf(&b, "openclaw:\n agentModel: %s\n\n", result.AgentModel) + fmt.Fprintf(&b, "openclaw:\n agentModel: %s\n\n", yamlScalar(result.AgentModel)) } if len(result.Providers) > 0 { b.WriteString("models:\n") for _, p := range result.Providers { - fmt.Fprintf(&b, " %s:\n", p.Name) + fmt.Fprintf(&b, " %s:\n", yamlScalar(p.Name)) if p.Disabled { b.WriteString(" enabled: false\n") @@ -234,7 +235,7 @@ func TranslateToOverlayYAML(result *ImportResult) string { b.WriteString(" enabled: true\n") if p.BaseURL != "" { - fmt.Fprintf(&b, " baseUrl: %s\n", p.BaseURL) + fmt.Fprintf(&b, " baseUrl: %s\n", yamlScalar(p.BaseURL)) } // Always emit api to override any stale base chart value. // Empty string makes the Helm template omit it from JSON, @@ -246,17 +247,17 @@ func TranslateToOverlayYAML(result *ImportResult) string { } if p.APIKeyEnvVar != "" { - fmt.Fprintf(&b, " apiKeyEnvVar: %s\n", p.APIKeyEnvVar) + fmt.Fprintf(&b, " apiKeyEnvVar: %s\n", yamlScalar(p.APIKeyEnvVar)) } if len(p.Models) > 0 { b.WriteString(" models:\n") for _, m := range p.Models { - fmt.Fprintf(&b, " - id: %s\n", m.ID) + fmt.Fprintf(&b, " - id: %s\n", yamlScalar(m.ID)) if m.Name != "" { - fmt.Fprintf(&b, " name: %s\n", m.Name) + fmt.Fprintf(&b, " name: %s\n", yamlScalar(m.Name)) } } } @@ -457,3 +458,23 @@ func extractEnvVarName(s string) (string, bool) { func isEnvVarRef(s string) bool { return strings.Contains(s, "${") } + +// yamlScalar renders s as a double-quoted YAML scalar so it can be safely +// interpolated into the hand-built overlay YAML. Values sourced from an +// imported ~/.openclaw/openclaw.json are untrusted: without this, an +// embedded newline (e.g. "x\nimage:\n repository: evil") would let the +// string break out of its field and inject new keys into values-obol.yaml, +// which is later applied to the cluster via `helmfile sync`. +func yamlScalar(s string) string { + node := yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: s, Style: yaml.DoubleQuotedStyle} + + out, err := yaml.Marshal(&node) + if err != nil { + // Unreachable for a plain string scalar; fall back to a manual + // double-quote/escape rather than emit the untrusted value raw. + r := strings.NewReplacer(`\`, `\\`, `"`, `\"`, "\n", `\n`, "\r", `\r`, "\t", `\t`) + return `"` + r.Replace(s) + `"` + } + + return strings.TrimSuffix(string(out), "\n") +} diff --git a/internal/openclaw/import_test.go b/internal/openclaw/import_test.go index 78fbb897..dedc4d27 100644 --- a/internal/openclaw/import_test.go +++ b/internal/openclaw/import_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "testing" + + "gopkg.in/yaml.v3" ) func TestIsEnvVarRef(t *testing.T) { @@ -226,7 +228,7 @@ func TestTranslateToOverlayYAML_AgentModelOnly(t *testing.T) { } got := TranslateToOverlayYAML(result) - if !strings.Contains(got, "agentModel: claude-sonnet-4-6") { + if !strings.Contains(got, `agentModel: "claude-sonnet-4-6"`) { t.Errorf("YAML missing agentModel, got:\n%s", got) } @@ -252,11 +254,11 @@ func TestTranslateToOverlayYAML_ProviderWithModels(t *testing.T) { got := TranslateToOverlayYAML(result) checks := []string{ - "anthropic:\n enabled: true", - "baseUrl: https://api.anthropic.com", + "\"anthropic\":\n enabled: true", + `baseUrl: "https://api.anthropic.com"`, "api: anthropic-messages", - "- id: claude-sonnet-4-6", - "name: Claude Sonnet 4.6", + `- id: "claude-sonnet-4-6"`, + `name: "Claude Sonnet 4.6"`, } for _, check := range checks { if !strings.Contains(got, check) { @@ -273,7 +275,7 @@ func TestTranslateToOverlayYAML_DisabledProvider(t *testing.T) { } got := TranslateToOverlayYAML(result) - if !strings.Contains(got, "openai:\n enabled: false") { + if !strings.Contains(got, "\"openai\":\n enabled: false") { t.Errorf("YAML missing disabled openai, got:\n%s", got) } @@ -346,15 +348,15 @@ func TestTranslateToOverlayYAML_FullConfig(t *testing.T) { } got := TranslateToOverlayYAML(result) - if !strings.Contains(got, "agentModel: claude-sonnet-4-6") { + if !strings.Contains(got, `agentModel: "claude-sonnet-4-6"`) { t.Errorf("YAML missing agentModel, got:\n%s", got) } - if !strings.Contains(got, "anthropic:\n enabled: true") { + if !strings.Contains(got, "\"anthropic\":\n enabled: true") { t.Errorf("YAML missing enabled anthropic, got:\n%s", got) } - if !strings.Contains(got, "openai:\n enabled: false") { + if !strings.Contains(got, "\"openai\":\n enabled: false") { t.Errorf("YAML missing disabled openai, got:\n%s", got) } @@ -617,3 +619,79 @@ func TestDetectExistingConfigAt_EmptyConfig(t *testing.T) { t.Errorf("AgentModel = %q, want empty", result.AgentModel) } } + +// TestYAMLScalar_EscapesInjection is a Canary402 full-surface audit +// regression test: values imported from ~/.openclaw/openclaw.json must never +// be able to inject additional YAML keys (e.g. overriding `image:` to +// achieve RCE via `helmfile sync`) when hand-formatted into the overlay +// values file. +func TestYAMLScalar_EscapesInjection(t *testing.T) { + tests := []struct { + name string + in string + }{ + {"plain", "openai/claude-sonnet-4-6"}, + {"newline_key_injection", "x\nimage:\n repository: evil"}, + {"colon", "has: colon"}, + {"quotes_and_backslash", `back\slash"quote`}, + {"empty", ""}, + {"trailing_newline_injection", "gpt-5.2\nrbac:\n create: false"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + line := "key: " + yamlScalar(tt.in) + "\n" + + var doc map[string]any + if err := yaml.Unmarshal([]byte(line), &doc); err != nil { + t.Fatalf("yamlScalar(%q) produced invalid YAML %q: %v", tt.in, line, err) + } + + if got, ok := doc["key"].(string); !ok || got != tt.in { + t.Errorf("round-trip = %#v, want single scalar %q (no injected keys); rendered: %q", doc["key"], tt.in, line) + } + + if len(doc) != 1 { + t.Errorf("doc has %d top-level keys, want 1 (injection leaked extra keys): %#v", len(doc), doc) + } + }) + } +} + +// TestGenerateOverlayValues_RejectsYAMLInjection feeds generateOverlayValues +// a malicious imported agentModel and provider name (newline + "image:") +// and asserts the rendered values-obol.yaml parses to the SAME single +// scalar values, with no injected top-level "image" key — i.e. the fix for +// the confirmed Canary402 full-surface audit finding holds end-to-end. +func TestGenerateOverlayValues_RejectsYAMLInjection(t *testing.T) { + maliciousModel := "x\nimage:\n repository: pwned-by-canary402\nrbac:\n create: false" + maliciousProvider := "ollama\nimage:\n repository: pwned-by-canary402" + + imported := &ImportResult{ + AgentModel: maliciousModel, + Providers: []ImportedProvider{ + {Name: maliciousProvider, BaseURL: "http://localhost:11434"}, + }, + } + + rendered := generateOverlayValues(testConfig(t), "openclaw-default.obol.stack", imported, false, nil, "") + + var doc map[string]any + if err := yaml.Unmarshal([]byte(rendered), &doc); err != nil { + t.Fatalf("rendered overlay is not valid YAML: %v\n%s", err, rendered) + } + + // The chart-default image override is legitimate and expected to stay a + // map with only "tag" — injection would add a "repository" key to it. + if img, ok := doc["image"].(map[string]any); ok { + if _, hasRepo := img["repository"]; hasRepo { + t.Errorf("attacker injected an 'image.repository' key: %#v\nrendered:\n%s", img, rendered) + } + } + + openclawSection, _ := doc["openclaw"].(map[string]any) + if openclawSection == nil || openclawSection["agentModel"] != "openai/"+maliciousModel { + t.Errorf("agentModel = %#v, want single scalar %q (rewritten with openai/ prefix); rendered:\n%s", + openclawSection["agentModel"], "openai/"+maliciousModel, rendered) + } +} diff --git a/internal/openclaw/openclaw.go b/internal/openclaw/openclaw.go index 3fba946c..3391568a 100644 --- a/internal/openclaw/openclaw.go +++ b/internal/openclaw/openclaw.go @@ -2035,7 +2035,7 @@ rbac: b.WriteString("openclaw:\n") if agentModel != "" { - fmt.Fprintf(&b, " agentModel: %s\n", agentModel) + fmt.Fprintf(&b, " agentModel: %s\n", yamlScalar(agentModel)) } b.WriteString(` gateway: diff --git a/internal/openclaw/overlay_test.go b/internal/openclaw/overlay_test.go index 18b1ed55..79159780 100644 --- a/internal/openclaw/overlay_test.go +++ b/internal/openclaw/overlay_test.go @@ -111,21 +111,21 @@ func TestOverlayYAML_LiteLLMRouted(t *testing.T) { yaml := TranslateToOverlayYAML(result) // Agent model should have ollama/ prefix - if !strings.Contains(yaml, "agentModel: openai/claude-sonnet-4-5-20250929") { + if !strings.Contains(yaml, `agentModel: "openai/claude-sonnet-4-5-20250929"`) { t.Errorf("YAML missing agentModel, got:\n%s", yaml) } // openai should be enabled with LiteLLM baseUrl - if !strings.Contains(yaml, "openai:\n enabled: true") { + if !strings.Contains(yaml, "\"openai\":\n enabled: true") { t.Errorf("YAML missing enabled openai provider, got:\n%s", yaml) } - if !strings.Contains(yaml, "baseUrl: http://litellm.llm.svc.cluster.local:4000/v1") { + if !strings.Contains(yaml, `baseUrl: "http://litellm.llm.svc.cluster.local:4000/v1"`) { t.Errorf("YAML missing LiteLLM baseUrl, got:\n%s", yaml) } // apiKeyEnvVar should be OPENAI_API_KEY (LiteLLM master key injected via this env var) - if !strings.Contains(yaml, "apiKeyEnvVar: OPENAI_API_KEY") { + if !strings.Contains(yaml, `apiKeyEnvVar: "OPENAI_API_KEY"`) { t.Errorf("YAML missing apiKeyEnvVar, got:\n%s", yaml) } @@ -140,16 +140,16 @@ func TestOverlayYAML_LiteLLMRouted(t *testing.T) { } // Cloud model should appear in ollama's model list - if !strings.Contains(yaml, "- id: claude-sonnet-4-5-20250929") { + if !strings.Contains(yaml, `- id: "claude-sonnet-4-5-20250929"`) { t.Errorf("YAML missing cloud model ID, got:\n%s", yaml) } // anthropic and ollama should be disabled - if !strings.Contains(yaml, "anthropic:\n enabled: false") { + if !strings.Contains(yaml, "\"anthropic\":\n enabled: false") { t.Errorf("YAML missing disabled anthropic, got:\n%s", yaml) } - if !strings.Contains(yaml, "ollama:\n enabled: false") { + if !strings.Contains(yaml, "\"ollama\":\n enabled: false") { t.Errorf("YAML missing disabled ollama, got:\n%s", yaml) } } @@ -159,7 +159,7 @@ func TestGenerateOverlayValues_OllamaDefaultWithModels(t *testing.T) { models := []string{"llama3.2:3b", "mistral:7b"} yaml := generateOverlayValues(testConfig(t), "openclaw-default.obol.stack", nil, false, models, "") - if !strings.Contains(yaml, "agentModel: openai/llama3.2:3b") { + if !strings.Contains(yaml, `agentModel: "openai/llama3.2:3b"`) { t.Errorf("default overlay missing ollama agentModel, got:\n%s", yaml) }