diff --git a/echo.go b/echo.go index c95033f8e..614de188a 100644 --- a/echo.go +++ b/echo.go @@ -1020,8 +1020,8 @@ func escapeControlChars(s string) string { return b.String() } -// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment. A single leading and a single -// trailing slash are allowed. +// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment, or a segment with a "." or ".." +// part between backslashes (e.g. `..\x`). A single leading and a single trailing slash are allowed. // Keep in sync with the copy in middleware/static.go. func hasDotOrEmptySegment(p string) bool { p = strings.TrimPrefix(p, "/") @@ -1033,6 +1033,15 @@ func hasDotOrEmptySegment(p string) bool { if segment == "" || segment == "." || segment == ".." { return true } + // A backslash is a literal character in fs.FS names, but a filesystem that wrongly treats it as a separator + // (for example one built on filepath.Join on Windows) would resolve `..\` outside its root. + if strings.Contains(segment, `\`) { + for part := range strings.SplitSeq(segment, `\`) { + if part == "." || part == ".." { + return true + } + } + } } return false } diff --git a/echo_test.go b/echo_test.go index 6c2201750..e7c022541 100644 --- a/echo_test.go +++ b/echo_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type user struct { @@ -1889,3 +1890,79 @@ func TestStaticDirectoryHandler_encodedDotsWithPathUnescaping(t *testing.T) { }) } } + +// nonValidatingDirFS is a custom fs.FS that does not enforce fs.ValidPath, so a name with ".." escapes its root. Echo +// must never pass such a name to a user supplied filesystem. +type nonValidatingDirFS struct{ root string } + +func (f nonValidatingDirFS) Open(name string) (fs.File, error) { + // treat a backslash as a separator on every OS, like filepath.Join does on Windows + return os.Open(filepath.Join(f.root, filepath.FromSlash(strings.ReplaceAll(name, `\`, "/")))) +} + +func TestEcho_StaticFS_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + for _, unescape := range []bool{false, true} { + e := NewWithConfig(Config{EnablePathUnescapingStaticFiles: unescape}) + e.StaticFS("/", nonValidatingDirFS{root: filepath.Join(dir, "public")}) + + for _, target := range []string{ + "/../secret.txt", + "/%2e%2e/secret.txt", + "/..%2fsecret.txt", + "/sub/../../secret.txt", + "/..%5csecret.txt", + "/..%5Csecret.txt", + "/a%5C..%5C..%5Csecret.txt", + `/..\secret.txt`, + } { + req := httptest.NewRequest(http.MethodGet, target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", target, unescape) + assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", target, unescape) + } + + req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "public", rec.Body.String()) + } +} + +func TestHasDotOrEmptySegment(t *testing.T) { + var testCases = []struct { + path string + expect bool + }{ + {path: "", expect: false}, + {path: "/", expect: false}, + {path: "/index.html", expect: false}, + {path: "/css/app.css", expect: false}, + {path: "/..", expect: true}, + {path: "/a/../b", expect: true}, + {path: "/a/./b", expect: true}, + {path: "/a//b", expect: true}, + {path: `/..\secret.txt`, expect: true}, + {path: `/a\..\b`, expect: true}, + {path: `/.\secret.txt`, expect: true}, + {path: `/\..`, expect: true}, + {path: `/..\`, expect: true}, + {path: `/dir\file.txt`, expect: false}, + {path: "/...", expect: false}, + {path: "/..foo", expect: false}, + {path: `/a\\b`, expect: false}, + {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped + } + for _, tc := range testCases { + t.Run(tc.path, func(t *testing.T) { + assert.Equal(t, tc.expect, hasDotOrEmptySegment(tc.path)) + }) + } +} diff --git a/middleware/static.go b/middleware/static.go index deb77cd64..3fbef23ed 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -246,9 +246,11 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { // 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 - // 3. The "/" prefix forces absolute path interpretation, removing ".." components - // 4. Backslashes are treated as literal characters (not path separators), preventing traversal - // See static_windows.go for Go 1.20+ filepath.Clean compatibility notes + // 3. A path with a ".." segment is unclean and is not opened (it is handled like a missing file below), so the + // "./" prefix only keeps the name relative to the filesystem root; path.Clean() does not remove a leading + // ".." from a relative path + // 4. path.Clean() treats backslashes as literal characters; "."/".." parts between backslashes are rejected above + // for filesystems that wrongly treat a backslash as a separator filePath := path.Clean("./" + p) if config.IgnoreBase { @@ -413,8 +415,8 @@ func format(b int64) string { return fmt.Sprintf("%.2f%s", value, multiple) } -// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment. A single leading and a single -// trailing slash are allowed. +// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment, or a segment with a "." or ".." +// part between backslashes (e.g. `..\x`). A single leading and a single trailing slash are allowed. // Keep in sync with the copy in echo.go. func hasDotOrEmptySegment(p string) bool { p = strings.TrimPrefix(p, "/") @@ -426,6 +428,15 @@ func hasDotOrEmptySegment(p string) bool { if segment == "" || segment == "." || segment == ".." { return true } + // A backslash is a literal character in fs.FS names, but a filesystem that wrongly treats it as a separator + // (for example one built on filepath.Join on Windows) would resolve `..\` outside its root. + if strings.Contains(segment, `\`) { + for part := range strings.SplitSeq(segment, `\`) { + if part == "." || part == ".." { + return true + } + } + } } return false } diff --git a/middleware/static_test.go b/middleware/static_test.go index d1d59566e..5db1be2c7 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -8,11 +8,14 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" + "strings" "testing" "testing/fstest" "github.com/labstack/echo/v5" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestStatic_useCaseForApiAndSPAs(t *testing.T) { @@ -795,3 +798,90 @@ func TestStatic_HTML5WithUncleanPath(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "spa", rec.Body.String()) } + +// nonValidatingFS is a custom fs.FS that does not enforce fs.ValidPath, so a name with ".." escapes its root. Echo must +// never pass such a name to a user supplied filesystem. +type nonValidatingFS struct{ root string } + +func (f nonValidatingFS) Open(name string) (fs.File, error) { + // treat a backslash as a separator on every OS, like filepath.Join does on Windows + return os.Open(filepath.Join(f.root, filepath.FromSlash(strings.ReplaceAll(name, `\`, "/")))) +} + +func TestStatic_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + targets := []string{ + "/../secret.txt", + "/%2e%2e/secret.txt", + "/..%2fsecret.txt", + "/sub/../../secret.txt", + "/..%5csecret.txt", + "/..%5Csecret.txt", + "/a%5C..%5C..%5Csecret.txt", + `/..\secret.txt`, + } + for _, group := range []string{"", "/static"} { + for _, unescape := range []bool{false, true} { + e := echo.New() + mw := StaticWithConfig(StaticConfig{ + Filesystem: nonValidatingFS{root: filepath.Join(dir, "public")}, + EnablePathUnescaping: unescape, + }) + if group == "" { + e.Use(mw) + } else { + e.Group(group, mw) + } + + for _, target := range targets { + req := httptest.NewRequest(http.MethodGet, group+target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", group+target, unescape) + assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", group+target, unescape) + } + + req := httptest.NewRequest(http.MethodGet, group+"/index.txt", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code, "group=%q unescape=%v", group, unescape) + assert.Equal(t, "public", rec.Body.String()) + } + } +} + +func TestHasDotOrEmptySegment(t *testing.T) { + var testCases = []struct { + path string + expect bool + }{ + {path: "", expect: false}, + {path: "/", expect: false}, + {path: "/index.html", expect: false}, + {path: "/css/app.css", expect: false}, + {path: "/..", expect: true}, + {path: "/a/../b", expect: true}, + {path: "/a/./b", expect: true}, + {path: "/a//b", expect: true}, + {path: `/..\secret.txt`, expect: true}, + {path: `/a\..\b`, expect: true}, + {path: `/.\secret.txt`, expect: true}, + {path: `/\..`, expect: true}, + {path: `/..\`, expect: true}, + {path: `/dir\file.txt`, expect: false}, + {path: "/...", expect: false}, + {path: "/..foo", expect: false}, + {path: `/a\\b`, expect: false}, + {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped + } + for _, tc := range testCases { + t.Run(tc.path, func(t *testing.T) { + assert.Equal(t, tc.expect, hasDotOrEmptySegment(tc.path)) + }) + } +}