From 91f630e034cbe0f5f21b3b450ce222fa0f7ab8c1 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 14 Jun 2026 21:27:30 -0700 Subject: [PATCH] fix(static): disable static path unescaping by default to prevent ACL bypass Fixes GHSA-3pmx-cf9f-34xr, a bypass of the GHSA-vfp3-v2gw-7wfq fix. The router matches the raw, still-encoded request path, so encoded separators and dot segments in a static wildcard are not seen as traversal during routing. Unescaping them in the static file resolver afterwards let an attacker reach a file across a route-level middleware guard the encoded path never matched: - /public/%2E%2E/admin/secret.txt resolved to admin/secret.txt (high severity, default router) - /public%2F..%2Fadmin%2Fsecret.txt under UseEscapedPathForMatching=true, where the router decodes the path itself before the handler sees it Rather than keep extending an encoding denylist, address the root cause: make static path unescaping opt-in. - echo: Config.EnablePathUnescapingStaticFiles (default false) controls unescaping for Echo.Static/StaticFS and Group.Static/StaticFS. - middleware: StaticConfig.EnablePathUnescaping replaces the now deprecated DisablePathUnescaping (default is the safe, no-unescape mode). With unescaping off, %2F/%5C/%2E%2E stay literal and never become separators or traversal. As defense in depth, and to also close the UseEscapedPathForMatching variant (where the router, not the handler, does the decoding), reject any ".." path segment in the resolved wildcard via pathutil.HasDotDotSegment, mirroring the fs.ValidPath "no .. element" invariant. The existing encoded-separator guard remains as a backstop on the opt-in unescaping path. BREAKING CHANGE: static files whose names contain URL-encoded characters (e.g. "hello world.txt" via /hello%20world.txt) are no longer served by default; set EnablePathUnescapingStaticFiles / EnablePathUnescaping to opt back in. Co-Authored-By: Claude Opus 4.8 (1M context) --- echo.go | 35 +++++++++++- group.go | 2 +- internal/pathutil/pathutil.go | 32 +++++++++++ internal/pathutil/pathutil_test.go | 20 +++++++ middleware/static.go | 38 +++++++++--- middleware/static_test.go | 67 ++++++++++++++++++++++ static_encoded_dotdot_test.go | 92 ++++++++++++++++++++++++++++++ 7 files changed, 276 insertions(+), 10 deletions(-) create mode 100644 static_encoded_dotdot_test.go diff --git a/echo.go b/echo.go index ee3a53cd0..2594a2482 100644 --- a/echo.go +++ b/echo.go @@ -111,6 +111,10 @@ type Echo struct { // formParseMaxMemory is passed to Context for multipart form parsing (See http.Request.ParseMultipartForm) formParseMaxMemory int64 + + // enablePathUnescapingStaticFiles unescapes the static wildcard param (set via + // Config.EnablePathUnescapingStaticFiles). Default false (safe): see that field. + enablePathUnescapingStaticFiles bool } // JSONSerializer is the interface that encodes and decodes JSON to and from interfaces. @@ -299,6 +303,21 @@ type Config struct { // FormParseMaxMemory is default value for memory limit that is used // when parsing multipart forms (See (*http.Request).ParseMultipartForm) FormParseMaxMemory int64 + + // EnablePathUnescapingStaticFiles enables unescaping of the wildcard param (param: *) + // for static file methods (Echo.Static, Echo.StaticFS, Group.Static, Group.StaticFS). + // + // Default false (safe): the router matches the raw, still-encoded request path, so + // encoded separators/dot segments (%2F, %5C, %2E%2E) are NOT decoded when resolving + // the file. This prevents ACL bypass where e.g. /admin%2Fprivate.txt or + // /public/%2E%2E/admin/secret.txt resolve a file across a route-level middleware guard + // the encoded path never matched (GHSA-vfp3-v2gw-7wfq, GHSA-3pmx-cf9f-34xr). + // + // Set to true only when serving files whose names contain URL-encoded characters + // (e.g. "hello world.txt" via /hello%20world.txt) and you do not rely on route-based + // ACL guards to restrict access. Encoded separators and ".." segments are still + // rejected even when enabled. + EnablePathUnescapingStaticFiles bool } // NewWithConfig creates an instance of Echo with given configuration. @@ -337,6 +356,7 @@ func NewWithConfig(config Config) *Echo { if config.FormParseMaxMemory > 0 { e.formParseMaxMemory = config.FormParseMaxMemory } + e.enablePathUnescapingStaticFiles = config.EnablePathUnescapingStaticFiles return e } @@ -567,7 +587,7 @@ func (e *Echo) Static(pathPrefix, fsRoot string, middleware ...MiddlewareFunc) R return e.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(subFs, false), + StaticDirectoryHandler(subFs, !e.enablePathUnescapingStaticFiles), middleware..., ) } @@ -581,7 +601,7 @@ func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Middl return e.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(filesystem, false), + StaticDirectoryHandler(filesystem, !e.enablePathUnescapingStaticFiles), middleware..., ) } @@ -609,6 +629,17 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle p = tmpPath } + // Reject parent-directory traversal in the resolved wildcard. A ".." segment is + // only ever present here because it was decoded after routing (encoded "%2E%2E" + // when unescaping is enabled, or decoded by the router itself when + // UseEscapedPathForMatching is set); the router matched a prefix that never + // contained it. Resolving it would cross the route/static-mount boundary the + // match authorized, bypassing route-level middleware (GHSA-3pmx-cf9f-34xr). + if pathutil.HasDotDotSegment(p) { + return NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)). + Wrap(fmt.Errorf("rejected dot-dot path segment in static path %q", p)) + } + // fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid. // Use path.Clean (not filepath.Clean): fs.FS paths are always forward-slash, so a backslash must stay a literal // character rather than being interpreted as a separator on Windows (which would resolve a file across a boundary diff --git a/group.go b/group.go index 8092bc904..03289fac3 100644 --- a/group.go +++ b/group.go @@ -123,7 +123,7 @@ func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Midd return g.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(filesystem, false), + StaticDirectoryHandler(filesystem, !g.echo.enablePathUnescapingStaticFiles), middleware..., ) } diff --git a/internal/pathutil/pathutil.go b/internal/pathutil/pathutil.go index e9171752e..e792e8afd 100644 --- a/internal/pathutil/pathutil.go +++ b/internal/pathutil/pathutil.go @@ -6,6 +6,8 @@ // middleware package without becoming part of the public API. package pathutil +import "strings" + // HasEncodedPathSeparator reports whether s contains a percent-encoded path // separator, case-insensitively: %2F/%2f (forward slash) or %5C/%5c (backslash). // Backslash is included as defense-in-depth against Windows-style separators even @@ -28,3 +30,33 @@ func HasEncodedPathSeparator(s string) bool { } return false } + +// HasDotDotSegment reports whether p, split on '/', contains a ".." path segment. +// Both ends of the string and every '/' act as boundaries, so "..", "../x", "x/..", +// and "a/../b" match while "..foo" and "foo.." do not. +// +// A ".." segment is parent-directory traversal. The router matches a specific path +// prefix, but a ".." that only becomes a real segment after decoding (encoded as +// "%2E%2E", or decoded by the router itself when UseEscapedPathForMatching is set) +// is never seen as traversal during routing. Cleaning it then resolves a file +// across the route or static-mount boundary the matched route authorized, bypassing +// route-level middleware (GHSA-3pmx-cf9f-34xr). This mirrors the "no .. element" +// invariant of fs.ValidPath; no real filename is "..", so reject it. +// +// Only '/' is a separator here: encoded backslashes are rejected earlier by +// HasEncodedPathSeparator, and on a forward-slash fs.FS a literal backslash never +// acts as a separator, so a "..\\" segment cannot traverse. +func HasDotDotSegment(p string) bool { + for len(p) > 0 { + seg := p + if i := strings.IndexByte(p, '/'); i >= 0 { + seg, p = p[:i], p[i+1:] + } else { + p = "" + } + if seg == ".." { + return true + } + } + return false +} diff --git a/internal/pathutil/pathutil_test.go b/internal/pathutil/pathutil_test.go index fd9a9d90d..72e256181 100644 --- a/internal/pathutil/pathutil_test.go +++ b/internal/pathutil/pathutil_test.go @@ -25,3 +25,23 @@ func TestHasEncodedPathSeparator(t *testing.T) { assert.Equal(t, want, HasEncodedPathSeparator(s), "input=%q", s) } } + +func TestHasDotDotSegment(t *testing.T) { + for s, want := range map[string]bool{ + "..": true, + "../x": true, + "x/..": true, + "a/../b": true, + "public/../admin/sec.txt": true, + "/..": true, // leading slash + "foo/bar.txt": false, + "..foo": false, // not a standalone segment + "foo..": false, + "...": false, // three dots is a real name + "%2E%2E": false, // still-encoded, not yet a separator-bounded ".." + `a/..\b`: false, // backslash is literal on fs.FS, segment is "..\b" + "": false, + } { + assert.Equal(t, want, HasDotDotSegment(s), "input=%q", s) + } +} diff --git a/middleware/static.go b/middleware/static.go index 5169ec80d..e74feae21 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -54,10 +54,25 @@ type StaticConfig struct { // Optional. Default value false. IgnoreBase bool - // DisablePathUnescaping disables path parameter (param: *) unescaping. This is useful when router is set to unescape - // all parameter and doing it again in this middleware would corrupt filename that is requested. + // Deprecated: use EnablePathUnescaping instead. With unescaping now disabled by + // default, this flag is redundant; when set it still forces unescaping off (the + // safe direction) so existing code keeps working. Will be removed in a future version. DisablePathUnescaping bool + // EnablePathUnescaping enables path parameter (param: *) unescaping. + // + // Default false (safe): the router matches the raw, still-encoded request path, so + // encoded separators/dot segments (%2F, %5C, %2E%2E) are NOT decoded when resolving + // the file. This prevents ACL bypass where e.g. /admin%2Fprivate.txt or + // /public/%2E%2E/admin/secret.txt resolve a file across a route-level middleware guard + // the encoded path never matched (GHSA-vfp3-v2gw-7wfq, GHSA-3pmx-cf9f-34xr). + // + // Set to true only when serving files whose names contain URL-encoded characters + // (e.g. "hello world.txt" via /hello%20world.txt) and you do not rely on route-based + // ACL guards to restrict access. Encoded separators and ".." segments are still + // rejected even when enabled. + EnablePathUnescaping bool + // DirectoryListTemplate is template to list directory contents // Optional. Default to `directoryListHTMLTemplate` constant below. DirectoryListTemplate string @@ -200,14 +215,14 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { // URL.Path is already decoded by net/http, so it must not be unescaped // again (doing so breaks file names containing '%', see #2599). Only the // wildcard param from a group route (set below) may still be escaped. - pathUnescape := false if strings.HasSuffix(c.Path(), "*") { // When serving from a group, e.g. `/static*`. p = c.Param("*") - pathUnescape = !config.DisablePathUnescaping // because router could already do PathUnescape } - if pathUnescape { - // The router matched on the raw, still-encoded path (by default), so an encoded - // path separator in the wildcard would only now become a real separator and + // Unescaping is opt-in (see EnablePathUnescaping). DisablePathUnescaping is the + // deprecated inverse and, if set, still forces it off (the safe direction). + if config.EnablePathUnescaping && !config.DisablePathUnescaping { + // The router matched on the raw, still-encoded path, so an encoded path + // separator in the wildcard would only now become a real separator and // resolve a file the matched route never authorized, bypassing route-level // middleware. Reject it before unescaping (see echo.StaticDirectoryHandler). if pathutil.HasEncodedPathSeparator(p) { @@ -219,6 +234,15 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { return err } } + // Reject parent-directory traversal: a ".." segment present here was decoded + // after routing (encoded "%2E%2E", or decoded by the router itself when + // UseEscapedPathForMatching is set), so the matched route never saw it as + // traversal. Resolving it would cross the route/static-mount boundary the match + // authorized, bypassing route-level middleware (GHSA-3pmx-cf9f-34xr). + if pathutil.HasDotDotSegment(p) { + return echo.NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)). + Wrap(fmt.Errorf("rejected dot-dot path segment in static path %q", p)) + } // Security: We use path.Clean() (not filepath.Clean()) because: // 1. HTTP URLs always use forward slashes, regardless of server OS // 2. path.Clean() provides platform-independent behavior for URL paths diff --git a/middleware/static_test.go b/middleware/static_test.go index 0390c9e93..f21ea057a 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -309,6 +309,73 @@ func TestStatic_EncodedSeparatorDoesNotBypassRoute(t *testing.T) { } } +// Regression for GHSA-3pmx-cf9f-34xr in the static middleware: encoded or +// router-decoded ".." must not resolve a file across a route-level guard. +func TestStatic_EncodedDotDotDoesNotBypassRoute(t *testing.T) { + run := func(t *testing.T, e *echo.Echo) { + fsys := fstest.MapFS{ + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + "public/ok.txt": {Data: []byte("public")}, + } + g := e.Group("/files", StaticWithConfig(StaticConfig{Filesystem: fsys})) + g.GET("/*", func(c *echo.Context) error { return echo.ErrNotFound }) + + cases := []struct { + target string + wantCode int + }{ + {"/files/public/ok.txt", http.StatusOK}, + {"/files/public/%2E%2E/admin/secret.txt", http.StatusNotFound}, + {"/files/public%2F..%2Fadmin%2Fsecret.txt", http.StatusNotFound}, + {"/files/public/../admin/secret.txt", http.StatusNotFound}, + } + for _, tc := range cases { + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + } + } + + t.Run("default router", func(t *testing.T) { run(t, echo.New()) }) + t.Run("UseEscapedPathForMatching", func(t *testing.T) { + run(t, echo.NewWithConfig(echo.Config{Router: echo.NewRouter(echo.RouterConfig{UseEscapedPathForMatching: true})})) + }) +} + +// With EnablePathUnescaping the wildcard is unescaped (encoded file names work) but +// encoded separators and ".." segments are still rejected. +func TestStatic_EnablePathUnescaping(t *testing.T) { + fsys := fstest.MapFS{ + "hello world.txt": {Data: []byte("spaced")}, + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + } + e := echo.New() + g := e.Group("/files", StaticWithConfig(StaticConfig{Filesystem: fsys, EnablePathUnescaping: true})) + g.GET("/*", func(c *echo.Context) error { return echo.ErrNotFound }) + + cases := []struct { + target string + wantCode int + wantBody string + }{ + {"/files/hello%20world.txt", http.StatusOK, "spaced"}, + {"/files/admin%2Fsecret.txt", http.StatusNotFound, ""}, + {"/files/public/%2E%2E/admin/secret.txt", http.StatusNotFound, ""}, + } + for _, tc := range cases { + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + if tc.wantBody != "" { + assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target) + } + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + } +} + func TestMustStaticWithConfig_panicsInvalidDirListTemplate(t *testing.T) { assert.Panics(t, func() { StaticWithConfig(StaticConfig{DirectoryListTemplate: `{{}`}) diff --git a/static_encoded_dotdot_test.go b/static_encoded_dotdot_test.go new file mode 100644 index 000000000..53bbb50c6 --- /dev/null +++ b/static_encoded_dotdot_test.go @@ -0,0 +1,92 @@ +package echo + +import ( + "net/http" + "net/http/httptest" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/assert" +) + +func adminSecretFS() fstest.MapFS { + return fstest.MapFS{ + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + "public/ok.txt": {Data: []byte("public")}, + "index.html": {Data: []byte("index")}, + } +} + +// Regression for GHSA-3pmx-cf9f-34xr: encoded/decoded ".." in a static wildcard must +// not resolve a file across a route-level middleware guard. With unescaping disabled +// by default the encoded form never decodes, and the dot-dot guard rejects any ".." +// that the router (e.g. UseEscapedPathForMatching) decoded itself. +func TestStaticFS_EncodedDotDotDoesNotBypassRoute(t *testing.T) { + run := func(t *testing.T, e *Echo) { + g := e.Group("/admin", func(next HandlerFunc) HandlerFunc { + return func(c *Context) error { return c.String(http.StatusForbidden, "denied") } + }) + g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached-protected-handler") }) + e.StaticFS("/", adminSecretFS()) + + cases := []struct { + target string + wantCode int + }{ + {"/admin/secret.txt", http.StatusForbidden}, // protected route fires + {"/public/%2E%2E/admin/secret.txt", http.StatusNotFound}, // high-sev: encoded dot-dot + {"/public/%2e%2e/admin/secret.txt", http.StatusNotFound}, // lower-case hex + {"/public%2F..%2Fadmin%2Fsecret.txt", http.StatusNotFound}, // encoded-slash variant + {"/public/../admin/secret.txt", http.StatusNotFound}, // literal dot-dot + {"/public/ok.txt", http.StatusOK}, // legitimate static file + {"/index.html", http.StatusOK}, // legitimate static file + } + for _, tc := range cases { + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + } + } + + t.Run("default router", func(t *testing.T) { run(t, New()) }) + t.Run("UseEscapedPathForMatching", func(t *testing.T) { + run(t, NewWithConfig(Config{Router: NewRouter(RouterConfig{UseEscapedPathForMatching: true})})) + }) +} + +// With EnablePathUnescapingStaticFiles the wildcard is unescaped (so encoded file +// names work), but encoded separators and ".." segments are still rejected. +func TestStaticFS_EnablePathUnescapingStaticFiles(t *testing.T) { + fsys := fstest.MapFS{ + "hello world.txt": {Data: []byte("spaced")}, + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + } + e := NewWithConfig(Config{EnablePathUnescapingStaticFiles: true}) + g := e.Group("/admin", func(next HandlerFunc) HandlerFunc { + return func(c *Context) error { return c.String(http.StatusForbidden, "denied") } + }) + g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached") }) + e.StaticFS("/", fsys) + + cases := []struct { + target string + wantCode int + wantBody string + }{ + {"/hello%20world.txt", http.StatusOK, "spaced"}, // encoded space now decoded and served + {"/admin%2Fsecret.txt", http.StatusNotFound, ""}, // encoded slash still rejected + {"/public/%2E%2E/admin/secret.txt", http.StatusNotFound, ""}, // encoded dot-dot still rejected + } + for _, tc := range cases { + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + if tc.wantBody != "" { + assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target) + } + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + } +}