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) + } +}