From 8570f82c8b17c70b538e2c133ff2f68893e58bb8 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 19 Apr 2021 21:57:08 -0700 Subject: [PATCH] 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 --- ipn/ipnlocal/peerapi.go | 59 ++++++++++- ipn/ipnlocal/peerapi_test.go | 197 ++++++++++++++++++++++++++++++++++- 2 files changed, 251 insertions(+), 5 deletions(-) diff --git a/ipn/ipnlocal/peerapi.go b/ipn/ipnlocal/peerapi.go index d8376e51e..9d3399291 100644 --- a/ipn/ipnlocal/peerapi.go +++ b/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) diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 40b9e14fd..4285fd5d9 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/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()) + } + } + } }) } }