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 <percy@tailscale.com>
This commit is contained in:
Percy Wegmann 2024-05-01 10:45:57 -05:00 committed by Percy Wegmann
parent cd633a7252
commit 9d22ec0ba2
6 changed files with 135 additions and 53 deletions

View File

@ -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,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)

View File

@ -10,10 +10,12 @@ import (
"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) {

View File

@ -4,6 +4,9 @@
package driveimpl
import (
"crypto/rand"
"encoding/hex"
"fmt"
"net"
"net/http"
"sync"
@ -17,6 +20,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 +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()

View File

@ -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,
})
}

View File

@ -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()
}

View File

@ -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