Fix Static Middleware and Router Inconsistent Escaping leading to possible Unauthorized static file disclosure

Fix StaticDirectoryHandler and Router Inconsistent Escaping leading to possible Unauthorized static file disclosure

Author: aldas

_fixture/dist/%2f..

@@ -0,0 +1 @@
+This filename is escaped to `/..` in URL.Path

_fixture/dist/public/admin/private.txt

@@ -0,0 +1 @@
+private file

echo.go

@@ -43,6 +43,7 @@ Learn more at https://echo.labstack.com
 package echo
 
 import (
+	"cmp"
 	stdContext "context"
 	"encoding/json"
 	"errors"
@@ -53,14 +54,11 @@ import (
 	"net/url"
 	"os"
 	"os/signal"
-	"path"
 	"path/filepath"
 	"strings"
 	"sync"
 	"sync/atomic"
 	"syscall"
-
-	"github.com/labstack/echo/v5/internal/pathutil"
 )
 
 // Echo is the top-level framework instance.
@@ -111,6 +109,8 @@ type Echo struct {
 
 	// formParseMaxMemory is passed to Context for multipart form parsing (See http.Request.ParseMultipartForm)
 	formParseMaxMemory int64
+
+	enablePathUnescapingStaticFiles bool
 }
 
 // JSONSerializer is the interface that encodes and decodes JSON to and from interfaces.
@@ -299,6 +299,19 @@ 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 path parameter (param: *) unescaping for static file methods.
+	// Default false (safe): encoded slashes (%2f) in the wildcard param are NOT decoded,
+	// preventing ACL bypass where /admin%2fprivate.txt bypasses a /admin/* route guard by
+	// not matching that route but having its wildcard param decoded to admin/private.txt.
+	// Set to true only when serving files whose names contain URL-encoded characters
+	// (e.g. "hello world.txt" via /hello%20world.txt) and you are not relying on
+	// route-based ACL guards to restrict access.
+	// If you are enabling this option, make sure you understand the security implications.
+	// See: https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
+	// Enabling RouterConfig.UseEscapedPathForMatching makes path escaping in static files and router consistent.
+	// Applies to methods: Echo.Static, Echo.StaticFS, Group.Static, Group.StaticFS.
+	EnablePathUnescapingStaticFiles bool
 }
 
 // NewWithConfig creates an instance of Echo with given configuration.
@@ -337,6 +350,8 @@ func NewWithConfig(config Config) *Echo {
 	if config.FormParseMaxMemory > 0 {
 		e.formParseMaxMemory = config.FormParseMaxMemory
 	}
+	e.enablePathUnescapingStaticFiles = config.EnablePathUnescapingStaticFiles
+
 	return e
 }
 
@@ -567,7 +582,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,49 +596,45 @@ 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...,
 	)
 }
 
 // StaticDirectoryHandler creates handler function to serve files from provided file system
 // When disablePathUnescaping is set then file name from path is not unescaped and is served as is.
+//
+// Note: Router is matching against unescaped path and using disablePathUnescaping=false here can lead to serving files
+// outside of the intended directory - if there are Routes that are meant to forbit some subset of the served filesystem
+// to be accessed, but inconsistency between how Router sees an unescaped path and this function will use an escaped path.
+// Enabling RouterConfig.UseEscapedPathForMatching makes path escaping in static files and router consistent.
+// See https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
 func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) HandlerFunc {
 	return func(c *Context) error {
 		p := c.Param("*")
 		if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
-			// By default the router matches routes against the raw, still-encoded request path
-			// (unless UseEscapedPathForMatching is enabled), so an encoded path separator is not
-			// treated as a segment boundary during routing. Unescaping it here would let it act as
-			// a separator and resolve a file outside the path the router authorized, bypassing
-			// route-level middleware (e.g. auth on a sibling route). No real filename contains a
-			// separator, so reject it as not found, carrying the reason internally for operators.
-			if pathutil.HasEncodedPathSeparator(p) {
-				return NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)).
-					Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p))
-			}
 			tmpPath, err := url.PathUnescape(p)
 			if err != nil {
 				return fmt.Errorf("failed to unescape path variable: %w", err)
 			}
 			p = tmpPath
 		}
 
-		// 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
-		// the router never matched on, the same Windows backslash traversal class as GHSA-pgvm-wxw2-hrv9).
-		name := path.Clean(strings.TrimPrefix(p, "/"))
+		// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/`
+		// as invalid
+		name := filepath.ToSlash(filepath.Clean(strings.TrimPrefix(p, "/")))
 		fi, err := fs.Stat(fileSystem, name)
 		if err != nil {
 			return ErrNotFound
 		}
 
-		// If the request is for a directory and does not end with "/"
-		p = c.Request().URL.Path // path must not be empty.
+		// If the request is for a directory and does not end with "/" redirect to path which ends with "/"
+		p = c.Request().URL.Path
 		if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' {
-			// Redirect to ends with "/"
-			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/"))
+			// prefer RawPath in redirect to avoid inconsistency when router is possible already escaping or not escaping
+			// as we are redirecting to same path, except added "/" at the end the RawPath is more consistent behavior
+			redirectPath := cmp.Or(c.Request().URL.RawPath, c.Request().URL.Path)
+			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(redirectPath+"/"))
 		}
 		return fsFile(c, name, fileSystem)
 	}

echo_test.go

@@ -134,14 +134,15 @@ func TestNewDefaultFS(t *testing.T) {
 
 func TestEcho_StaticFS(t *testing.T) {
 	var testCases = []struct {
-		givenFs              fs.FS
-		name                 string
-		givenPrefix          string
-		givenFsRoot          string
-		whenURL              string
-		expectHeaderLocation string
-		expectBodyStartsWith string
-		expectStatus         int
+		givenFs                              fs.FS
+		name                                 string
+		givenPrefix                          string
+		givenFsRoot                          string
+		givenEnablePathUnescapingStaticFiles bool
+		whenURL                              string
+		expectHeaderLocation                 string
+		expectBodyStartsWith                 string
+		expectStatus                         int
 	}{
 		{
 			name:                 "ok",
@@ -259,22 +260,40 @@ func TestEcho_StaticFS(t *testing.T) {
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
 		{
-			// An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): by default the
-			// router matches on the raw path so %2f is not a separator, and unescaping it here
-			// would let it act as one. No redirect is emitted, closing the open-redirect vector.
-			name:                 "encoded slash is rejected, not redirected",
+			name:                 "do not unescape path variables by default",
 			givenPrefix:          "/",
 			givenFs:              os.DirFS("_fixture/"),
 			whenURL:              "/open.redirect.hackercom%2f..",
 			expectStatus:         http.StatusNotFound,
-			expectHeaderLocation: "",
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
+		{
+			name:                                 "possible open redirect vulnerability when unescaping path variables in static handler",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: true,
+			whenURL:                              "/open.redirect.hackercom%2f..",
+			expectStatus:                         http.StatusMovedPermanently,
+			expectHeaderLocation:                 "/open.redirect.hackercom%2f../", // location starting with `//open` would be very bad
+			expectBodyStartsWith:                 "",
+		},
+		{
+			name:                                 "possible open redirect vulnerability when not unescaping path variables in static handler",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/dist/"),
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/%2f..",
+			expectStatus:                         http.StatusOK,
+			expectHeaderLocation:                 "",
+			expectBodyStartsWith:                 "This filename is escaped to `/..` in URL.Path\n",
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			e := New()
+			e := NewWithConfig(Config{
+				EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles,
+			})
 
 			tmpFs := tc.givenFs
 			if tc.givenFsRoot != "" {
@@ -305,6 +324,87 @@ func TestEcho_StaticFS(t *testing.T) {
 	}
 }
 
+func TestStaticDirectoryHandlerAndRouterInconsistentEscaping(t *testing.T) {
+	var testCases = []struct {
+		name                                 string
+		givenEnablePathUnescapingStaticFiles bool
+		givenRouterUnescapePathParamValues   bool
+		whenURL                              string
+		expectBodyStartsWith                 string
+		expectStatus                         int
+	}{
+		{
+			name:                                 "ok, file is served from not-forbidden path",
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/test.txt",
+			expectBodyStartsWith:                 "test.txt contents\n",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                                 "ok, forbidden path is matched by route wildcard and forbidden by that",
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/admin/private.txt",
+			expectBodyStartsWith:                 "{\"message\":\"Forbidden\"}\n",
+			expectStatus:                         http.StatusForbidden,
+		},
+		{
+			name:                                 "ok, escaped filename from forbidden path is not escaped and results 404",
+			givenEnablePathUnescapingStaticFiles: false, // router path escaping and StaticDirectoryHandler is consistent
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBodyStartsWith:                 "{\"message\":\"Not Found\"}\n",
+			expectStatus:                         http.StatusNotFound,
+		},
+		{
+			name:                                 "nok, escaped filename from forbidden path is escaped and returns file contents (handler escapes)",
+			givenEnablePathUnescapingStaticFiles: true, // router path escaping and StaticDirectoryHandler is NOT consistent
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBodyStartsWith:                 "private file\n",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                               "nok, escaped filename from forbidden path is escaped and returns file contents (router escapes)",
+			givenRouterUnescapePathParamValues: true, // router path escaping and StaticDirectoryHandler is NOT consistent
+			whenURL:                            "/admin%2fprivate.txt",
+			expectBodyStartsWith:               "private file\n",
+			expectStatus:                       http.StatusOK,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			r := NewRouter(RouterConfig{
+				UnescapePathParamValues: tc.givenRouterUnescapePathParamValues,
+			})
+			e := NewWithConfig(Config{
+				EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles,
+				Router:                          r,
+				Filesystem:                      os.DirFS("./_fixture/dist"),
+			})
+
+			// 1. share folder contents from the server root. This folder actually contains folder `admin` which contents we
+			// want to forbid from downloading
+			e.Static("/", "public")
+
+			// 2. naively assume that everything under /admin folder is now forbidden
+			e.GET("/admin/*", func(c *Context) error {
+				return ErrForbidden
+			})
+
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.Equal(t, tc.expectStatus, rec.Code)
+			body := rec.Body.String()
+			if tc.expectBodyStartsWith != "" {
+				assert.True(t, strings.HasPrefix(body, tc.expectBodyStartsWith))
+			} else {
+				assert.Equal(t, "", body)
+			}
+		})
+	}
+}
+
 func TestEcho_FileFS(t *testing.T) {
 	var testCases = []struct {
 		whenFS           fs.FS

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...,
 	)
 }