net/http: add protections against misuse of ServeFile

Martin Lenord pointed out that bad patterns have emerged in online
examples of how to use ServeFile, where people pass r.URL.Path[1:] to
ServeFile. This is unsafe. Document that it's unsafe, and add some
protections.

Fixes #14110

Change-Id: Ifeaa15534b2b3e46d3a8137be66748afa8fcd634
Reviewed-on: https://go-review.googlesource.com/18939
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index c41d001d8f12e12c64753d4e4cd3573f6f3210f1..f61c138c1d944390f3fc1f29d9e7d382f1e6e601 100644 (file)
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -451,15 +451,44 @@ func localRedirect(w ResponseWriter, r *Request, newPath string) {
 // ServeFile replies to the request with the contents of the named
 // file or directory.
 //
+// If the provided file or direcory name is a relative path, it is
+// interpreted relative to the current directory and may ascend to parent
+// directories. If the provided name is constructed from user input, it
+// should be sanitized before calling ServeFile. As a precaution, ServeFile
+// will reject requests where r.URL.Path contains a ".." path element.
+//
 // As a special case, ServeFile redirects any request where r.URL.Path
 // ends in "/index.html" to the same path, without the final
 // "index.html". To avoid such redirects either modify the path or
 // use ServeContent.
 func ServeFile(w ResponseWriter, r *Request, name string) {
+       if containsDotDot(r.URL.Path) {
+               // Too many programs use r.URL.Path to construct the argument to
+               // serveFile. Reject the request under the assumption that happened
+               // here and ".." may not be wanted.
+               // Note that name might not contain "..", for example if code (still
+               // incorrectly) used filepath.Join(myDir, r.URL.Path).
+               Error(w, "invalid URL path", StatusBadRequest)
+               return
+       }
        dir, file := filepath.Split(name)
        serveFile(w, r, Dir(dir), file, false)
 }
 
+func containsDotDot(v string) bool {
+       if !strings.Contains(v, "..") {
+               return false
+       }
+       for _, ent := range strings.FieldsFunc(v, isSlashRune) {
+               if ent == ".." {
+                       return true
+               }
+       }
+       return false
+}
+
+func isSlashRune(r rune) bool { return r == '/' || r == '\\' }
+
 type fileHandler struct {
        root FileSystem
 }

diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index 2e17d3f4bb675f8b85f35164663233a92871a69c..69d78066cd60560e8b0e2f3802a32e01e61c9793 100644 (file)
--- a/src/net/http/fs_test.go
+++ b/src/net/http/fs_test.go
@@ -5,6 +5,7 @@
 package http_test
 
 import (
+       "bufio"
        "bytes"
        "errors"
        "fmt"
@@ -177,6 +178,36 @@ Cases:
        }
 }
 
+func TestServeFile_DotDot(t *testing.T) {
+       tests := []struct {
+               req        string
+               wantStatus int
+       }{
+               {"/testdata/file", 200},
+               {"/../file", 400},
+               {"/..", 400},
+               {"/../", 400},
+               {"/../foo", 400},
+               {"/..\\foo", 400},
+               {"/file/a", 200},
+               {"/file/a..", 200},
+               {"/file/a/..", 400},
+               {"/file/a\\..", 400},
+       }
+       for _, tt := range tests {
+               req, err := ReadRequest(bufio.NewReader(strings.NewReader("GET " + tt.req + " HTTP/1.1\r\nHost: foo\r\n\r\n")))
+               if err != nil {
+                       t.Errorf("bad request %q: %v", tt.req, err)
+                       continue
+               }
+               rec := httptest.NewRecorder()
+               ServeFile(rec, req, "testdata/file")
+               if rec.Code != tt.wantStatus {
+                       t.Errorf("for request %q, status = %d; want %d", tt.req, rec.Code, tt.wantStatus)
+               }
+       }
+}
+
 var fsRedirectTestData = []struct {
        original, redirect string
 }{