Merge pull request #1718 from little-cui/master

Fix static directory traversal security vulnerability for Windows
labstack · Dec 15, 2020 · 4422e3b · 4422e3b
2 parents e7741d4 + 1beaf09
commit 4422e3b
Show file tree

Hide file tree

Showing 3 changed files with 99 additions and 40 deletions.
diff --git a/echo.go b/echo.go
@@ -49,7 +49,6 @@ import (
 	"net/http"
 	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"reflect"
 	"runtime"
@@ -486,7 +485,7 @@ func (common) static(prefix, root string, get func(string, HandlerFunc, ...Middl
 			return err
 		}
 
-		name := filepath.Join(root, path.Clean("/"+p)) // "/"+ for security
+		name := filepath.Join(root, filepath.Clean("/"+p)) // "/"+ for security
 		fi, err := os.Stat(name)
 		if err != nil {
 			// The access path does not exist

diff --git a/echo_test.go b/echo_test.go
@@ -60,45 +60,105 @@ func TestEcho(t *testing.T) {
 }
 
 func TestEchoStatic(t *testing.T) {
-	e := New()
-
-	assert := assert.New(t)
-
-	// OK
-	e.Static("/images", "_fixture/images")
-	c, b := request(http.MethodGet, "/images/walle.png", e)
-	assert.Equal(http.StatusOK, c)
-	assert.NotEmpty(b)
-
-	// No file
-	e.Static("/images", "_fixture/scripts")
-	c, _ = request(http.MethodGet, "/images/bolt.png", e)
-	assert.Equal(http.StatusNotFound, c)
-
-	// Directory
-	e.Static("/images", "_fixture/images")
-	c, _ = request(http.MethodGet, "/images/", e)
-	assert.Equal(http.StatusNotFound, c)
-
-	// Directory Redirect
-	e.Static("/", "_fixture")
-	req := httptest.NewRequest(http.MethodGet, "/folder", nil)
-	rec := httptest.NewRecorder()
-	e.ServeHTTP(rec, req)
-	assert.Equal(http.StatusMovedPermanently, rec.Code)
-	assert.Equal("/folder/", rec.HeaderMap["Location"][0])
-
-	// Directory with index.html
-	e.Static("/", "_fixture")
-	c, r := request(http.MethodGet, "/", e)
-	assert.Equal(http.StatusOK, c)
-	assert.Equal(true, strings.HasPrefix(r, "<!doctype html>"))
+	var testCases = []struct {
+		name                 string
+		givenPrefix          string
+		givenRoot            string
+		whenURL              string
+		expectStatus         int
+		expectHeaderLocation string
+		expectBodyStartsWith string
+	}{
+		{
+			name:                 "ok",
+			givenPrefix:          "/images",
+			givenRoot:            "_fixture/images",
+			whenURL:              "/images/walle.png",
+			expectStatus:         http.StatusOK,
+			expectBodyStartsWith: string([]byte{0x89, 0x50, 0x4e, 0x47}),
+		},
+		{
+			name:                 "No file",
+			givenPrefix:          "/images",
+			givenRoot:            "_fixture/scripts",
+			whenURL:              "/images/bolt.png",
+			expectStatus:         http.StatusNotFound,
+			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
+		},
+		{
+			name:                 "Directory",
+			givenPrefix:          "/images",
+			givenRoot:            "_fixture/images",
+			whenURL:              "/images/",
+			expectStatus:         http.StatusNotFound,
+			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
+		},
+		{
+			name:                 "Directory Redirect",
+			givenPrefix:          "/",
+			givenRoot:            "_fixture",
+			whenURL:              "/folder",
+			expectStatus:         http.StatusMovedPermanently,
+			expectHeaderLocation: "/folder/",
+			expectBodyStartsWith: "",
+		},
+		{
+			name:                 "Directory with index.html",
+			givenPrefix:          "/",
+			givenRoot:            "_fixture",
+			whenURL:              "/",
+			expectStatus:         http.StatusOK,
+			expectBodyStartsWith: "<!doctype html>",
+		},
+		{
+			name:                 "Sub-directory with index.html",
+			givenPrefix:          "/",
+			givenRoot:            "_fixture",
+			whenURL:              "/folder/",
+			expectStatus:         http.StatusOK,
+			expectBodyStartsWith: "<!doctype html>",
+		},
+		{
+			name:                 "do not allow directory traversal (backslash - windows separator)",
+			givenPrefix:          "/",
+			givenRoot:            "_fixture/",
+			whenURL:              `/..\\middleware/basic_auth.go`,
+			expectStatus:         http.StatusNotFound,
+			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
+		},
+		{
+			name:                 "do not allow directory traversal (slash - unix separator)",
+			givenPrefix:          "/",
+			givenRoot:            "_fixture/",
+			whenURL:              `/../middleware/basic_auth.go`,
+			expectStatus:         http.StatusNotFound,
+			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
+		},
+	}
 
-	// Sub-directory with index.html
-	c, r = request(http.MethodGet, "/folder/", e)
-	assert.Equal(http.StatusOK, c)
-	assert.Equal(true, strings.HasPrefix(r, "<!doctype html>"))
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := New()
+			e.Static(tc.givenPrefix, tc.givenRoot)
+			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)
+			}
 
+			if tc.expectHeaderLocation != "" {
+				assert.Equal(t, tc.expectHeaderLocation, rec.Result().Header["Location"][0])
+			} else {
+				_, ok := rec.Result().Header["Location"]
+				assert.False(t, ok)
+			}
+		})
+	}
 }
 
 func TestEchoFile(t *testing.T) {

diff --git a/middleware/static.go b/middleware/static.go
@@ -167,7 +167,7 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 			if err != nil {
 				return
 			}
-			name := filepath.Join(config.Root, path.Clean("/"+p)) // "/"+ for security
+			name := filepath.Join(config.Root, filepath.Clean("/"+p)) // "/"+ for security
 
 			if config.IgnoreBase {
 				routePath := path.Base(strings.TrimRight(c.Path(), "/*"))