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>

2 files changed, 60 insertions, 0 deletions

diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index c41d001d8f..f61c138c1d 100644
--- 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 2e17d3f4bb..69d78066cd 100644
--- 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
 }{