diff --git a/coderd/workspaceapps/proxy.go b/coderd/workspaceapps/proxy.go index 9859171462..bb35843a06 100644 --- a/coderd/workspaceapps/proxy.go +++ b/coderd/workspaceapps/proxy.go @@ -166,6 +166,18 @@ func (s *Server) Attach(r chi.Router) { r.Get("/api/v2/workspaceagents/{workspaceagent}/pty", s.workspaceAgentPTY) } +// originLocalURL returns p as a relative URL rooted at the current origin, +// safe to use as a redirect Location. p (typically r.URL.Path) is +// attacker-controlled and already percent-decoded, so a leading "//" or "/\" +// run would otherwise be parsed by http.Redirect or a browser as a +// scheme-relative URL pointing at another host, turning a redirect back to the +// same path into an open redirect. Collapsing the leading slash and backslash +// run to a single "/" keeps the result same-origin, and url.URL.String() +// percent-encodes any control characters in the path. +func originLocalURL(p string) *url.URL { + return &url.URL{Path: "/" + strings.TrimLeft(p, `/\`)} +} + // handleAPIKeySmuggling is called by the proxy path and subdomain handlers to // process any "smuggled" API keys in the query parameters. // @@ -266,19 +278,17 @@ func (s *Server) handleAPIKeySmuggling(rw http.ResponseWriter, r *http.Request, HttpOnly: true, })) - // Strip the query parameter. - path := r.URL.Path - if path == "" { - path = "/" - } + // Strip the smuggled API key query parameter and redirect back to the same + // path. r.URL.Path is attacker-controlled and can smuggle a separate host + // (e.g. "//evil.com"); originLocalURL keeps the redirect on the current + // origin. + redirectURL := originLocalURL(r.URL.Path) + q := r.URL.Query() q.Del(SubdomainProxyAPIKeyParam) - rawQuery := q.Encode() - if rawQuery != "" { - path += "?" + q.Encode() - } + redirectURL.RawQuery = q.Encode() - http.Redirect(rw, r, path, http.StatusSeeOther) + http.Redirect(rw, r, redirectURL.String(), http.StatusSeeOther) return false } @@ -616,7 +626,14 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT // Web applications typically request paths relative to the // root URL. This allows for routing behind a proxy or subpath. // See https://github.com/coder/code-server/issues/241 for examples. - http.Redirect(rw, r, r.URL.Path+"/", http.StatusTemporaryRedirect) + // + // r.URL.Path is attacker-controlled, so sanitize it before redirecting + // to avoid an off-origin "//host" Location (see originLocalURL). + redirectURL := originLocalURL(r.URL.Path) + if !strings.HasSuffix(redirectURL.Path, "/") { + redirectURL.Path += "/" + } + http.Redirect(rw, r, redirectURL.String(), http.StatusTemporaryRedirect) return } if path == "/" && r.URL.RawQuery == "" && appURL.RawQuery != "" { @@ -625,8 +642,13 @@ func (s *Server) proxyWorkspaceApp(rw http.ResponseWriter, r *http.Request, appT // query parameters for server-side requests, but sometimes // client-side applications require the query parameters to render // properly. With code-server, this is the "folder" param. - r.URL.RawQuery = appURL.RawQuery - http.Redirect(rw, r, r.URL.String(), http.StatusTemporaryRedirect) + // + // r.URL.Path is attacker-controlled, so build the Location from a + // sanitized same-origin path instead of r.URL directly (see + // originLocalURL). + redirectURL := originLocalURL(r.URL.Path) + redirectURL.RawQuery = appURL.RawQuery + http.Redirect(rw, r, redirectURL.String(), http.StatusTemporaryRedirect) return } diff --git a/coderd/workspaceapps/proxy_internal_test.go b/coderd/workspaceapps/proxy_internal_test.go new file mode 100644 index 0000000000..cde66026f3 --- /dev/null +++ b/coderd/workspaceapps/proxy_internal_test.go @@ -0,0 +1,117 @@ +package workspaceapps + +import ( + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// Test_originLocalURL checks that originLocalURL produces a redirect target that +// stays on the current origin. +func Test_originLocalURL(t *testing.T) { + t.Parallel() + + t.Run("RejectsOffOrigin", func(t *testing.T) { + t.Parallel() + + // Each path models an already-percent-decoded r.URL.Path that tries to + // smuggle a separate host into the redirect. + cases := []struct { + name string + path string + }{ + {name: "DoubleSlash", path: "//evil.com/phish"}, + {name: "TripleSlash", path: "///evil.com/phish"}, + {name: "SlashBackslash", path: "/\\evil.com/phish"}, + {name: "SlashBackslashSlash", path: "/\\/evil.com/phish"}, + {name: "DoubleBackslash", path: "\\\\evil.com/phish"}, + {name: "SlashTab", path: "/\t/evil.com/phish"}, + {name: "SlashTabBackslash", path: "/\t\\evil.com/phish"}, + {name: "SlashNewline", path: "/\n/evil.com/phish"}, + {name: "SlashCarriageReturn", path: "/\r/evil.com/phish"}, + {name: "SlashTabDoubleSlash", path: "/\t//evil.com/phish"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + loc := originLocalURL(tc.path).String() + + // The Location must parse as a relative, same-origin reference. + require.Falsef(t, strings.HasPrefix(loc, "//"), + "path %q produced scheme-relative Location %q", tc.path, loc) + parsed, err := url.Parse(loc) + require.NoErrorf(t, err, "path %q produced unparseable Location %q", tc.path, loc) + require.Emptyf(t, parsed.Scheme, "path %q produced Location %q with a scheme", tc.path, loc) + require.Emptyf(t, parsed.Host, "path %q produced Location %q with a host", tc.path, loc) + + // It must also be free of raw bytes a browser would normalize back + // into an authority before resolving (a backslash becomes "/", and + // tab/newline/CR are stripped, either of which could re-form + // "//host"). url.URL.String() guarantees this by percent-encoding + // them; we assert it here rather than reproducing browser + // normalization in the code. + for _, raw := range []string{`\`, "\t", "\n", "\r"} { + require.NotContainsf(t, loc, raw, + "path %q produced Location %q containing a raw %q", tc.path, loc, raw) + } + }) + } + }) + + t.Run("EscapesControlCharacters", func(t *testing.T) { + t.Parallel() + + // A redirect built from a path containing a raw control character is an + // open redirect: http.Redirect emits it verbatim (url.Parse rejects the + // control byte and skips cleaning) and browsers strip tab/newline/CR + // before resolving, re-forming "//evil.com". originLocalURL percent-encodes + // each one. Assert every class is escaped so a future change that breaks + // encoding for only one class is caught. + cases := []struct { + name string + in string + want string + }{ + {name: "Tab", in: "/\t/evil.com", want: "/%09/evil.com"}, + {name: "Newline", in: "/\n/evil.com", want: "/%0A/evil.com"}, + {name: "CarriageReturn", in: "/\r/evil.com", want: "/%0D/evil.com"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + require.Equalf(t, tc.want, originLocalURL(tc.in).String(), + "originLocalURL(%q) must percent-encode the control character", tc.in) + }) + } + }) + + t.Run("PreservesLegitPaths", func(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + want string + }{ + {name: "Empty", in: "", want: "/"}, + {name: "Root", in: "/", want: "/"}, + {name: "Simple", in: "/test", want: "/test"}, + {name: "Nested", in: "/app/sub/page", want: "/app/sub/page"}, + {name: "PathApp", in: "/@user/ws/apps/app", want: "/@user/ws/apps/app"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + require.Equalf(t, tc.want, originLocalURL(tc.in).String(), "originLocalURL(%q)", tc.in) + }) + } + }) +} diff --git a/coderd/workspaceapps/proxy_test.go b/coderd/workspaceapps/proxy_test.go index 8678614243..cd180b3289 100644 --- a/coderd/workspaceapps/proxy_test.go +++ b/coderd/workspaceapps/proxy_test.go @@ -8,12 +8,15 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" + "time" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/jwtutils" "github.com/coder/coder/v2/coderd/workspaceapps" "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/testutil" @@ -34,57 +37,148 @@ func (s *fakeSignedTokenProvider) Issue(_ context.Context, _ http.ResponseWriter return nil, "", false } -func TestHandleSubdomain_IgnoresUntrustedForwardedHost(t *testing.T) { +func TestHandleSubdomain(t *testing.T) { t.Parallel() - hostnamePattern := "*--apps.test.coder.com" - hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern) - require.NoError(t, err) + t.Run("IgnoresUntrustedForwardedHost", func(t *testing.T) { + t.Parallel() - dashboardURL, err := url.Parse("https://dashboard.test.coder.com") - require.NoError(t, err) + hostnamePattern := "*--apps.test.coder.com" + hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern) + require.NoError(t, err) - provider := &fakeSignedTokenProvider{} - srv := workspaceapps.NewServer(workspaceapps.ServerOptions{ - Logger: testutil.Logger(t), - DashboardURL: dashboardURL, - AccessURL: dashboardURL, - Hostname: hostnamePattern, - HostnameRegex: hostnameRegex, - RealIPConfig: &httpmw.RealIPConfig{ - TrustedOrigins: []*net.IPNet{{ - IP: net.ParseIP("10.0.0.1"), - Mask: net.CIDRMask(32, 32), - }}, - }, - SignedTokenProvider: provider, + dashboardURL, err := url.Parse("https://dashboard.test.coder.com") + require.NoError(t, err) + + provider := &fakeSignedTokenProvider{} + srv := workspaceapps.NewServer(workspaceapps.ServerOptions{ + Logger: testutil.Logger(t), + DashboardURL: dashboardURL, + AccessURL: dashboardURL, + Hostname: hostnamePattern, + HostnameRegex: hostnameRegex, + RealIPConfig: &httpmw.RealIPConfig{ + TrustedOrigins: []*net.IPNet{{ + IP: net.ParseIP("10.0.0.1"), + Mask: net.CIDRMask(32, 32), + }}, + }, + SignedTokenProvider: provider, + }) + + forgedHost := appurl.ApplicationURL{ + AppSlugOrPort: "app", + WorkspaceName: "workspace", + Username: "victim", + }.String() + "--apps.test.coder.com" + + nextCalled := false + next := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + nextCalled = true + }) + + // Given: a request with a forged X-Forwarded-Host set to a valid + // app hostname, and an immediate peer outside the trusted proxy + // config. + req := httptest.NewRequest(http.MethodGet, "https://dashboard.test.coder.com/", nil) + req.Header.Set(httpapi.XForwardedHostHeader, forgedHost) + req.RemoteAddr = "17.18.19.20:1234" + + // When: HandleSubdomain runs. + srv.HandleSubdomain()(next).ServeHTTP(httptest.NewRecorder(), req) + + // Then: it ignores untrusted X-Forwarded-Host, so the received + // dashboard host is used, the request falls through to the next + // handler, and the signed app token provider is never called. + require.True(t, nextCalled) + require.Zero(t, provider.fromRequestCalls) + require.Zero(t, provider.issueCalls) }) - forgedHost := appurl.ApplicationURL{ - AppSlugOrPort: "app", - WorkspaceName: "workspace", - Username: "victim", - }.String() + "--apps.test.coder.com" + // After consuming a smuggled API key, the handler redirects to strip the key. + // A path that smuggles a separate host (e.g. "//evil.com") must redirect back + // to the current origin, not off-site. The full path-sanitization matrix lives + // in Test_originLocalURL. + t.Run("APIKeySmugglingStaysOnOrigin", func(t *testing.T) { + t.Parallel() - nextCalled := false - next := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { - nextCalled = true + ctx := testutil.Context(t, testutil.WaitShort) + + hostnamePattern := "*--apps.test.coder.com" + hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern) + require.NoError(t, err) + + dashboardURL, err := url.Parse("https://dashboard.test.coder.com") + require.NoError(t, err) + + // StaticKey lets us mint a smuggled key the handler can decrypt. + // A256GCMKW needs a 32-byte key. + keycache := jwtutils.StaticKey{ID: "test", Key: generateSecret(t, 32)} + payload := workspaceapps.EncryptedAPIKeyPayload{APIKey: "fake-api-key"} + payload.Fill(time.Now()) + encryptedAPIKey, err := jwtutils.Encrypt(ctx, keycache, payload) + require.NoError(t, err) + + srv := workspaceapps.NewServer(workspaceapps.ServerOptions{ + Logger: testutil.Logger(t), + DashboardURL: dashboardURL, + AccessURL: dashboardURL, + Hostname: hostnamePattern, + HostnameRegex: hostnameRegex, + RealIPConfig: &httpmw.RealIPConfig{ + TrustedOrigins: []*net.IPNet{{ + IP: net.ParseIP("10.0.0.1"), + Mask: net.CIDRMask(32, 32), + }}, + }, + SignedTokenProvider: &fakeSignedTokenProvider{}, + APIKeyEncryptionKeycache: keycache, + }) + + host := appurl.ApplicationURL{ + AppSlugOrPort: "app", + WorkspaceName: "workspace", + Username: "user", + }.String() + "--apps.test.coder.com" + + // Set r.URL.Path directly: the slash-collapsing middleware writes to chi's + // RoutePath, not r.URL.Path, so the raw "//evil.com" survives to the handler. + // "x=1" rides along to confirm unrelated params are preserved. + req := httptest.NewRequest(http.MethodGet, "https://"+host+"/", nil) + req.Host = host + req.RemoteAddr = "10.0.0.1:1234" + req.URL.Path = "//evil.com/phish" + req.URL.RawQuery = url.Values{ + workspaceapps.SubdomainProxyAPIKeyParam: {encryptedAPIKey}, + "x": {"1"}, + }.Encode() + + rec := httptest.NewRecorder() + nextCalled := false + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + nextCalled = true + }) + + srv.HandleSubdomain()(next).ServeHTTP(rec, req) + + res := rec.Result() + defer res.Body.Close() + + // The key is consumed and a redirect is issued instead of proxying. + require.Equal(t, http.StatusSeeOther, res.StatusCode) + require.False(t, nextCalled) + + loc := res.Header.Get("Location") + require.NotEmpty(t, loc) + require.False(t, strings.HasPrefix(loc, "//"), "redirect %q must stay same-origin", loc) + + parsed, err := url.Parse(loc) + require.NoError(t, err) + require.Empty(t, parsed.Scheme, "redirect %q must not carry a scheme", loc) + require.Empty(t, parsed.Host, "redirect %q must not carry a host", loc) + + // The smuggled key is stripped and unrelated query params survive. + require.Empty(t, parsed.Query().Get(workspaceapps.SubdomainProxyAPIKeyParam)) + require.Equal(t, "1", parsed.Query().Get("x")) }) - - // Given: a request with a forged X-Forwarded-Host set to a valid - // app hostname, and an immediate peer outside the trusted proxy - // config. - req := httptest.NewRequest(http.MethodGet, "https://dashboard.test.coder.com/", nil) - req.Header.Set(httpapi.XForwardedHostHeader, forgedHost) - req.RemoteAddr = "17.18.19.20:1234" - - // When: HandleSubdomain runs. - srv.HandleSubdomain()(next).ServeHTTP(httptest.NewRecorder(), req) - - // Then: it ignores untrusted X-Forwarded-Host, so the received - // dashboard host is used, the request falls through to the next - // handler, and the signed app token provider is never called. - require.True(t, nextCalled) - require.Zero(t, provider.fromRequestCalls) - require.Zero(t, provider.issueCalls) }