From f3a183ecc1c642b05dee12a06b3e977d90b49eee Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Fri, 11 Mar 2016 15:39:13 +0100 Subject: [PATCH 1/4] Use filepath.Clean for fileserver. --- middleware/fileserver.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/middleware/fileserver.go b/middleware/fileserver.go index 80cf6f84..9c15900e 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -4,6 +4,7 @@ import ( "net/http" "os" "path" + "path/filepath" "strings" ) @@ -43,7 +44,7 @@ func (fh *fileHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, e upath = "/" + upath r.URL.Path = upath } - return fh.serveFile(w, r, path.Clean(upath)) + return fh.serveFile(w, r, filepath.Clean(upath)) } // serveFile writes the specified file to the HTTP response. From 84845a66ab3953351f2dccc8ed00cd491f7f52f6 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Fri, 11 Mar 2016 23:11:21 +0100 Subject: [PATCH 2/4] Fix broken build. --- middleware/fileserver.go | 42 +++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/middleware/fileserver.go b/middleware/fileserver.go index 9c15900e..4b3cab02 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -44,12 +44,19 @@ func (fh *fileHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, e upath = "/" + upath r.URL.Path = upath } - return fh.serveFile(w, r, filepath.Clean(upath)) + return fh.serveFile(w, r, path.Clean(upath)) } // serveFile writes the specified file to the HTTP response. // name is '/'-separated, not filepath.Separator. func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name string) (int, error) { + // Prevent absolute path access on Windows. + // TODO remove when stdlib http.Dir fixes this. + if runtimeGoos == "windows" { + if filepath.IsAbs(name[1:]) { + return http.StatusNotFound, nil + } + } f, err := fh.root.Open(name) if err != nil { if os.IsNotExist(err) { @@ -114,6 +121,28 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st return http.StatusNotFound, nil } + // If file is on hide list. + if fh.isHidden(d.Name()) { + return http.StatusNotFound, nil + } + + // Note: Errors generated by ServeContent are written immediately + // to the response. This usually only happens if seeking fails (rare). + http.ServeContent(w, r, d.Name(), d.ModTime(), f) + + return http.StatusOK, nil +} + +// isHidden checks if file with name is on hide list. +func (fh fileHandler) isHidden(name string) bool { + // Clean up on Windows. + // Remove trailing dots and trim whitespaces. + if runtimeGoos == "windows" { + name = strings.TrimSpace(name) + for strings.HasSuffix(name, ".") { + name = name[:len(name)-1] + } + } // If the file is supposed to be hidden, return a 404 // (TODO: If the slice gets large, a set may be faster) for _, hiddenPath := range fh.hide { @@ -123,16 +152,11 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st // TODO: This matches file NAME only, regardless of path. In other // words, trying to serve another file with the same name as the // active config file will result in a 404 when it shouldn't. - if strings.EqualFold(d.Name(), path.Base(hiddenPath)) { - return http.StatusNotFound, nil + if strings.EqualFold(name, filepath.Base(hiddenPath)) { + return true } } - - // Note: Errors generated by ServeContent are written immediately - // to the response. This usually only happens if seeking fails (rare). - http.ServeContent(w, r, d.Name(), d.ModTime(), f) - - return http.StatusOK, nil + return false } // redirect is taken from http.localRedirect of the std lib. It From e92a911e7d70c5cec0ec1434a501d6a777c333a4 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Fri, 11 Mar 2016 23:44:50 +0100 Subject: [PATCH 3/4] Add more tests. --- middleware/fileserver.go | 1 + middleware/fileserver_test.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/middleware/fileserver.go b/middleware/fileserver.go index 4b3cab02..6cdb0ff5 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -141,6 +141,7 @@ func (fh fileHandler) isHidden(name string) bool { name = strings.TrimSpace(name) for strings.HasSuffix(name, ".") { name = name[:len(name)-1] + name = strings.TrimSpace(name) } } // If the file is supposed to be hidden, return a 404 diff --git a/middleware/fileserver_test.go b/middleware/fileserver_test.go index 0f5b1fac..ba2f23ba 100644 --- a/middleware/fileserver_test.go +++ b/middleware/fileserver_test.go @@ -112,6 +112,26 @@ func TestServeHTTP(t *testing.T) { expectedStatus: http.StatusMovedPermanently, expectedBodyContent: movedPermanently, }, + // Test 11 - attempt to bypass hidden file + { + url: "https://foo/dir/hidden.html%20", + expectedStatus: http.StatusNotFound, + }, + // Test 12 - attempt to bypass hidden file + { + url: "https://foo/dir/hidden.html.", + expectedStatus: http.StatusNotFound, + }, + // Test 13 - attempt to bypass hidden file + { + url: "https://foo/dir/hidden.html.%20", + expectedStatus: http.StatusNotFound, + }, + // Test 14 - attempt to bypass hidden file + { + url: "https://foo/dir/hidden.html%20.", + expectedStatus: http.StatusNotFound, + }, } for i, test := range tests { From 008ad398cec59c7af600c501ae34a76b5ca1fe0a Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Sat, 12 Mar 2016 17:47:53 +0100 Subject: [PATCH 4/4] Hopefully, this is the final nail on the coffin. --- middleware/fileserver.go | 30 ++++++++++-------------------- middleware/fileserver_test.go | 25 +++++++++++++++---------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/middleware/fileserver.go b/middleware/fileserver.go index 6cdb0ff5..a1efd945 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -122,7 +122,7 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st } // If file is on hide list. - if fh.isHidden(d.Name()) { + if fh.isHidden(d) { return http.StatusNotFound, nil } @@ -133,28 +133,18 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st return http.StatusOK, nil } -// isHidden checks if file with name is on hide list. -func (fh fileHandler) isHidden(name string) bool { - // Clean up on Windows. - // Remove trailing dots and trim whitespaces. - if runtimeGoos == "windows" { - name = strings.TrimSpace(name) - for strings.HasSuffix(name, ".") { - name = name[:len(name)-1] - name = strings.TrimSpace(name) - } - } +// isHidden checks if file with FileInfo d is on hide list. +func (fh fileHandler) isHidden(d os.FileInfo) bool { // If the file is supposed to be hidden, return a 404 // (TODO: If the slice gets large, a set may be faster) for _, hiddenPath := range fh.hide { - // Case-insensitive file systems may have loaded "CaddyFile" when - // we think we got "Caddyfile", which poses a security risk if we - // aren't careful here: case-insensitive comparison is required! - // TODO: This matches file NAME only, regardless of path. In other - // words, trying to serve another file with the same name as the - // active config file will result in a 404 when it shouldn't. - if strings.EqualFold(name, filepath.Base(hiddenPath)) { - return true + // Check if the served file is exactly the hidden file. + if hFile, err := fh.root.Open(hiddenPath); err == nil { + fs, _ := hFile.Stat() + hFile.Close() + if os.SameFile(d, fs) { + return true + } } } return false diff --git a/middleware/fileserver_test.go b/middleware/fileserver_test.go index ba2f23ba..c9011238 100644 --- a/middleware/fileserver_test.go +++ b/middleware/fileserver_test.go @@ -35,7 +35,7 @@ func TestServeHTTP(t *testing.T) { beforeServeHTTPTest(t) defer afterServeHTTPTest(t) - fileserver := FileServer(http.Dir(testDir), []string{"hidden.html"}) + fileserver := FileServer(http.Dir(testDir), []string{"dir/hidden.html"}) movedPermanently := "Moved Permanently" @@ -84,54 +84,59 @@ func TestServeHTTP(t *testing.T) { expectedStatus: http.StatusMovedPermanently, expectedBodyContent: movedPermanently, }, - // Test 6 - access file with trailing slash + // Test 7 - access file with trailing slash { url: "https://foo/file1.html/", expectedStatus: http.StatusMovedPermanently, expectedBodyContent: movedPermanently, }, - // Test 7 - access not existing path + // Test 8 - access not existing path { url: "https://foo/not_existing", expectedStatus: http.StatusNotFound, }, - // Test 8 - access a file, marked as hidden + // Test 9 - access a file, marked as hidden { url: "https://foo/dir/hidden.html", expectedStatus: http.StatusNotFound, }, - // Test 9 - access a index file directly + // Test 10 - access a index file directly { url: "https://foo/dirwithindex/index.html", expectedStatus: http.StatusOK, expectedBodyContent: testFiles[filepath.Join("dirwithindex", "index.html")], }, - // Test 10 - send a request with query params + // Test 11 - send a request with query params { url: "https://foo/dir?param1=val", expectedStatus: http.StatusMovedPermanently, expectedBodyContent: movedPermanently, }, - // Test 11 - attempt to bypass hidden file + // Test 12 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html%20", expectedStatus: http.StatusNotFound, }, - // Test 12 - attempt to bypass hidden file + // Test 13 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html.", expectedStatus: http.StatusNotFound, }, - // Test 13 - attempt to bypass hidden file + // Test 14 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html.%20", expectedStatus: http.StatusNotFound, }, - // Test 14 - attempt to bypass hidden file + // Test 15 - attempt to bypass hidden file { url: "https://foo/dir/hidden.html%20.", expectedStatus: http.StatusNotFound, }, + // Test 16 - serve another file with same name as hidden file. + { + url: "https://foo/hidden.html", + expectedStatus: http.StatusNotFound, + }, } for i, test := range tests {