From 4a6121f9891d608137eacbf446b2c9d1f826ec27 Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Sat, 16 Apr 2016 18:57:16 +0200 Subject: [PATCH 1/3] Tell usage of 'path' from 'filepath' and fix *path checking --- caddy/restart.go | 4 +- caddy/setup/ext.go | 3 +- middleware/browse/browse.go | 248 ++++++++++++++++++---------------- middleware/fileserver.go | 13 +- middleware/fileserver_test.go | 47 +++++-- server/server.go | 17 ++- 6 files changed, 183 insertions(+), 149 deletions(-) diff --git a/caddy/restart.go b/caddy/restart.go index 28b79d7a9..717bd3a32 100644 --- a/caddy/restart.go +++ b/caddy/restart.go @@ -11,7 +11,7 @@ import ( "net" "os" "os/exec" - "path" + "path/filepath" "sync/atomic" "github.com/mholt/caddy/caddy/https" @@ -138,7 +138,7 @@ func Restart(newCaddyfile Input) error { func getCertsForNewCaddyfile(newCaddyfile Input) error { // parse the new caddyfile only up to (and including) TLS // so we can know what we need to get certs for. - configs, _, _, err := loadConfigsUpToIncludingTLS(path.Base(newCaddyfile.Path()), bytes.NewReader(newCaddyfile.Body())) + configs, _, _, err := loadConfigsUpToIncludingTLS(filepath.Base(newCaddyfile.Path()), bytes.NewReader(newCaddyfile.Body())) if err != nil { return errors.New("loading Caddyfile: " + err.Error()) } diff --git a/caddy/setup/ext.go b/caddy/setup/ext.go index 4495da664..bfd4cba55 100644 --- a/caddy/setup/ext.go +++ b/caddy/setup/ext.go @@ -2,6 +2,7 @@ package setup import ( "os" + "path/filepath" "github.com/mholt/caddy/middleware" "github.com/mholt/caddy/middleware/extensions" @@ -47,7 +48,7 @@ func extParse(c *Controller) ([]string, error) { // resourceExists returns true if the file specified at // root + path exists; false otherwise. func resourceExists(root, path string) bool { - _, err := os.Stat(root + path) + _, err := os.Stat(filepath.Join(root, path)) // technically we should use os.IsNotExist(err) // but we don't handle any other kinds of errors anyway return err == nil diff --git a/middleware/browse/browse.go b/middleware/browse/browse.go index 80ae8881e..0a874ec64 100644 --- a/middleware/browse/browse.go +++ b/middleware/browse/browse.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path" + "path/filepath" "sort" "strconv" "strings" @@ -173,9 +174,11 @@ func (l Listing) applySort() { } func directoryListing(files []os.FileInfo, r *http.Request, canGoUp bool, root string, ignoreIndexes bool, vars interface{}) (Listing, error) { - var fileinfos []FileInfo - var dirCount, fileCount int - var urlPath = r.URL.Path + var ( + fileinfos []FileInfo + dirCount, fileCount int + urlPath = r.URL.Path + ) for _, f := range files { name := f.Name() @@ -226,143 +229,150 @@ func directoryListing(files []os.FileInfo, r *http.Request, canGoUp bool, root s // ServeHTTP implements the middleware.Handler interface. func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { - filename := b.Root + r.URL.Path - info, err := os.Stat(filename) - if err != nil { + var bc *Config + // See if there's a browse configuration to match the path + for i := range b.Configs { + if middleware.Path(r.URL.Path).Matches(b.Configs[i].PathScope) { + bc = &b.Configs[i] + break + } + } + if bc == nil { return b.Next.ServeHTTP(w, r) } + // Browse works on existing directories; delegate everything else + requestedFilepath := filepath.Join(b.Root, r.URL.Path) + info, err := os.Stat(requestedFilepath) + if err != nil { + switch { + case os.IsPermission(err): + return http.StatusForbidden, err + case os.IsExist(err): + return http.StatusNotFound, err + default: + return b.Next.ServeHTTP(w, r) + } + } if !info.IsDir() { return b.Next.ServeHTTP(w, r) } - // See if there's a browse configuration to match the path - for _, bc := range b.Configs { - if !middleware.Path(r.URL.Path).Matches(bc.PathScope) { - continue - } - switch r.Method { - case http.MethodGet, http.MethodHead: - default: - return http.StatusMethodNotAllowed, nil - } + // Do not reply to anything else because it might be nonsensical + switch r.Method { + case http.MethodGet, http.MethodHead: + default: + return http.StatusMethodNotAllowed, nil + } - // Browsing navigation gets messed up if browsing a directory - // that doesn't end in "/" (which it should, anyway) - if r.URL.Path[len(r.URL.Path)-1] != '/' { - http.Redirect(w, r, r.URL.Path+"/", http.StatusTemporaryRedirect) - return 0, nil - } + // Browsing navigation gets messed up if browsing a directory + // that doesn't end in "/" (which it should, anyway) + if !strings.HasSuffix(r.URL.Path, "/") { + http.Redirect(w, r, r.URL.Path+"/", http.StatusTemporaryRedirect) + return 0, nil + } - // Load directory contents - file, err := os.Open(b.Root + r.URL.Path) - if err != nil { - if os.IsPermission(err) { - return http.StatusForbidden, err - } - return http.StatusNotFound, err - } - defer file.Close() - - files, err := file.Readdir(-1) - if err != nil { + // Load directory contents + file, err := os.Open(requestedFilepath) + if err != nil { + if os.IsPermission(err) { return http.StatusForbidden, err } + return http.StatusInternalServerError, err + } + defer file.Close() - // Determine if user can browse up another folder - var canGoUp bool - curPath := strings.TrimSuffix(r.URL.Path, "/") - for _, other := range b.Configs { - if strings.HasPrefix(path.Dir(curPath), other.PathScope) { - canGoUp = true - break + files, err := file.Readdir(-1) + if err != nil { + if os.IsPermission(err) { + return http.StatusForbidden, err + } + return http.StatusInternalServerError, err + } + + // Determine if user can browse up another folder + var canGoUp bool + curPath := strings.TrimSuffix(r.URL.Path, "/") + for _, other := range b.Configs { + if strings.HasPrefix(path.Dir(curPath), other.PathScope) { + canGoUp = true + break + } + } + // Assemble listing of directory contents + listing, err := directoryListing(files, r, canGoUp, b.Root, b.IgnoreIndexes, bc.Variables) + if err != nil { // directory isn't browsable + return http.StatusInternalServerError, err + } + + // Copy the query values into the Listing struct + listing.Sort, listing.Order = r.URL.Query().Get("sort"), r.URL.Query().Get("order") + + // If the query 'sort' or 'order' is empty, use defaults or any values previously saved in Cookies + if listing.Sort == "" { + listing.Sort = "name" + if sortCookie, sortErr := r.Cookie("sort"); sortErr == nil { + listing.Sort = sortCookie.Value + } + } else { // Save the query value of 'sort' and 'order' as cookies. + http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: "/"}) + http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) + } + + if listing.Order == "" { + listing.Order = "asc" + if orderCookie, orderErr := r.Cookie("order"); orderErr == nil { + listing.Order = orderCookie.Value + } + } else { + http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) + } + + listing.applySort() + + var buf bytes.Buffer + // Check if we should provide json + acceptHeader := strings.Join(r.Header["Accept"], ",") + if strings.Contains(strings.ToLower(acceptHeader), "application/json") { + var marsh []byte + // Check if we are limited + if limitQuery := r.URL.Query().Get("limit"); limitQuery != "" { + limit, err := strconv.Atoi(limitQuery) + if err != nil { // if the 'limit' query can't be interpreted as a number, return err + return http.StatusBadRequest, err } - } - // Assemble listing of directory contents - listing, err := directoryListing(files, r, canGoUp, b.Root, b.IgnoreIndexes, bc.Variables) - if err != nil { // directory isn't browsable - continue - } - - // Get the query vales and store them in the Listing struct - listing.Sort, listing.Order = r.URL.Query().Get("sort"), r.URL.Query().Get("order") - - // If the query 'sort' or 'order' is empty, check the cookies - if listing.Sort == "" { - sortCookie, sortErr := r.Cookie("sort") - // if there's no sorting values in the cookies, default to "name" and "asc" - if sortErr != nil { - listing.Sort = "name" - } else { // if we have values in the cookies, use them - listing.Sort = sortCookie.Value - } - } else { // save the query value of 'sort' and 'order' as cookies - http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: "/"}) - http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) - } - - if listing.Order == "" { - orderCookie, orderErr := r.Cookie("order") - // if there's no sorting values in the cookies, default to "name" and "asc" - if orderErr != nil { - listing.Order = "asc" - } else { // if we have values in the cookies, use them - listing.Order = orderCookie.Value - } - } else { // save the query value of 'sort' and 'order' as cookies - http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) - } - - // Apply the sorting - listing.applySort() - - var buf bytes.Buffer - // check if we should provide json - acceptHeader := strings.Join(r.Header["Accept"], ",") - if strings.Contains(strings.ToLower(acceptHeader), "application/json") { - var marsh []byte - // check if we are limited - if limitQuery := r.URL.Query().Get("limit"); limitQuery != "" { - limit, err := strconv.Atoi(limitQuery) - if err != nil { // if the 'limit' query can't be interpreted as a number, return err - return http.StatusBadRequest, err - } - // if `limit` is equal or less than len(listing.Items) and bigger than 0, list them - if limit <= len(listing.Items) && limit > 0 { - marsh, err = json.Marshal(listing.Items[:limit]) - } else { // if the 'limit' query is empty, or has the wrong value, list everything - marsh, err = json.Marshal(listing.Items) - } - if err != nil { - return http.StatusInternalServerError, err - } - } else { // there's no 'limit' query, list them all + // if `limit` is equal or less than len(listing.Items) and bigger than 0, list them + if limit <= len(listing.Items) && limit > 0 { + marsh, err = json.Marshal(listing.Items[:limit]) + } else { // if the 'limit' query is empty, or has the wrong value, list everything marsh, err = json.Marshal(listing.Items) - if err != nil { - return http.StatusInternalServerError, err - } } - - // write the marshaled json to buf - if _, err = buf.Write(marsh); err != nil { - return http.StatusInternalServerError, err - } - w.Header().Set("Content-Type", "application/json; charset=utf-8") - - } else { // there's no 'application/json' in the 'Accept' header, browse normally - err = bc.Template.Execute(&buf, listing) if err != nil { return http.StatusInternalServerError, err } - w.Header().Set("Content-Type", "text/html; charset=utf-8") - + } else { // There's no 'limit' query; list them all + marsh, err = json.Marshal(listing.Items) + if err != nil { + return http.StatusInternalServerError, err + } } - buf.WriteTo(w) + // Write the marshaled json to buf + if _, err = buf.Write(marsh); err != nil { + return http.StatusInternalServerError, err + } + w.Header().Set("Content-Type", "application/json; charset=utf-8") + + } else { // There's no 'application/json' in the 'Accept' header; browse normally + err = bc.Template.Execute(&buf, listing) + if err != nil { + return http.StatusInternalServerError, err + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") - return http.StatusOK, nil } - // Didn't qualify; pass-thru - return b.Next.ServeHTTP(w, r) + buf.WriteTo(w) + + return http.StatusOK, nil } diff --git a/middleware/fileserver.go b/middleware/fileserver.go index cba4da09c..cb619dc54 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -40,12 +40,11 @@ type fileHandler struct { } func (fh *fileHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { - upath := r.URL.Path - if !strings.HasPrefix(upath, "/") { - upath = "/" + upath - r.URL.Path = upath + // r.URL.Path has already been cleaned in caddy/server by path.Clean(). + if r.URL.Path == "" { + r.URL.Path = "/" } - return fh.serveFile(w, r, path.Clean(upath)) + return fh.serveFile(w, r, r.URL.Path) } // serveFile writes the specified file to the HTTP response. @@ -86,13 +85,13 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st url := r.URL.Path if d.IsDir() { // Ensure / at end of directory url - if url[len(url)-1] != '/' { + if !strings.HasSuffix(url, "/") { redirect(w, r, path.Base(url)+"/") return http.StatusMovedPermanently, nil } } else { // Ensure no / at end of file url - if url[len(url)-1] == '/' { + if strings.HasSuffix(url, "/") { redirect(w, r, "../"+path.Base(url)) return http.StatusMovedPermanently, nil } diff --git a/middleware/fileserver_test.go b/middleware/fileserver_test.go index 96292bdc9..40d369a8b 100644 --- a/middleware/fileserver_test.go +++ b/middleware/fileserver_test.go @@ -4,6 +4,7 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -11,23 +12,30 @@ import ( "time" ) -var testDir = filepath.Join(os.TempDir(), "caddy_testdir") -var ErrCustom = errors.New("Custom Error") +var ( + ErrCustom = errors.New("Custom Error") + + testDir = filepath.Join(os.TempDir(), "caddy_testdir") + testWebRoot = filepath.Join(testDir, "webroot") +) // testFiles is a map with relative paths to test files as keys and file content as values. // The map represents the following structure: // - $TEMP/caddy_testdir/ -// '-- file1.html -// '-- dirwithindex/ -// '---- index.html -// '-- dir/ -// '---- file2.html -// '---- hidden.html +// '-- unreachable.html +// '-- webroot/ +// '---- file1.html +// '---- dirwithindex/ +// '------ index.html +// '---- dir/ +// '------ file2.html +// '------ hidden.html var testFiles = map[string]string{ - "file1.html": "

file1.html

", - filepath.Join("dirwithindex", "index.html"): "

dirwithindex/index.html

", - filepath.Join("dir", "file2.html"): "

dir/file2.html

", - filepath.Join("dir", "hidden.html"): "

dir/hidden.html

", + "unreachable.html": "

must not leak

", + filepath.Join("webroot", "file1.html"): "

file1.html

", + filepath.Join("webroot", "dirwithindex", "index.html"): "

dirwithindex/index.html

", + filepath.Join("webroot", "dir", "file2.html"): "

dir/file2.html

", + filepath.Join("webroot", "dir", "hidden.html"): "

dir/hidden.html

", } // TestServeHTTP covers positive scenarios when serving files. @@ -36,7 +44,7 @@ func TestServeHTTP(t *testing.T) { beforeServeHTTPTest(t) defer afterServeHTTPTest(t) - fileserver := FileServer(http.Dir(testDir), []string{"dir/hidden.html"}) + fileserver := FileServer(http.Dir(testWebRoot), []string{"dir/hidden.html"}) movedPermanently := "Moved Permanently" @@ -142,11 +150,20 @@ func TestServeHTTP(t *testing.T) { url: "https://foo/hidden.html", expectedStatus: http.StatusNotFound, }, + // Test 17 - try to get below the root directory. + { + url: "https://foo/%2f..%2funreachable.html", + expectedStatus: http.StatusNotFound, + }, } for i, test := range tests { responseRecorder := httptest.NewRecorder() - request, err := http.NewRequest("GET", test.url, strings.NewReader("")) + request, err := http.NewRequest("GET", test.url, nil) + // prevent any URL sanitization within Go: we need unmodified paths here + if u, _ := url.Parse(test.url); u.RawPath != "" { + request.URL.Path = u.RawPath + } status, err := fileserver.ServeHTTP(responseRecorder, request) etag := responseRecorder.Header().Get("Etag") @@ -176,7 +193,7 @@ func TestServeHTTP(t *testing.T) { // beforeServeHTTPTest creates a test directory with the structure, defined in the variable testFiles func beforeServeHTTPTest(t *testing.T) { // make the root test dir - err := os.Mkdir(testDir, os.ModePerm) + err := os.MkdirAll(testWebRoot, os.ModePerm) if err != nil { if !os.IsExist(err) { t.Fatalf("Failed to create test dir. Error was: %v", err) diff --git a/server/server.go b/server/server.go index 41d1a6373..684d6151a 100644 --- a/server/server.go +++ b/server/server.go @@ -14,7 +14,7 @@ import ( "net" "net/http" "os" - "path/filepath" + "path" "runtime" "strings" "sync" @@ -336,11 +336,18 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Use URL.RawPath If you need the original, "raw" URL.Path in your middleware. // Collapse any ./ ../ /// madness here instead of doing that in every plugin. if r.URL.Path != "/" { - path := filepath.Clean(r.URL.Path) - if !strings.HasPrefix(path, "/") { - path = "/" + path + cleanedPath := path.Clean(r.URL.Path) + if cleanedPath == "." { + r.URL.Path = "/" + } else { + if !strings.HasPrefix(cleanedPath, "/") { + cleanedPath = "/" + cleanedPath + } + if strings.HasSuffix(r.URL.Path, "/") && !strings.HasSuffix(cleanedPath, "/") { + cleanedPath = cleanedPath + "/" + } + r.URL.Path = cleanedPath } - r.URL.Path = path } // Execute the optional request callback if it exists and it's not disabled From 3513b6f2f7f9008458c101ffc98d6da607416428 Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Sat, 16 Apr 2016 20:59:24 +0200 Subject: [PATCH 2/3] fileserver: When out of filedescriptors, spread retry attempts --- middleware/fileserver.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/middleware/fileserver.go b/middleware/fileserver.go index cb619dc54..b1c3d66d5 100644 --- a/middleware/fileserver.go +++ b/middleware/fileserver.go @@ -2,10 +2,12 @@ package middleware import ( "fmt" + "math/rand" "net/http" "os" "path" "path/filepath" + "strconv" "strings" ) @@ -65,7 +67,8 @@ func (fh *fileHandler) serveFile(w http.ResponseWriter, r *http.Request, name st return http.StatusForbidden, err } // Likely the server is under load and ran out of file descriptors - w.Header().Set("Retry-After", "5") // TODO: 5 seconds enough delay? Or too much? + backoff := int(3 + rand.Int31()%3) // 3–5 seconds to prevent a stampede + w.Header().Set("Retry-After", strconv.Itoa(backoff)) return http.StatusServiceUnavailable, err } defer f.Close() From c05c5163e293c92f7e5562df558fe955c7075f7a Mon Sep 17 00:00:00 2001 From: W-Mark Kubacki Date: Sat, 16 Apr 2016 21:37:06 +0200 Subject: [PATCH 3/3] browse: Don't leak Cookies to sessions in HTTP from HTTPS --- middleware/browse/browse.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/middleware/browse/browse.go b/middleware/browse/browse.go index 0a874ec64..8caed54c8 100644 --- a/middleware/browse/browse.go +++ b/middleware/browse/browse.go @@ -315,8 +315,8 @@ func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { listing.Sort = sortCookie.Value } } else { // Save the query value of 'sort' and 'order' as cookies. - http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: "/"}) - http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) + http.SetCookie(w, &http.Cookie{Name: "sort", Value: listing.Sort, Path: bc.PathScope, Secure: r.TLS != nil}) + http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: bc.PathScope, Secure: r.TLS != nil}) } if listing.Order == "" { @@ -325,7 +325,7 @@ func (b Browse) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { listing.Order = orderCookie.Value } } else { - http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: "/"}) + http.SetCookie(w, &http.Cookie{Name: "order", Value: listing.Order, Path: bc.PathScope, Secure: r.TLS != nil}) } listing.applySort()