From eee0bc24936ba531137885336f3f08a6e1e2ffd4 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Thu, 16 Jul 2026 20:44:35 +0400 Subject: [PATCH] fix(openclaw): yaml-escape Ollama model names in overlay generation The #775 injection fix wrapped imported values with yamlScalar() but missed the two sites that interpolate names sourced from listOllamaModels() (which JSON-decodes the local Ollama HTTP endpoint with no charset validation): generateOverlayValues's model-list block and patchOverlayModelList's overlay-update path both spliced the raw model id/display-name into hand-built YAML lines. A model name containing a newline plus a YAML key (e.g. "x\nimage:\n repository: evil") could inject new top-level keys into values-obol.yaml, later applied to the cluster via helmfile sync. Route both id and name through the existing yamlScalar() encoder at both call sites. Added regression tests mirroring the #775 TestGenerateOverlayValues_RejectsYAMLInjection pattern for each site, plus an injection subtest under TestPatchOverlayModelList; both fail on the pre-fix code and pass after. Updated the three pre-existing assertions that expected unquoted 'id: ' output, since yamlScalar always double-quotes. Claude-Session: https://claude.ai/code/session_014YjPMViNrZ7zBVgUQzwEKk --- internal/openclaw/openclaw.go | 6 +-- internal/openclaw/overlay_test.go | 85 +++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/internal/openclaw/openclaw.go b/internal/openclaw/openclaw.go index 1f7bf166..1bb2a718 100644 --- a/internal/openclaw/openclaw.go +++ b/internal/openclaw/openclaw.go @@ -1922,8 +1922,8 @@ func patchOverlayModelList(content string, models []string) (string, bool) { } else { result = append(result, indent+"models:") for _, m := range models { - result = append(result, indent+" - id: "+m) - result = append(result, indent+" name: "+ollamaModelDisplayName(m)) + result = append(result, indent+" - id: "+yamlScalar(m)) + result = append(result, indent+" name: "+yamlScalar(ollamaModelDisplayName(m))) } } @@ -2070,7 +2070,7 @@ models: b.WriteString(" models:\n") for _, m := range ollamaModels { - fmt.Fprintf(&b, " - id: %s\n name: %s\n", m, ollamaModelDisplayName(m)) + fmt.Fprintf(&b, " - id: %s\n name: %s\n", yamlScalar(m), yamlScalar(ollamaModelDisplayName(m))) } } else { b.WriteString(" models: []\n") diff --git a/internal/openclaw/overlay_test.go b/internal/openclaw/overlay_test.go index 79159780..49243083 100644 --- a/internal/openclaw/overlay_test.go +++ b/internal/openclaw/overlay_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/ObolNetwork/obol-stack/internal/config" + "gopkg.in/yaml.v3" ) // testConfig creates a temp config dir with a .stack-id file for testing. @@ -167,15 +168,53 @@ func TestGenerateOverlayValues_OllamaDefaultWithModels(t *testing.T) { t.Errorf("default overlay missing LiteLLM baseUrl, got:\n%s", yaml) } - if !strings.Contains(yaml, "id: llama3.2:3b") { + if !strings.Contains(yaml, `id: "llama3.2:3b"`) { t.Errorf("default overlay missing first model, got:\n%s", yaml) } - if !strings.Contains(yaml, "id: mistral:7b") { + if !strings.Contains(yaml, `id: "mistral:7b"`) { t.Errorf("default overlay missing second model, got:\n%s", yaml) } } +// TestGenerateOverlayValues_OllamaModelNameInjection is a #775-style +// regression test for a listOllamaModels() name (or its derived display +// name) breaking out of the hand-formatted "- id: %s\n name: %s\n" lines +// and injecting new YAML keys into the overlay applied via `helmfile sync`. +func TestGenerateOverlayValues_OllamaModelNameInjection(t *testing.T) { + malicious := "x\nimage:\n repository: pwned-by-ollama\nrbac:\n create: false" + + rendered := generateOverlayValues(testConfig(t), "openclaw-default.obol.stack", nil, false, []string{malicious}, "") + + 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) + } + + 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) + } + } + + modelsSection, _ := doc["models"].(map[string]any) + openai, _ := modelsSection["openai"].(map[string]any) + + modelList, _ := openai["models"].([]any) + if len(modelList) != 1 { + t.Fatalf("expected 1 model entry, got %d\nrendered:\n%s", len(modelList), rendered) + } + + entry, _ := modelList[0].(map[string]any) + if entry["id"] != malicious { + t.Errorf("id = %#v, want single scalar %q (no injected keys); rendered:\n%s", entry["id"], malicious, rendered) + } + + if entry["name"] != ollamaModelDisplayName(malicious) { + t.Errorf("name = %#v, want single scalar %q; rendered:\n%s", entry["name"], ollamaModelDisplayName(malicious), rendered) + } +} + func TestGenerateOverlayValues_OllamaDefaultNoModels(t *testing.T) { // When no Ollama models are available, overlay should have empty model list yaml := generateOverlayValues(testConfig(t), "openclaw-default.obol.stack", nil, false, nil, "") @@ -380,11 +419,11 @@ erpc: t.Fatal("expected change") } - if !strings.Contains(updated, "id: claude-sonnet-4-5-20250929") { + if !strings.Contains(updated, `id: "claude-sonnet-4-5-20250929"`) { t.Errorf("missing claude model in updated overlay:\n%s", updated) } - if !strings.Contains(updated, "id: gpt-4o") { + if !strings.Contains(updated, `id: "gpt-4o"`) { t.Errorf("missing gpt model in updated overlay:\n%s", updated) } // eRPC section should still be present @@ -427,10 +466,46 @@ erpc: t.Fatal("expected change") } - if !strings.Contains(updated, "id: llama3.2:3b") { + if !strings.Contains(updated, `id: "llama3.2:3b"`) { t.Errorf("missing model in updated overlay:\n%s", updated) } }) + + // #775-style regression: a malicious/malformed Ollama model name must not + // break out of the " - id: "+m hand-formatted line and inject new keys + // into the overlay applied via `helmfile sync`. + t.Run("model name injection", func(t *testing.T) { + malicious := "x\nimage:\n repository: pwned-by-ollama\nrbac:\n create: false" + + updated, changed := patchOverlayModelList(overlay, []string{malicious}) + if !changed { + t.Fatal("expected change") + } + + var doc map[string]any + if err := yaml.Unmarshal([]byte(updated), &doc); err != nil { + t.Fatalf("patched overlay is not valid YAML: %v\n%s", err, updated) + } + + if img, ok := doc["image"].(map[string]any); ok { + if _, hasRepo := img["repository"]; hasRepo { + t.Errorf("attacker injected an 'image.repository' key: %#v\nupdated:\n%s", img, updated) + } + } + + modelsSection, _ := doc["models"].(map[string]any) + openai, _ := modelsSection["openai"].(map[string]any) + + modelList, _ := openai["models"].([]any) + if len(modelList) != 1 { + t.Fatalf("expected 1 model entry, got %d\nupdated:\n%s", len(modelList), updated) + } + + entry, _ := modelList[0].(map[string]any) + if entry["id"] != malicious { + t.Errorf("id = %#v, want single scalar %q (no injected keys); updated:\n%s", entry["id"], malicious, updated) + } + }) } func TestPatchAgentModelsJSON(t *testing.T) {