Merge 541cdd7267 into 96712e10a7

This prevents Mark-of-the-Web bypass attacks in case someone visits the
localhost WebDAV server directly.

Fixes tailscale/corp#19592

Signed-off-by: Percy Wegmann <percy@tailscale.com>

drive/driveimpl/compositedav/compositedav.go

@@ -27,10 +27,10 @@ import (
 type Child struct {
 	*dirfs.Child
 
-	// BaseURL is the base URL of the WebDAV service to which we'll proxy
+	// BaseURL returns the base URL of the WebDAV service to which we'll proxy
 	// requests for this Child. We will append the filename from the original
 	// URL to this.
-	BaseURL string
+	BaseURL func() (string, error)
 
 	// Transport (if specified) is the http transport to use when communicating
 	// with this Child's WebDAV service.
@@ -154,9 +154,15 @@ func (h *Handler) delegate(mpl int, pathComponents []string, w http.ResponseWrit
 		return
 	}
 
-	u, err := url.Parse(child.BaseURL)
+	baseURL, err := child.BaseURL()
 	if err != nil {
-		h.logf("warning: parse base URL %s failed: %s", child.BaseURL, err)
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+
+	u, err := url.Parse(baseURL)
+	if err != nil {
+		h.logf("warning: parse base URL %s failed: %s", baseURL, err)
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}

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"
@@ -112,7 +114,35 @@ func TestPermissions(t *testing.T) {
 	if err := s.client.Rename(pathTo(remote1, share12, file111), pathTo(remote1, share12, file112), true); err == nil {
 		t.Error("moving file on read-only remote should fail")
 	}
+}
 
+// TestSecretTokenAuth verifies that the fileserver running at localhost cannot
+// be accessed directly without the correct secret token. This matters because
+// if a victim can be induced to visit the localhost URL and access a malicious
+// file on their own share, it could allow a Mark-of-the-Web bypass attack.
+func TestSecretTokenAuth(t *testing.T) {
+	s := newSystem(t)
+
+	fileserverAddr := 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)
+
+	client := &http.Client{
+		Transport: &http.Transport{DisableKeepAlives: true},
+	}
+	addr := strings.Split(fileserverAddr, "|")[1]
+	wrongSecret, err := generateSecretToken()
+	if err != nil {
+		t.Fatal(err)
+	}
+	u := fmt.Sprintf("http://%s/%s/%s", addr, wrongSecret, url.PathEscape(file111))
+	resp, err := client.Get(u)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp.StatusCode != http.StatusForbidden {
+		t.Errorf("expected %d for incorrect secret token, but got %d", http.StatusForbidden, resp.StatusCode)
+	}
 }
 
 type local struct {
@@ -183,7 +213,7 @@ func newSystem(t *testing.T) *system {
 	return s
 }
 
-func (s *system) addRemote(name string) {
+func (s *system) addRemote(name string) string {
 	l, err := net.Listen("tcp", "127.0.0.1:0")
 	if err != nil {
 		s.t.Fatalf("failed to Listen: %s", err)
@@ -222,6 +252,8 @@ func (s *system) addRemote(name string) {
 			DisableKeepAlives:     true,
 			ResponseHeaderTimeout: 5 * time.Second,
 		})
+
+	return fileServer.Addr()
 }
 
 func (s *system) addShare(remoteName, shareName string, permission drive.Permission) {

drive/driveimpl/fileserver.go

@@ -4,6 +4,10 @@
 package driveimpl
 
 import (
+	"crypto/rand"
+	"crypto/subtle"
+	"encoding/hex"
+	"fmt"
 	"net"
 	"net/http"
 	"sync"
@@ -17,6 +21,7 @@ import (
 // serve up files as an unprivileged user.
 type FileServer struct {
 	l             net.Listener
+	secretToken   string
 	shareHandlers map[string]http.Handler
 	sharesMu      sync.RWMutex
 }
@@ -41,18 +46,36 @@ func NewFileServer() (*FileServer, error) {
 	// TODO(oxtoacart): actually get safesocket working in more environments (MacOS Sandboxed, Windows, ???)
 	l, err := net.Listen("tcp", "127.0.0.1:0")
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("listen: %w", err)
 	}
 	// }
+
+	secretToken, err := generateSecretToken()
+	if err != nil {
+		return nil, err
+	}
+
 	return &FileServer{
 		l:             l,
+		secretToken:   secretToken,
 		shareHandlers: make(map[string]http.Handler),
 	}, nil
 }
 
-// Addr returns the address at which this FileServer is listening.
+// generateSecretToken generates a hex-encoded 256 bit secet.
+func generateSecretToken() (string, error) {
+	tokenBytes := make([]byte, 32)
+	_, err := rand.Read(tokenBytes)
+	if err != nil {
+		return "", fmt.Errorf("generateSecretToken: %w", err)
+	}
+	return hex.EncodeToString(tokenBytes), nil
+}
+
+// Addr returns the address at which this FileServer is listening. This
+// includes the secret token in front of the address, delimited by a pipe |.
 func (s *FileServer) Addr() string {
-	return s.l.Addr().String()
+	return fmt.Sprintf("%s|%s", s.secretToken, s.l.Addr().String())
 }
 
 // Serve() starts serving files and blocks until it encounters a fatal error.
@@ -95,11 +118,33 @@ func (s *FileServer) SetShares(shares map[string]string) {
 	}
 }
 
-// ServeHTTP implements the http.Handler interface.
+// ServeHTTP implements the http.Handler interface. This requires a secret
+// token in the path in order to prevent Mark-of-the-Web (MOTW) bypass attacks
+// of the below sort:
+//
+//  1. Attacker with write access to the share puts a malicious file via
+//     http://100.100.100.100:8080/<tailnet>/<machine>/</share>/bad.exe
+//  2. Attacker then induces victim to visit
+//     http://localhost:[PORT]/<share>/bad.exe
+//  3. Because that is loaded from localhost, it does not get the MOTW
+//     thereby bypasses some OS-level security.
+//
+// The path on this file server is actually not as above, but rather
+// http://localhost:[PORT]/<secretToken>/<share>/bad.exe. Unless the attacker
+// can discover the secretToken, the attacker cannot craft a localhost URL that
+// will work.
 func (s *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	parts := shared.CleanAndSplit(r.URL.Path)
-	r.URL.Path = shared.Join(parts[1:]...)
-	share := parts[0]
+
+	token := parts[0]
+	a, b := []byte(token), []byte(s.secretToken)
+	if subtle.ConstantTimeCompare(a, b) != 1 {
+		w.WriteHeader(http.StatusForbidden)
+		return
+	}
+
+	r.URL.Path = shared.Join(parts[2:]...)
+	share := parts[1]
 	s.sharesMu.RLock()
 	h, found := s.shareHandlers[share]
 	s.sharesMu.RUnlock()

drive/driveimpl/local_impl.go

@@ -77,7 +77,7 @@ func (s *FileSystemForLocal) SetRemotes(domain string, remotes []*drive.Remote,
 				Name:      remote.Name,
 				Available: remote.Available,
 			},
-			BaseURL:   remote.URL,
+			BaseURL:   func() (string, error) { return remote.URL, nil },
 			Transport: transport,
 		})
 	}

drive/driveimpl/remote_impl.go

@@ -51,17 +51,18 @@ type FileSystemForRemote struct {
 
 	// mu guards the below values. Acquire a write lock before updating any of
 	// them, acquire a read lock before reading any of them.
-	mu             sync.RWMutex
-	fileServerAddr string
-	shares         []*drive.Share
-	children       map[string]*compositedav.Child
-	userServers    map[string]*userServer
+	mu sync.RWMutex
+	// fileServerTokenAndAddr is the secretToken|fileserverAddress
+	fileServerTokenAndAddr string
+	shares                 []*drive.Share
+	children               map[string]*compositedav.Child
+	userServers            map[string]*userServer
 }
 
 // SetFileServerAddr implements drive.FileSystemForRemote.
 func (s *FileSystemForRemote) SetFileServerAddr(addr string) {
 	s.mu.Lock()
-	s.fileServerAddr = addr
+	s.fileServerTokenAndAddr = addr
 	s.mu.Unlock()
 }
 
@@ -113,11 +114,58 @@ func (s *FileSystemForRemote) SetShares(shares []*drive.Share) {
 }
 
 func (s *FileSystemForRemote) buildChild(share *drive.Share) *compositedav.Child {
+	getTokenAndAddr := func(shareName string) (string, string, error) {
+		s.mu.RLock()
+		var share *drive.Share
+		i, shareFound := slices.BinarySearchFunc(s.shares, shareName, func(s *drive.Share, name string) int {
+			return strings.Compare(s.Name, name)
+		})
+		if shareFound {
+			share = s.shares[i]
+		}
+		userServers := s.userServers
+		fileServerTokenAndAddr := s.fileServerTokenAndAddr
+		s.mu.RUnlock()
+
+		if !shareFound {
+			return "", "", fmt.Errorf("unknown share %v", shareName)
+		}
+
+		var tokenAndAddr string
+		if !drive.AllowShareAs() {
+			tokenAndAddr = fileServerTokenAndAddr
+		} else {
+			userServer, found := userServers[share.As]
+			if found {
+				userServer.mu.RLock()
+				tokenAndAddr = userServer.tokenAndAddr
+				userServer.mu.RUnlock()
+			}
+		}
+
+		if tokenAndAddr == "" {
+			return "", "", fmt.Errorf("unable to determine address for share %v", shareName)
+		}
+
+		parts := strings.Split(tokenAndAddr, "|")
+		if len(parts) != 2 {
+			return "", "", fmt.Errorf("invalid address for share %v", shareName)
+		}
+
+		return parts[0], parts[1], nil
+	}
+
 	return &compositedav.Child{
 		Child: &dirfs.Child{
 			Name: share.Name,
 		},
-		BaseURL: fmt.Sprintf("http://%v/%v", hex.EncodeToString([]byte(share.Name)), url.PathEscape(share.Name)),
+		BaseURL: func() (string, error) {
+			secretToken, _, err := getTokenAndAddr(share.Name)
+			if err != nil {
+				return "", err
+			}
+			return fmt.Sprintf("http://%s/%s/%s", hex.EncodeToString([]byte(share.Name)), secretToken, url.PathEscape(share.Name)), nil
+		},
 		Transport: &http.Transport{
 			Dial: func(_, shareAddr string) (net.Conn, error) {
 				shareNameHex, _, err := net.SplitHostPort(shareAddr)
@@ -132,36 +180,9 @@ func (s *FileSystemForRemote) buildChild(share *drive.Share) *compositedav.Child
 				}
 				shareName := string(shareNameBytes)
 
-				s.mu.RLock()
-				var share *drive.Share
-				i, shareFound := slices.BinarySearchFunc(s.shares, shareName, func(s *drive.Share, name string) int {
-					return strings.Compare(s.Name, name)
-				})
-				if shareFound {
-					share = s.shares[i]
-				}
-				userServers := s.userServers
-				fileServerAddr := s.fileServerAddr
-				s.mu.RUnlock()
-
-				if !shareFound {
-					return nil, fmt.Errorf("unknown share %v", shareName)
-				}
-
-				var addr string
-				if !drive.AllowShareAs() {
-					addr = fileServerAddr
-				} else {
-					userServer, found := userServers[share.As]
-					if found {
-						userServer.mu.RLock()
-						addr = userServer.addr
-						userServer.mu.RUnlock()
-					}
-				}
-
-				if addr == "" {
-					return nil, fmt.Errorf("unable to determine address for share %v", shareName)
+				_, addr, err := getTokenAndAddr(shareName)
+				if err != nil {
+					return nil, err
 				}
 
 				_, err = netip.ParseAddrPort(addr)
@@ -253,10 +274,10 @@ type userServer struct {
 
 	// mu guards the below values. Acquire a write lock before updating any of
 	// them, acquire a read lock before reading any of them.
-	mu     sync.RWMutex
-	cmd    *exec.Cmd
-	addr   string
-	closed bool
+	mu           sync.RWMutex
+	cmd          *exec.Cmd
+	tokenAndAddr string
+	closed       bool
 }
 
 func (s *userServer) Close() error {
@@ -366,7 +387,7 @@ func (s *userServer) run() error {
 		}
 	}()
 	s.mu.Lock()
-	s.addr = strings.TrimSpace(addr)
+	s.tokenAndAddr = strings.TrimSpace(addr)
 	s.mu.Unlock()
 	return cmd.Wait()
 }

drive/remote.go

@@ -95,6 +95,9 @@ type FileSystemForRemote interface {
 	// sandboxed where we can't spawn user-specific sub-processes and instead
 	// rely on the UI application that's already running as an unprivileged
 	// user to access the filesystem for us.
+	//
+	// Note that this includes both the file server's secret token and its
+	// address, delimited by a pipe |.
 	SetFileServerAddr(addr string)
 
 	// SetShares sets the complete set of shares exposed by this node. If