From 9d22ec0ba27bef6dbb029273fa6e68c7894af01b Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Wed, 1 May 2024 10:45:57 -0500 Subject: [PATCH] drive: use secret token to authenticate access to file server on localhost 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 --- drive/driveimpl/compositedav/compositedav.go | 12 ++- drive/driveimpl/drive_test.go | 30 +++++- drive/driveimpl/fileserver.go | 36 +++++-- drive/driveimpl/local_impl.go | 2 +- drive/driveimpl/remote_impl.go | 105 +++++++++++-------- drive/remote.go | 3 + 6 files changed, 135 insertions(+), 53 deletions(-) diff --git a/drive/driveimpl/compositedav/compositedav.go b/drive/driveimpl/compositedav/compositedav.go index 024f8be3b..c83293691 100644 --- a/drive/driveimpl/compositedav/compositedav.go +++ b/drive/driveimpl/compositedav/compositedav.go @@ -27,10 +27,10 @@ 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,7 +154,13 @@ 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 { + 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", child.BaseURL, err) http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/drive/driveimpl/drive_test.go b/drive/driveimpl/drive_test.go index 66405648e..fe9986433 100644 --- a/drive/driveimpl/drive_test.go +++ b/drive/driveimpl/drive_test.go @@ -10,10 +10,12 @@ "log" "net" "net/http" + "net/url" "os" "path" "path/filepath" "slices" + "strings" "sync" "testing" "time" @@ -112,7 +114,31 @@ 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] + u := fmt.Sprintf("http://%s/%s/%s", addr, "fakesecret", 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 +209,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 +248,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) { diff --git a/drive/driveimpl/fileserver.go b/drive/driveimpl/fileserver.go index 73fbd37bb..5d9f183a7 100644 --- a/drive/driveimpl/fileserver.go +++ b/drive/driveimpl/fileserver.go @@ -4,6 +4,9 @@ package driveimpl import ( + "crypto/rand" + "encoding/hex" + "fmt" "net" "net/http" "sync" @@ -17,6 +20,7 @@ // 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 +45,27 @@ 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) } // } + + tokenBytes := make([]byte, 32) + _, err = rand.Read(tokenBytes) + if err != nil { + return nil, fmt.Errorf("generate token bytes: %w", err) + } + return &FileServer{ l: l, + secretToken: hex.EncodeToString(tokenBytes), shareHandlers: make(map[string]http.Handler), }, nil } -// Addr returns the address at which this FileServer is listening. +// 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 +108,22 @@ func (s *FileServer) SetShares(shares map[string]string) { } } -// ServeHTTP implements the http.Handler interface. +// ServeHTTP implements the http.Handler interface. In order to prevent +// Mark-of-the-Web bypass attacks if someone visits this fileserver directly +// within a browser, it requires that the first path element be this file +// server's secret token. Without knowing the secret token, it's impossible +// to construct a URL that passes validation. 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] + if token != s.secretToken { + 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() diff --git a/drive/driveimpl/local_impl.go b/drive/driveimpl/local_impl.go index 44aaac228..22936ceaa 100644 --- a/drive/driveimpl/local_impl.go +++ b/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, }) } diff --git a/drive/driveimpl/remote_impl.go b/drive/driveimpl/remote_impl.go index 446e0ed6c..c579c0f39 100644 --- a/drive/driveimpl/remote_impl.go +++ b/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() } diff --git a/drive/remote.go b/drive/remote.go index b41bd054a..9aeead710 100644 --- a/drive/remote.go +++ b/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