diff --git a/docs/content/migration/v2.md b/docs/content/migration/v2.md index 3e8ac3584..d56767071 100644 --- a/docs/content/migration/v2.md +++ b/docs/content/migration/v2.md @@ -656,3 +656,21 @@ Please check out the [entrypoint forwarded headers connection option configurati In `v2.11.14`, the `X-Forwarded-Prefix` header is now handled like the other `X-Forwarded-*` headers: Traefik removes it when it's sent from an untrusted source. Please refer to the Forwarded headers [documentation](../routing/entrypoints.md#forwarded-headers) for more details. + +## v2.11.23 + +### Request Path Sanitization + +Since `v2.11.23`, the incoming request path is now cleaned before being used to match the router rules and sent to the backends. +Any `/../`, `/./` or duplicate slash segments in the request path is interpreted and/or collapsed. + +If you want to disable this behavior, you can set the [`sanitizePath` option](../routing/entrypoints.md#sanitizepath) to `false` in the entryPoint HTTP configuration. +This can be useful when dealing with legacy clients that are not url-encoding data in the request path. +For example, as base64 uses the “/” character internally, +if it's not url encoded, +it can lead to unsafe routing when the `sanitizePath` option is set to `false`. + +!!! warning "Security" + + Setting the `sanitizePath` option to `false` is not safe. + Ensure every request is properly url encoded instead. diff --git a/docs/content/reference/static-configuration/cli-ref.md b/docs/content/reference/static-configuration/cli-ref.md index 540ff8ad0..5bdbe09a3 100644 --- a/docs/content/reference/static-configuration/cli-ref.md +++ b/docs/content/reference/static-configuration/cli-ref.md @@ -141,6 +141,9 @@ Scheme used for the redirection. (Default: ```https```) `--entrypoints..http.redirections.entrypoint.to`: Targeted entry point of the redirection. +`--entrypoints..http.sanitizepath`: +Defines whether to enable request path sanitization (removal of /./, /../ and multiple slash sequences). (Default: ```true```) + `--entrypoints..http.tls`: Default TLS configuration for the routers linked to the entry point. (Default: ```false```) diff --git a/docs/content/reference/static-configuration/env-ref.md b/docs/content/reference/static-configuration/env-ref.md index 173c18c3e..4e3630035 100644 --- a/docs/content/reference/static-configuration/env-ref.md +++ b/docs/content/reference/static-configuration/env-ref.md @@ -150,6 +150,9 @@ Scheme used for the redirection. (Default: ```https```) `TRAEFIK_ENTRYPOINTS__HTTP_REDIRECTIONS_ENTRYPOINT_TO`: Targeted entry point of the redirection. +`TRAEFIK_ENTRYPOINTS__HTTP_SANITIZEPATH`: +Defines whether to enable request path sanitization (removal of /./, /../ and multiple slash sequences). (Default: ```true```) + `TRAEFIK_ENTRYPOINTS__HTTP_TLS`: Default TLS configuration for the routers linked to the entry point. (Default: ```false```) diff --git a/docs/content/reference/static-configuration/file.toml b/docs/content/reference/static-configuration/file.toml index 89844adfe..c6ec8e715 100644 --- a/docs/content/reference/static-configuration/file.toml +++ b/docs/content/reference/static-configuration/file.toml @@ -37,6 +37,7 @@ [entryPoints.EntryPoint0.http] middlewares = ["foobar", "foobar"] encodeQuerySemicolons = true + sanitizePath = true [entryPoints.EntryPoint0.http.redirections] [entryPoints.EntryPoint0.http.redirections.entryPoint] to = "foobar" diff --git a/docs/content/reference/static-configuration/file.yaml b/docs/content/reference/static-configuration/file.yaml index 38541da46..42d9274d0 100644 --- a/docs/content/reference/static-configuration/file.yaml +++ b/docs/content/reference/static-configuration/file.yaml @@ -63,6 +63,7 @@ entryPoints: - foobar - foobar encodeQuerySemicolons: true + sanitizePath: true http2: maxConcurrentStreams: 42 http3: diff --git a/docs/content/routing/entrypoints.md b/docs/content/routing/entrypoints.md index d210a652e..df88cf16e 100644 --- a/docs/content/routing/entrypoints.md +++ b/docs/content/routing/entrypoints.md @@ -994,6 +994,56 @@ entryPoints: | false | foo=bar&baz=bar;foo | foo=bar&baz=bar&foo | | true | foo=bar&baz=bar;foo | foo=bar&baz=bar%3Bfoo | +### SanitizePath + +_Optional, Default=true_ + +The `sanitizePath` option defines whether to enable the request path sanitization. +When disabled, the incoming request path is passed to the backend as is. +This can be useful when dealing with legacy clients that are not url-encoding data in the request path. +For example, as base64 uses the “/” character internally, +if it's not url encoded, +it can lead to unsafe routing when the `sanitizePath` option is set to `false`. + +!!! warning "Security" + + Setting the sanitizePath option to false is not safe. + Ensure every request is properly url encoded instead. + +```yaml tab="File (YAML)" +entryPoints: + websecure: + address: ':443' + http: + sanitizePath: false +``` + +```toml tab="File (TOML)" +[entryPoints.websecure] + address = ":443" + + [entryPoints.websecure.http] + sanitizePath = false +``` + +```bash tab="CLI" +--entryPoints.websecure.address=:443 +--entryPoints.websecure.http.sanitizePath=false +``` + +#### Examples + +| SanitizePath | Request Path | Resulting Request Path | +|--------------|-----------------|------------------------| +| false | /./foo/bar | /./foo/bar | +| true | /./foo/bar | /foo/bar | +| false | /foo/../bar | /foo/../bar | +| true | /foo/../bar | /bar | +| false | /foo/bar// | /foo/bar// | +| true | /foo/bar// | /foo/bar/ | +| false | /./foo/../bar// | /./foo/../bar// | +| true | /./foo/../bar// | /bar/ | + ### Middlewares The list of middlewares that are prepended by default to the list of middlewares of each router associated to the named entry point. diff --git a/integration/fixtures/simple_clean_path.toml b/integration/fixtures/simple_clean_path.toml new file mode 100644 index 000000000..b889b750b --- /dev/null +++ b/integration/fixtures/simple_clean_path.toml @@ -0,0 +1,41 @@ +[global] + checkNewVersion = false + sendAnonymousUsage = false + +[entryPoints] + [entryPoints.web] + address = ":8000" + [entryPoints.web2] + address = ":8001" + [entryPoints.web2.http] + sanitizePath = false + +[log] + level = "DEBUG" + +[api] + insecure = true + +[providers.file] + filename = "{{ .SelfFilename }}" + +# dynamic configuration +[http.routers] + [http.routers.without] + rule = "PathPrefix(`/without`)" + service = "whoami" + + [http.routers.with] + rule = "PathPrefix(`/with`)" + middlewares = ["test-redirectscheme"] + service = "whoami" + +[http.middlewares] + [http.middlewares.test-redirectscheme.redirectScheme] + scheme = "https" + permanent = false + +[http.services] + [http.services.whoami.loadBalancer] + [[http.services.whoami.loadBalancer.servers]] + url = "{{ .Server1 }}" diff --git a/integration/https_test.go b/integration/https_test.go index 87a05480c..9ced3ffb9 100644 --- a/integration/https_test.go +++ b/integration/https_test.go @@ -937,11 +937,6 @@ func (s *HTTPSSuite) TestEntryPointHttpsRedirectAndPathModification() { hosts: []string{"example.com", "example2.com", "foo.com", "foo2.com", "bar.com", "bar2.com"}, path: "/api/", }, - { - desc: "Stripped URL with double trailing slash redirect", - hosts: []string{"example.com", "example2.com", "foo.com", "foo2.com", "bar.com", "bar2.com"}, - path: "/api//", - }, { desc: "Stripped URL with path redirect", hosts: []string{"example.com", "example2.com", "foo.com", "foo2.com", "bar.com", "bar2.com"}, @@ -952,21 +947,11 @@ func (s *HTTPSSuite) TestEntryPointHttpsRedirectAndPathModification() { hosts: []string{"example.com", "example2.com", "foo.com", "foo2.com", "bar.com", "bar2.com"}, path: "/api/bacon/", }, - { - desc: "Stripped URL with path and double trailing slash redirect", - hosts: []string{"example.com", "example2.com", "foo.com", "foo2.com", "bar.com", "bar2.com"}, - path: "/api/bacon//", - }, { desc: "Root Path with redirect", hosts: []string{"test.com", "test2.com", "pow.com", "pow2.com"}, path: "/", }, - { - desc: "Root Path with double trailing slash redirect", - hosts: []string{"test.com", "test2.com", "pow.com", "pow2.com"}, - path: "//", - }, { desc: "Path modify with redirect", hosts: []string{"test.com", "test2.com", "pow.com", "pow2.com"}, diff --git a/integration/simple_test.go b/integration/simple_test.go index 8ab5e0626..b48e41081 100644 --- a/integration/simple_test.go +++ b/integration/simple_test.go @@ -1385,3 +1385,88 @@ func (s *SimpleSuite) TestDenyFragment() { require.NoError(s.T(), err) assert.Equal(s.T(), http.StatusBadRequest, resp.StatusCode) } + +func (s *SimpleSuite) TestSanitizePath() { + s.createComposeProject("base") + + s.composeUp() + defer s.composeDown() + + whoami1URL := "http://" + net.JoinHostPort(s.getComposeServiceIP("whoami1"), "80") + + file := s.adaptFile("fixtures/simple_clean_path.toml", struct { + Server1 string + }{whoami1URL}) + + s.traefikCmd(withConfigFile(file)) + + err := try.GetRequest("http://127.0.0.1:8080/api/rawdata", 1*time.Second, try.BodyContains("PathPrefix(`/with`)")) + require.NoError(s.T(), err) + + testCases := []struct { + desc string + request string + target string + body string + expected int + }{ + { + desc: "Explicit call to the route with a middleware", + request: "GET /with HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8000", + expected: http.StatusFound, + }, + { + desc: "Explicit call to the route without a middleware", + request: "GET /without HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8000", + expected: http.StatusOK, + body: "GET /without HTTP/1.1", + }, + { + desc: "Implicit call to the route with a middleware", + request: "GET /without/../with HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8000", + expected: http.StatusFound, + }, + { + desc: "Explicit call to the route with a middleware, and disable path sanitization", + request: "GET /with HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8001", + expected: http.StatusFound, + }, + { + desc: "Explicit call to the route without a middleware, and disable path sanitization", + request: "GET /without HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8001", + expected: http.StatusOK, + body: "GET /without HTTP/1.1", + }, + { + desc: "Implicit call to the route with a middleware, and disable path sanitization", + request: "GET /without/../with HTTP/1.1\r\nHost: other.localhost\r\n\r\n", + target: "127.0.0.1:8001", + // The whoami is redirecting to /with, but the path is not sanitized. + expected: http.StatusMovedPermanently, + }, + } + + for _, test := range testCases { + conn, err := net.Dial("tcp", test.target) + require.NoError(s.T(), err) + + _, err = conn.Write([]byte(test.request)) + require.NoError(s.T(), err) + + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + require.NoError(s.T(), err) + + assert.Equalf(s.T(), test.expected, resp.StatusCode, "%s failed with %d instead of %d", test.desc, resp.StatusCode, test.expected) + + if test.body != "" { + body, err := io.ReadAll(resp.Body) + require.NoError(s.T(), err) + assert.Contains(s.T(), string(body), test.body) + } + } +} diff --git a/pkg/config/static/entrypoints.go b/pkg/config/static/entrypoints.go index 017480098..7fced1b85 100644 --- a/pkg/config/static/entrypoints.go +++ b/pkg/config/static/entrypoints.go @@ -52,6 +52,7 @@ func (ep *EntryPoint) SetDefaults() { ep.ForwardedHeaders = &ForwardedHeaders{} ep.UDP = &UDPConfig{} ep.UDP.SetDefaults() + ep.HTTP.SetDefaults() ep.HTTP2 = &HTTP2Config{} ep.HTTP2.SetDefaults() } @@ -61,7 +62,14 @@ type HTTPConfig struct { Redirections *Redirections `description:"Set of redirection" json:"redirections,omitempty" toml:"redirections,omitempty" yaml:"redirections,omitempty" export:"true"` Middlewares []string `description:"Default middlewares for the routers linked to the entry point." json:"middlewares,omitempty" toml:"middlewares,omitempty" yaml:"middlewares,omitempty" export:"true"` TLS *TLSConfig `description:"Default TLS configuration for the routers linked to the entry point." json:"tls,omitempty" toml:"tls,omitempty" yaml:"tls,omitempty" label:"allowEmpty" file:"allowEmpty" export:"true"` - EncodeQuerySemicolons bool `description:"Defines whether request query semicolons should be URLEncoded." json:"encodeQuerySemicolons,omitempty" toml:"encodeQuerySemicolons,omitempty" yaml:"encodeQuerySemicolons,omitempty"` + EncodeQuerySemicolons bool `description:"Defines whether request query semicolons should be URLEncoded." json:"encodeQuerySemicolons,omitempty" toml:"encodeQuerySemicolons,omitempty" yaml:"encodeQuerySemicolons,omitempty" export:"true"` + SanitizePath *bool `description:"Defines whether to enable request path sanitization (removal of /./, /../ and multiple slash sequences)." json:"sanitizePath,omitempty" toml:"sanitizePath,omitempty" yaml:"sanitizePath,omitempty" export:"true"` +} + +// SetDefaults sets the default values. +func (h *HTTPConfig) SetDefaults() { + sanitizePath := true + h.SanitizePath = &sanitizePath } // HTTP2Config is the HTTP2 configuration of an entry point. diff --git a/pkg/server/server_entrypoint_tcp.go b/pkg/server/server_entrypoint_tcp.go index 5d170685f..b3b42edb7 100644 --- a/pkg/server/server_entrypoint_tcp.go +++ b/pkg/server/server_entrypoint_tcp.go @@ -571,7 +571,12 @@ func createHTTPServer(ctx context.Context, ln net.Listener, configuration *stati return nil, err } - handler = denyFragment(handler) + if configuration.HTTP.SanitizePath != nil && *configuration.HTTP.SanitizePath { + // sanitizePath is used to clean the URL path by removing /../, /./ and duplicate slash sequences, + // to make sure the path is interpreted by the backends as it is evaluated inside rule matchers. + handler = sanitizePath(handler) + } + if configuration.HTTP.EncodeQuerySemicolons { handler = encodeQuerySemicolons(handler) } else { @@ -589,6 +594,8 @@ func createHTTPServer(ctx context.Context, ln net.Listener, configuration *stati }) } + handler = denyFragment(handler) + serverHTTP := &http.Server{ Handler: handler, ErrorLog: httpServerLogger, @@ -713,3 +720,20 @@ func denyFragment(h http.Handler) http.Handler { h.ServeHTTP(rw, req) }) } + +// sanitizePath removes the "..", "." and duplicate slash segments from the URL. +// It cleans the request URL Path and RawPath, and updates the request URI. +func sanitizePath(h http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + r2 := new(http.Request) + *r2 = *req + + // Cleans the URL raw path and path. + r2.URL = r2.URL.JoinPath() + + // Because the reverse proxy director is building query params from requestURI it needs to be updated as well. + r2.RequestURI = r2.URL.RequestURI() + + h.ServeHTTP(rw, r2) + }) +} diff --git a/pkg/server/server_entrypoint_tcp_test.go b/pkg/server/server_entrypoint_tcp_test.go index f3b8865c9..98066a4ca 100644 --- a/pkg/server/server_entrypoint_tcp_test.go +++ b/pkg/server/server_entrypoint_tcp_test.go @@ -8,6 +8,7 @@ import ( "io" "net" "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -382,3 +383,44 @@ func TestKeepAliveH2c(t *testing.T) { // to change. require.Contains(t, err.Error(), "use of closed network connection") } + +func TestSanitizePath(t *testing.T) { + tests := []struct { + path string + expected string + }{ + {path: "/b", expected: "/b"}, + {path: "/b/", expected: "/b/"}, + {path: "/../../b/", expected: "/b/"}, + {path: "/../../b", expected: "/b"}, + {path: "/a/b/..", expected: "/a"}, + {path: "/a/b/../", expected: "/a/"}, + {path: "/a/../../b", expected: "/b"}, + {path: "/..///b///", expected: "/b/"}, + {path: "/a/../b", expected: "/b"}, + {path: "/a/./b", expected: "/a/b"}, + {path: "/a//b", expected: "/a/b"}, + {path: "/a/../../b", expected: "/b"}, + {path: "/a/../c/../b", expected: "/b"}, + {path: "/a/../../../c/../b", expected: "/b"}, + {path: "/a/../c/../../b", expected: "/b"}, + {path: "/a/..//c/.././b", expected: "/b"}, + } + + for _, test := range tests { + t.Run("Testing case: "+test.path, func(t *testing.T) { + t.Parallel() + + var callCount int + clean := sanitizePath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + assert.Equal(t, test.expected, r.URL.Path) + })) + + request := httptest.NewRequest(http.MethodGet, "http://foo"+test.path, http.NoBody) + clean.ServeHTTP(httptest.NewRecorder(), request) + + assert.Equal(t, 1, callCount) + }) + } +}