Fix Security Vulnerability - Directory Traversal

[little-cui]

Hi, We are Apache ServiceComb team. labstack/echo is the good project, we use it in our frontend project.

Recently, we have found a security vulnerability.

At echo.go（Line 483）

the static directory is bound by calling e.static ("/", staticPath).

The original intention is to read the root directory.
In Windows platform, POC can be constructed for path traversal.

Attack vector(s) :

1) Set up demo
2) In Windows platform, you can traverse through ..\

    POC-Request：

    GET /..\\other-dir/secret.txt HTTP/1.1

    HOST: 127.0.0.1:80

    Accept-Encoding: gzip, deflate

    Accept: */*

    Accept-Language: en

    User-Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1;
Win64; x64; Trident/5.0)

    Connection: close

[codecov]

Codecov Report

Merging #1718 (1beaf09) into master (2b36b3d) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1718   +/-   ##
=======================================
  Coverage   85.19%   85.19%           
=======================================
  Files          29       29           
  Lines        1986     1986           
=======================================
  Hits         1692     1692           
  Misses        186      186           
  Partials      108      108           

Impacted Files	Coverage Δ
echo.go	86.39% <100.00%> (ø)
middleware/static.go	68.65% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b36b3d...1beaf09. Read the comment docs.

[iambenkay]

Wow this is superb. Quite a vulnerability. Could you write a test that explicitly confirms that this prevents directory traversal?

[little-cui]

Wow this is superb. Quite a vulnerability. Could you write a test that explicitly confirms that this prevents directory traversal?

Sure, i am working on it

[aldas]

You could use this as base

func TestDirectoryTraversal(t *testing.T) {
	var testCases = []struct {
		name           string
		givenURL       string
		whenStaticRoot string
		expectContent  string
		expectError    string
	}{
		{
			name:           "ok, serve index",
			givenURL:       `/index.html`,
			whenStaticRoot: "../_fixture",
			expectContent:  "Echo",
		},
		{
			name:           "nok, do not allow directory traversal",
			givenURL:       `/..\\middleware/basic_auth.go`,
			whenStaticRoot: "../_fixture",
			expectContent:  "package middleware",
			expectError:    "code=404, message=Not Found",
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			e := echo.New()

			staticHandler := StaticWithConfig(StaticConfig{Root: tc.whenStaticRoot})(echo.NotFoundHandler)

			req := httptest.NewRequest(http.MethodGet, tc.givenURL, nil)
			rec := httptest.NewRecorder()
			c := e.NewContext(req, rec)

			err := staticHandler(c)
			if tc.expectError != "" {
				assert.Error(t, err)
				assert.EqualError(t, err, tc.expectError)

				assert.NotContains(t, rec.Body.String(), tc.expectContent)
			} else {
				assert.NoError(t, err)
				assert.Contains(t, rec.Body.String(), tc.expectContent)
			}
		})
	}
}

[aldas]

This seems to be windows specific problem (different separator).

shorter fix would be to change

echo/middleware/static.go

Line 170 in 2b36b3d

name := filepath.Join(config.Root, path.Clean("/"+p)) // "/"+ for security

to

name := filepath.Join(config.Root, filepath.Clean("/"+p))

seems to fix problem on Windows

[iambenkay]

Great! And concise!

[iambenkay]

@little-cui can you test out @aldas suggestion to know if it actually fixes the issue?

[aldas]

seems that path.Clean() works only for slashes
https://golang.org/pkg/path/#Clean

1. Replace multiple slashes with a single slash.
2. Eliminate each . path name element (the current directory).
3. Eliminate each inner .. path name element (the parent directory)
   along with the non-.. element that precedes it.
4. Eliminate .. elements that begin a rooted path:
   that is, replace "/.." by "/" at the beginning of a path.

but filePath.Clean() is dealing with OS specific separator
https://golang.org/pkg/path/filepath/#Clean

1. Replace multiple Separator elements with a single one.
2. Eliminate each . path name element (the current directory).
3. Eliminate each inner .. path name element (the parent directory)
   along with the non-.. element that precedes it.
4. Eliminate .. elements that begin a rooted path:
   that is, replace "/.." by "/" at the beginning of a path,
   assuming Separator is '/'.

[lammel]

Great find! @little-cui .

Could you adjust your code to the fix @aldas proposed with an added test please so our CI can make sure it is fixed.
Thanks!

[pafuent]

It seems that I missed all the fun 😢
Please @little-cui bare in mind that you need to apply the fix in Echo#static and in Static middleware (https://github.com/labstack/echo/blob/master/middleware/static.go), and of course provide tests for both implementations.

[aldas]

maybe it is faster if mainters will edit this PR with this (fixes static route and static middleware)
fix_static_route_directory_traversal.patch.txt

[little-cui]

Please review again

[lammel]

Looking good!
Thanks @little-cui and @aldas

[gurshafriri]

👋 @lammel Do you have a plan to also tag a version containing this fix?
Since the details of this issue is already public we (at snyk) would like to add it to our vulnerability database, better when a fixed version is already tagged.

[lammel]

Yes, a release is being prepared and expected within the next few days. The most important changes are already merged.

[rvasilevsf]

Hello,

Looks like the latest release is v4.1.17 tagged on 28 Aug 2020.
When do you plan to cut a new release?