Skip to content

Commit 13f0ed1

Browse files
aldasaldas
authored
Merge pull request #3019 from aldas/v4_backport_3016
backport PR 3016 from v5 to v4
2 parents 8f167b9 + d16a4ec commit 13f0ed1

10 files changed

Lines changed: 289 additions & 158 deletions

File tree

‎echo.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,17 @@ type Echo struct {
106106
Debug bool
107107
HideBanner bool
108108
HidePort bool
109+
110+
// EnablePathUnescapingStaticFiles enables path parameter (param: *) unescaping for Static/StaticFS methods.
111+
// Default false (safe): encoded slashes (%2f) in the wildcard param are NOT decoded,
112+
// preventing ACL bypass where /admin%2fprivate.txt bypasses a /admin/* route guard by
113+
// not matching that route but having its wildcard param decoded to admin/private.txt.
114+
// Set to true only when serving files whose names contain URL-encoded characters
115+
// (e.g. "hello world.txt" via /hello%20world.txt) and you are not relying on
116+
// route-based ACL guards to restrict access.
117+
// If you are enabling this option, make sure you understand the security implications.
118+
// See: https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
119+
EnablePathUnescapingStaticFiles bool
109120
}
110121

111122
// Route contains a handler and information for matching against requests.

‎echo_fs.go‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ import (
1212
"path"
1313
"path/filepath"
1414
"strings"
15-
16-
"github.com/labstack/echo/v4/internal/pathutil"
1715
)
1816

1917
type filesystem struct {
@@ -38,7 +36,7 @@ func (e *Echo) Static(pathPrefix, fsRoot string) *Route {
3836
return e.Add(
3937
http.MethodGet,
4038
pathPrefix+"*",
41-
StaticDirectoryHandler(subFs, false),
39+
StaticDirectoryHandler(subFs, !e.EnablePathUnescapingStaticFiles),
4240
)
4341
}
4442

@@ -51,24 +49,23 @@ func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS) *Route {
5149
return e.Add(
5250
http.MethodGet,
5351
pathPrefix+"*",
54-
StaticDirectoryHandler(filesystem, false),
52+
StaticDirectoryHandler(filesystem, !e.EnablePathUnescapingStaticFiles),
5553
)
5654
}
5755

58-
// StaticDirectoryHandler creates handler function to serve files from provided file system
56+
// StaticDirectoryHandler creates handler function to serve files from provided file system.
5957
// When disablePathUnescaping is set then file name from path is not unescaped and is served as is.
58+
//
59+
// Note: when disablePathUnescaping=false, the handler decodes the wildcard param before serving.
60+
// If route guards (e.g. e.GET("/admin/*", forbidden)) are used to restrict parts of the
61+
// filesystem, an encoded separator (%2F) or encoded dot-dot (%2E%2E) in the URL can resolve to
62+
// a path that the router never matched against the guard route. Do not rely on route guards
63+
// alone to restrict a filesystem served by this handler.
64+
// See https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
6065
func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) HandlerFunc {
6166
return func(c Context) error {
6267
p := c.Param("*")
6368
if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
64-
// The router matches routes against the raw, still-encoded request path, so an
65-
// encoded path separator (%2F or %5C) is not treated as a segment boundary during
66-
// routing. Unescaping it here would let it act as a separator and resolve a file
67-
// outside the path the router authorized, bypassing route-level middleware (e.g. auth
68-
// on a sibling route). No real filename contains a separator, so reject it as not found.
69-
if pathutil.HasEncodedPathSeparator(p) {
70-
return ErrNotFound
71-
}
7269
tmpPath, err := url.PathUnescape(p)
7370
if err != nil {
7471
return fmt.Errorf("failed to unescape path variable: %w", err)

‎echo_fs_encoded_separator_test.go‎

Lines changed: 93 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -15,65 +15,113 @@ import (
1515
// Regression for GHSA-vfp3-v2gw-7wfq (v4 backport): an encoded path separator (%2F or %5C)
1616
// must not let a static file request resolve across a separator and bypass route-level middleware.
1717
func TestStaticDirectoryHandler_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
18-
fsys := fstest.MapFS{
19-
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
20-
"index.html": {Data: []byte("public")},
21-
}
22-
e := New()
23-
g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
24-
return func(c Context) error { return c.String(http.StatusForbidden, "denied") }
25-
})
26-
g.GET("/*", func(c Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
27-
e.StaticFS("/", fsys)
28-
29-
cases := []struct {
18+
var testCases = []struct {
19+
name string
3020
target string
3121
wantCode int
3222
wantBody string
3323
}{
34-
{"/admin/secret.txt", http.StatusForbidden, "denied"}, // protected route fires
35-
{"/admin%2Fsecret.txt", http.StatusNotFound, ""}, // encoded slash rejected, no disclosure
36-
{"/admin%2fsecret.txt", http.StatusNotFound, ""}, // lower-case hex variant
37-
{"/admin%5Csecret.txt", http.StatusNotFound, ""}, // encoded backslash (Windows separator) neutralized by path.Clean
38-
{"/admin%252Fsecret.txt", http.StatusNotFound, ""}, // double-encoded: single unescape -> literal filename, not a separator
39-
{"/index.html", http.StatusOK, "public"}, // legitimate static file still served
24+
{
25+
name: "protected route fires",
26+
target: "/admin/secret.txt",
27+
wantCode: http.StatusForbidden,
28+
wantBody: "denied",
29+
},
30+
{
31+
name: "encoded slash rejected, no disclosure",
32+
target: "/admin%2Fsecret.txt",
33+
wantCode: http.StatusNotFound,
34+
wantBody: "",
35+
},
36+
{
37+
name: "lower-case hex variant",
38+
target: "/admin%2fsecret.txt",
39+
wantCode: http.StatusNotFound,
40+
wantBody: "",
41+
},
42+
{
43+
name: "encoded backslash variant - Windows specific related",
44+
target: "/admin%5Csecret.txt",
45+
wantCode: http.StatusNotFound,
46+
wantBody: "",
47+
},
48+
{
49+
name: "double-encoded: single unescape -> literal filename, not a separator",
50+
target: "/admin%252Fsecret.txt",
51+
wantCode: http.StatusNotFound,
52+
wantBody: "",
53+
},
54+
{
55+
name: "legitimate static file still served",
56+
target: "/index.html",
57+
wantCode: http.StatusOK,
58+
wantBody: "public",
59+
},
4060
}
41-
for _, tc := range cases {
42-
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
43-
rec := httptest.NewRecorder()
44-
e.ServeHTTP(rec, req)
45-
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
46-
if tc.wantBody != "" {
47-
assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
48-
}
49-
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
61+
for _, tc := range testCases {
62+
t.Run(tc.name, func(t *testing.T) {
63+
fsys := fstest.MapFS{
64+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
65+
"index.html": {Data: []byte("public")},
66+
}
67+
e := New()
68+
g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
69+
return func(c Context) error { return c.String(http.StatusForbidden, "denied") }
70+
})
71+
g.GET("/*", func(c Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
72+
e.StaticFS("/", fsys)
73+
74+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
75+
rec := httptest.NewRecorder()
76+
e.ServeHTTP(rec, req)
77+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
78+
if tc.wantBody != "" {
79+
assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
80+
}
81+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
82+
})
5083
}
5184
}
5285

5386
// A Group-mounted StaticFS shares StaticDirectoryHandler, so it must reject the
5487
// same encoded separators when served under a non-root prefix.
5588
func TestGroupStaticFS_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
56-
fsys := fstest.MapFS{
57-
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
58-
"index.html": {Data: []byte("public")},
59-
}
60-
e := New()
61-
g := e.Group("/files")
62-
g.StaticFS("/", fsys)
63-
64-
cases := []struct {
89+
var testCases = []struct {
90+
name string
6591
target string
6692
wantCode int
6793
}{
68-
{"/files/index.html", http.StatusOK},
69-
{"/files/admin%2Fsecret.txt", http.StatusNotFound},
70-
{"/files/admin%5Csecret.txt", http.StatusNotFound},
94+
{
95+
name: "ok",
96+
target: "/files/index.html",
97+
wantCode: http.StatusOK,
98+
},
99+
{
100+
name: "nok, encoded slash",
101+
target: "/files/admin%2Fsecret.txt",
102+
wantCode: http.StatusNotFound,
103+
},
104+
{
105+
name: "nok encoded backslash",
106+
target: "/files/admin%5Csecret.txt",
107+
wantCode: http.StatusNotFound,
108+
},
71109
}
72-
for _, tc := range cases {
73-
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
74-
rec := httptest.NewRecorder()
75-
e.ServeHTTP(rec, req)
76-
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
77-
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
110+
for _, tc := range testCases {
111+
t.Run(tc.name, func(t *testing.T) {
112+
fsys := fstest.MapFS{
113+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
114+
"index.html": {Data: []byte("public")},
115+
}
116+
e := New()
117+
g := e.Group("/files")
118+
g.StaticFS("/", fsys)
119+
120+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
121+
rec := httptest.NewRecorder()
122+
e.ServeHTTP(rec, req)
123+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
124+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
125+
})
78126
}
79127
}

‎echo_fs_test.go‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,15 @@ import (
1515

1616
func TestEcho_StaticFS(t *testing.T) {
1717
var testCases = []struct {
18-
name string
19-
givenPrefix string
20-
givenFs fs.FS
21-
givenFsRoot string
22-
whenURL string
23-
expectStatus int
24-
expectHeaderLocation string
25-
expectBodyStartsWith string
18+
name string
19+
givenPrefix string
20+
givenFs fs.FS
21+
givenFsRoot string
22+
givenEnablePathUnescapingStaticFiles bool
23+
whenURL string
24+
expectStatus int
25+
expectHeaderLocation string
26+
expectBodyStartsWith string
2627
}{
2728
{
2829
name: "ok",
@@ -140,22 +141,38 @@ func TestEcho_StaticFS(t *testing.T) {
140141
expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
141142
},
142143
{
143-
// An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): the router matches
144-
// on the raw path so %2f is not a separator, and unescaping it here would let it act as
145-
// one. No redirect is emitted, closing the open-redirect vector.
146-
name: "encoded slash is rejected, not redirected",
144+
name: "do not unescape path variables by default",
147145
givenPrefix: "/",
148146
givenFs: os.DirFS("_fixture/"),
149147
whenURL: "/open.redirect.hackercom%2f..",
150148
expectStatus: http.StatusNotFound,
151149
expectHeaderLocation: "",
152150
expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
153151
},
152+
{
153+
name: "do not accept encoded dots in path (%2E%2E is `..`) to traverse within filesystem boundary",
154+
givenPrefix: "/",
155+
givenFs: os.DirFS("_fixture/"),
156+
givenEnablePathUnescapingStaticFiles: false,
157+
whenURL: `/folder/%2E%2E/index.html`, // `/folder/../index.html`
158+
expectStatus: http.StatusNotFound,
159+
expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
160+
},
161+
{
162+
name: "allow encoded dots in path (%2E%2E is `..`) to traverse within filesystem when path unescaping is enabled",
163+
givenPrefix: "/",
164+
givenFs: os.DirFS("_fixture/"),
165+
givenEnablePathUnescapingStaticFiles: true,
166+
whenURL: `/folder/%2E%2E/index.html`, // `/folder/../index.html`
167+
expectStatus: http.StatusOK,
168+
expectBodyStartsWith: "<!doctype html>",
169+
},
154170
}
155171

156172
for _, tc := range testCases {
157173
t.Run(tc.name, func(t *testing.T) {
158174
e := New()
175+
e.EnablePathUnescapingStaticFiles = tc.givenEnablePathUnescapingStaticFiles
159176

160177
tmpFs := tc.givenFs
161178
if tc.givenFsRoot != "" {

‎group_fs.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS) {
2323
g.Add(
2424
http.MethodGet,
2525
pathPrefix+"*",
26-
StaticDirectoryHandler(filesystem, false),
26+
StaticDirectoryHandler(filesystem, !g.echo.EnablePathUnescapingStaticFiles),
2727
)
2828
}
2929

‎internal/pathutil/pathutil.go‎

Lines changed: 0 additions & 30 deletions
This file was deleted.

‎internal/pathutil/pathutil_test.go‎

Lines changed: 0 additions & 27 deletions
This file was deleted.

0 commit comments

Comments
 (0)