ipn/ipnlocal: finish/fix up filename validation & encoding on disk

It used to just store received files URL-escaped on disk, but that was
a half done lazy implementation, and pushed the burden to callers to
validate and write things to disk in an unescaped way.

Instead, do all the validation in the receive handler and only
accept filenames that are UTF-8 and in the intersection of valid
names that all platforms support.

Fixes tailscale/corp#1594

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

ipn/ipnlocal/peerapi.go

@@ -22,6 +22,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+	"unicode"
+	"unicode/utf8"
 
 	"inet.af/netaddr"
 	"tailscale.com/client/tailscale/apitype"
@@ -50,15 +52,46 @@ type peerAPIServer struct {
 
 const partialSuffix = ".partial"
 
+func validFilenameRune(r rune) bool {
+	switch r {
+	case '/':
+		return false
+	case '\\', ':', '*', '"', '<', '>', '|':
+		// Invalid stuff on Windows, but we reject them everywhere
+		// for now.
+		// TODO(bradfitz): figure out a better plan. We initially just
+		// wrote things to disk URL path-escaped, but that's gross
+		// when debugging, and just moves the problem to callers.
+		// So now we put the UTF-8 filenames on disk directly as
+		// sent.
+		return false
+	}
+	return unicode.IsPrint(r)
+}
+
 func (s *peerAPIServer) diskPath(baseName string) (fullPath string, ok bool) {
+	if !utf8.ValidString(baseName) {
+		return "", false
+	}
+	if strings.TrimSpace(baseName) != baseName {
+		return "", false
+	}
+	if len(baseName) > 255 {
+		return "", false
+	}
+	// TODO: validate unicode normalization form too? Varies by platform.
 	clean := path.Clean(baseName)
 	if clean != baseName ||
-		clean == "." ||
-		strings.ContainsAny(clean, `/\`) ||
+		clean == "." || clean == ".." ||
 		strings.HasSuffix(clean, partialSuffix) {
 		return "", false
 	}
-	return filepath.Join(s.rootDir, strings.ReplaceAll(url.PathEscape(baseName), ":", "%3a")), true
+	for _, r := range baseName {
+		if !validFilenameRune(r) {
+			return "", false
+		}
+	}
+	return filepath.Join(s.rootDir, baseName), true
 }
 
 // hasFilesWaiting reports whether any files are buffered in the
@@ -420,7 +453,25 @@ func (h *peerAPIHandler) handlePeerPut(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "no rootdir", http.StatusInternalServerError)
 		return
 	}
-	baseName := path.Base(r.URL.Path)
+	rawPath := r.URL.EscapedPath()
+	suffix := strings.TrimPrefix(rawPath, "/v0/put/")
+	if suffix == rawPath {
+		http.Error(w, "misconfigured internals", 500)
+		return
+	}
+	if suffix == "" {
+		http.Error(w, "empty filename", 400)
+		return
+	}
+	if strings.Contains(suffix, "/") {
+		http.Error(w, "directories not supported", 400)
+		return
+	}
+	baseName, err := url.PathUnescape(suffix)
+	if err != nil {
+		http.Error(w, "bad path encoding", 400)
+		return
+	}
 	dstFile, ok := h.ps.diskPath(baseName)
 	if !ok {
 		http.Error(w, "bad filename", 400)

ipn/ipnlocal/peerapi_test.go

@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/fs"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -50,6 +51,14 @@ func bodyContains(sub string) check {
 	}
 }
 
+func bodyNotContains(sub string) check {
+	return func(t *testing.T, e *peerAPITestEnv) {
+		if body := e.rr.Body.String(); strings.Contains(body, sub) {
+			t.Errorf("HTTP response body unexpectedly contains %q; got: %s", sub, body)
+		}
+	}
+}
+
 func fileHasSize(name string, size int) check {
 	return func(t *testing.T, e *peerAPITestEnv) {
 		root := e.ph.ps.rootDir
@@ -85,6 +94,14 @@ func fileHasContents(name string, want string) check {
 	}
 }
 
+func hexAll(v string) string {
+	var sb strings.Builder
+	for i := 0; i < len(v); i++ {
+		fmt.Fprintf(&sb, "%%%02x", v[i])
+	}
+	return sb.String()
+}
+
 func TestHandlePeerPut(t *testing.T) {
 	tests := []struct {
 		name       string
@@ -94,6 +111,28 @@ func TestHandlePeerPut(t *testing.T) {
 		req        *http.Request
 		checks     []check
 	}{
+		{
+			name:       "not_peer_api",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("GET", "/", nil),
+			checks: checks(
+				httpStatus(200),
+				bodyContains("This is my Tailscale device."),
+				bodyContains("You are the owner of this node."),
+			),
+		},
+		{
+			name:       "not_peer_api_not_owner",
+			isSelf:     false,
+			capSharing: true,
+			req:        httptest.NewRequest("GET", "/", nil),
+			checks: checks(
+				httpStatus(200),
+				bodyContains("This is my Tailscale device."),
+				bodyNotContains("You are the owner of this node."),
+			),
+		},
 		{
 			name:       "reject_non_owner_put",
 			isSelf:     false,
@@ -191,6 +230,148 @@ func TestHandlePeerPut(t *testing.T) {
 				bodyContains("bad filename"),
 			),
 		},
+		{
+			name:       "bad_filename_empty",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/", nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("empty filename"),
+			),
+		},
+		{
+			name:       "bad_filename_slash",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/foo/bar", nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("directories not supported"),
+			),
+		},
+		{
+			name:       "bad_filename_encoded_dot",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("."), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "bad_filename_encoded_slash",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("/"), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "bad_filename_encoded_backslash",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("\\"), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "bad_filename_encoded_dotdot",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll(".."), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "bad_filename_encoded_dotdot_out",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("foo/../../../../../etc/passwd"), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "put_spaces_and_caps",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("Foo Bar.dat"), strings.NewReader("baz")),
+			checks: checks(
+				httpStatus(200),
+				bodyContains("{}"),
+				fileHasContents("Foo Bar.dat", "baz"),
+			),
+		},
+		{
+			name:       "put_unicode",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("Томас и его друзья.mp3"), strings.NewReader("главный озорник")),
+			checks: checks(
+				httpStatus(200),
+				bodyContains("{}"),
+				fileHasContents("Томас и его друзья.mp3", "главный озорник"),
+			),
+		},
+		{
+			name:       "put_invalid_utf8",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+(hexAll("😜")[:3]), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "put_invalid_null",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/%00", nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "put_invalid_non_printable",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/%01", nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "put_invalid_colon",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll("nul:"), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
+		{
+			name:       "put_invalid_surrounding_whitespace",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/"+hexAll(" foo "), nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -216,14 +397,28 @@ func TestHandlePeerPut(t *testing.T) {
 					b: lb,
 				},
 			}
+			var rootDir string
 			if !tt.omitRoot {
-				e.ph.ps.rootDir = t.TempDir()
+				rootDir = t.TempDir()
+				e.ph.ps.rootDir = rootDir
 			}
 			e.rr = httptest.NewRecorder()
 			e.ph.ServeHTTP(e.rr, tt.req)
 			for _, f := range tt.checks {
 				f(t, &e)
 			}
+			if t.Failed() && rootDir != "" {
+				t.Logf("Contents of %s:", rootDir)
+				des, _ := fs.ReadDir(os.DirFS(rootDir), ".")
+				for _, de := range des {
+					fi, err := de.Info()
+					if err != nil {
+						t.Log(err)
+					} else {
+						t.Logf("  %v %5d %s", fi.Mode(), fi.Size(), de.Name())
+					}
+				}
+			}
 		})
 	}
 }