drive/driveimpl: rewrite text/html Content-Type to text/plain

This prevents Taildrive from being able to serve HTML content, thereby preventing it from being used to distribute malicious JavaScript. Updates tailscale/corp#19592 Signed-off-by: Percy Wegmann <percy@tailscale.com>
2 weeks ago · 8fcac10029
parent 74c399483c
commit 8fcac10029
2 changed files with 69 additions and 9 deletions
--- a/drive/driveimpl/drive_test.go
+++ b/drive/driveimpl/drive_test.go
@ -10,10 +10,12 @@ import (
 	"log"
 	"net"
 	"net/http"
+	"net/url"
 	"os"
 	"path"
 	"path/filepath"
 	"slices"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@ -32,8 +34,10 @@ const (
 	remote2 = `_rem ote$%2`
 	share11 = `sha re$%11`
 	share12 = `_sha re$%12`
-	file111 = `fi le$%111.txt`
-	file112 = `file112.txt`
+	file111 = `fi le$%111.html`
+	file112 = `file112.html`
+
+	contents = "<html>hello world</html>"
 )

 func init() {
@ -56,7 +60,7 @@ func TestDirectoryListing(t *testing.T) {
 	s.checkDirList("remote with one share should contain that share", shared.Join(domain, remote1), share11)
 	s.addShare(remote1, share12, drive.PermissionReadOnly)
 	s.checkDirList("remote with two shares should contain both in lexicographical order", shared.Join(domain, remote1), share12, share11)
-	s.writeFile("writing file to read/write remote should succeed", remote1, share11, file111, "hello world", true)
+	s.writeFile("writing file to read/write remote should succeed", remote1, share11, file111, contents, true)
 	s.checkDirList("remote share should contain file", shared.Join(domain, remote1, share11), file111)

 	s.addRemote(remote2)
@ -78,9 +82,13 @@ func TestFileManipulation(t *testing.T) {

 	s.addRemote(remote1)
 	s.addShare(remote1, share11, drive.PermissionReadWrite)
-	s.writeFile("writing file to read/write remote should succeed", remote1, share11, file111, "hello world", true)
+	s.writeFile("writing file to read/write remote should succeed", remote1, share11, file111, contents, true)
 	s.checkFileStatus(remote1, share11, file111)
 	s.checkFileContents(remote1, share11, file111)
+	// Despite this being an HTML file, Content-Type should be text/plain to
+	// prevent user agents from treating this as an HTML page. This prevents
+	// Taildrive from being used to distribute malicious JavaScript.
+	s.checkContentType(remote1, share11, file111, "text/plain")

 	s.renameFile("renaming file across shares should fail", remote1, share11, file111, share12, file112, false)

@ -88,9 +96,9 @@ func TestFileManipulation(t *testing.T) {
 	s.checkFileContents(remote1, share11, file112)

 	s.addShare(remote1, share12, drive.PermissionReadOnly)
-	s.writeFile("writing file to read-only remote should fail", remote1, share12, file111, "hello world", false)
-	s.writeFile("writing file to non-existent remote should fail", "non-existent", share11, file111, "hello world", false)
-	s.writeFile("writing file to non-existent share should fail", remote1, "non-existent", file111, "hello world", false)
+	s.writeFile("writing file to read-only remote should fail", remote1, share12, file111, contents, false)
+	s.writeFile("writing file to non-existent remote should fail", "non-existent", share11, file111, contents, false)
+	s.writeFile("writing file to non-existent share should fail", remote1, "non-existent", file111, contents, false)
 }

 type local struct {
@ -124,6 +132,7 @@ func (r *remote) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 type system struct {
 	t       *testing.T
 	local   *local
+	baseURL string
 	client  *gowebdav.Client
 	remotes map[string]*remote
 }
@ -149,11 +158,13 @@ func newSystem(t *testing.T) *system {
 		}
 	}()

-	client := gowebdav.NewAuthClient(fmt.Sprintf("http://%s", l.Addr()), &noopAuthorizer{})
+	baseURL := fmt.Sprintf("http://%s", l.Addr())
+	client := gowebdav.NewAuthClient(baseURL, &noopAuthorizer{})
 	client.SetTransport(&http.Transport{DisableKeepAlives: true})
 	s := &system{
 		t:       t,
 		local:   &local{l: l, fs: fs},
+		baseURL: baseURL,
 		client:  client,
 		remotes: make(map[string]*remote),
 	}
@ -275,6 +286,21 @@ func (s *system) checkFileContents(remoteName, shareName, name string) {
 	}
 }

+func (s *system) checkContentType(remoteName, shareName, name, expectedContentType string) {
+	client := http.Client{
+		Transport: &http.Transport{DisableKeepAlives: true},
+	}
+	p := pathTo(remoteName, shareName, name)
+	resp, err := client.Head(fmt.Sprintf("%s/%s", s.baseURL, url.PathEscape(p)))
+	if err != nil {
+		s.t.Fatalf("unable to check content type: %s", err)
+	}
+	actual := resp.Header.Get("Content-Type")
+	if !strings.Contains(actual, expectedContentType) {
+		s.t.Errorf("%q has wrong Content-Type \nwant: %q\nhave: %q", p, expectedContentType, actual)
+	}
+}
+
 func (s *system) checkDirList(label string, path string, want ...string) {
 	got, err := s.client.ReadDir(path)
 	if err != nil {
--- a/drive/driveimpl/fileserver.go
+++ b/drive/driveimpl/fileserver.go
@ -6,6 +6,7 @@ package driveimpl
 import (
 	"net"
 	"net/http"
+	"strings"
 	"sync"

 	"github.com/tailscale/xnet/webdav"
@ -107,9 +108,42 @@ func (s *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusNotFound)
 		return
 	}
-	h.ServeHTTP(w, r)
+
+	// Rewrite text/html content type to text/plain, to prevent stored XSS
+	// vulnerabilities. This has the effect of not being able to serve HTML
+	// content via Taildrive, which is fine because this is a file sharing, not
+	// web serving, tool.
+	rw := &contentTypeRewritingResponseWriter{
+		ResponseWriter: w,
+	}
+	h.ServeHTTP(rw, r)
 }

 func (s *FileServer) Close() error {
 	return s.l.Close()
 }
+
+var contentTypeRewrites = map[string]string{
+	"text/html": "text/plain",
+}
+
+// contentTypeRewritingResponseWriter rewrites content types according to the
+// above contentTypeRewrites map.
+type contentTypeRewritingResponseWriter struct {
+	http.ResponseWriter
+}
+
+func (rw *contentTypeRewritingResponseWriter) Header() http.Header {
+	result := rw.ResponseWriter.Header()
+	contentTypes := result.Values("Content-Type")
+	if len(contentTypes) > 0 {
+		for i, contentType := range contentTypes {
+			for from, to := range contentTypeRewrites {
+				contentType = strings.ReplaceAll(contentType, from, to)
+			}
+			contentTypes[i] = contentType
+		}
+		result["Content-Type"] = contentTypes
+	}
+	return result
+}