From 176b043b0104cee3f894023cd5a598ac29e404bb Mon Sep 17 00:00:00 2001 From: Lohit Date: Wed, 27 May 2026 04:21:18 +0530 Subject: [PATCH] rewrite: prevent placeholder re-expansion in injected query (#7761) When the rewrite URI template ends with a literal '?' and contains a placeholder that expands to client-controlled bytes (e.g. {http.request.header.X-Fwd}), those bytes flow into buildQueryString which runs a second Replacer pass. If the bytes contain placeholder syntax such as {env.SECRET}, that placeholder is evaluated, allowing disclosure of environment variables, files (via {file./path}), or internal request vars through the rewritten request URI. Escape '{' and '}' in the injected query before assigning it to the query variable, so the second pass cannot find any placeholder syntax to evaluate. Operator-written placeholders in the rewrite template are already expanded by the first pass on the path component, so the only '{' or '}' surviving into the injected query must have come from replacement values. Fixes GHSA-j8px-rmrx-76h9. Includes three regression tests mirroring the 'is not re-expanded' tests in modules/caddyhttp/vars_test.go. Co-authored-by: Matt Holt --- modules/caddyhttp/rewrite/rewrite.go | 9 ++++++ modules/caddyhttp/rewrite/rewrite_test.go | 36 +++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go index 3500028f9..02ef524df 100644 --- a/modules/caddyhttp/rewrite/rewrite.go +++ b/modules/caddyhttp/rewrite/rewrite.go @@ -223,6 +223,15 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool { newPath, injectedQuery = before, after // don't overwrite explicitly-configured query string if query == "" { + // the injected query came from the first-pass placeholder + // expansion above, which means any '{' or '}' bytes in it + // must have come from replacement values (e.g. a request + // header), not from operator-written placeholder syntax. + // escape them so buildQueryString does not re-expand them, + // which would allow attacker input like {env.SECRET} to be + // evaluated (see GHSA-j8px-rmrx-76h9). + injectedQuery = strings.ReplaceAll(injectedQuery, "{", "%7B") + injectedQuery = strings.ReplaceAll(injectedQuery, "}", "%7D") query = injectedQuery } } diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go index 602e31084..e52a33256 100644 --- a/modules/caddyhttp/rewrite/rewrite_test.go +++ b/modules/caddyhttp/rewrite/rewrite_test.go @@ -18,6 +18,7 @@ import ( "net/http" "reflect" "regexp" + "strings" "testing" "github.com/caddyserver/caddy/v2" @@ -350,6 +351,32 @@ func TestRewrite(t *testing.T) { input: newRequest(t, "GET", "/foo//bar///baz?a=b//c"), expect: newRequest(t, "GET", "/foo/bar/baz?a=b//c"), }, + + // regression tests for GHSA-j8px-rmrx-76h9: when the rewrite URI + // ends with a literal '?', the first-pass placeholder expansion + // may produce a path containing attacker-controlled bytes that + // then get split at '?' and fed into buildQueryString, which runs + // a SECOND placeholder pass. Bytes injected via a header value (or + // any other client-controlled placeholder) must not be treated as + // placeholder syntax during this second pass. + { + // literal header value containing placeholder syntax is not re-expanded into query + rule: Rewrite{URI: "/serve/{http.request.header.X-Fwd}?"}, + input: newRequestWithHeader(t, "GET", "/anything", "X-Fwd", "foo?{env.CADDY_REWRITE_TEST_SECRET}=leak"), + expect: newRequest(t, "GET", "/serve/foo?%7Benv.CADDY_REWRITE_TEST_SECRET%7D=leak"), + }, + { + // literal header value with placeholder syntax in query position is not re-expanded + rule: Rewrite{URI: "/serve/{http.request.header.X-Fwd}?"}, + input: newRequestWithHeader(t, "GET", "/anything", "X-Fwd", "ok?key={env.CADDY_REWRITE_TEST_SECRET}"), + expect: newRequest(t, "GET", "/serve/ok?key=%7Benv.CADDY_REWRITE_TEST_SECRET%7D"), + }, + { + // literal header value with embedded file placeholder is not re-expanded + rule: Rewrite{URI: "/serve/{http.request.header.X-Fwd}?"}, + input: newRequestWithHeader(t, "GET", "/anything", "X-Fwd", "ok?path={file./etc/passwd}"), + expect: newRequest(t, "GET", "/serve/ok?path=%7Bfile./etc/passwd%7D"), + }, } { // copy the original input just enough so that we can // compare it after the rewrite to see if it changed @@ -364,6 +391,9 @@ func TestRewrite(t *testing.T) { repl.Set("http.request.uri", tc.input.RequestURI) repl.Set("http.request.uri.path", tc.input.URL.Path) repl.Set("http.request.uri.query", tc.input.URL.RawQuery) + for field, vals := range tc.input.Header { + repl.Set("http.request.header."+field, strings.Join(vals, ",")) + } // we can't directly call Provision() without a valid caddy.Context // (TODO: fix that) so here we ad-hoc compile the regex @@ -456,6 +486,12 @@ func newRequest(t *testing.T, method, uri string) *http.Request { return req } +func newRequestWithHeader(t *testing.T, method, uri, headerKey, headerVal string) *http.Request { + req := newRequest(t, method, uri) + req.Header.Set(headerKey, headerVal) + return req +} + // reqEqual if r1 and r2 are equal enough for our purposes. func reqEqual(r1, r2 *http.Request) bool { if r1.Method != r2.Method {