diff --git a/client/web/web.go b/client/web/web.go index 6eccdadcf..0cbf32c5f 100644 --- a/client/web/web.go +++ b/client/web/web.go @@ -6,7 +6,6 @@ package web import ( "context" - "crypto/rand" "encoding/json" "errors" "fmt" @@ -14,14 +13,11 @@ import ( "log" "net/http" "net/netip" - "os" "path" - "path/filepath" "strings" "sync" "time" - "github.com/gorilla/csrf" "tailscale.com/client/local" "tailscale.com/client/tailscale/apitype" "tailscale.com/clientupdate" @@ -205,7 +201,7 @@ func NewServer(opts ServerOpts) (s *Server, err error) { var metric string s.apiHandler, metric = s.modeAPIHandler(s.mode) - s.apiHandler = s.withCSRF(s.apiHandler) + s.apiHandler = s.withSecFetchSite(s.apiHandler) // Don't block startup on reporting metric. // Report in separate go routine with 5 second timeout. @@ -218,23 +214,30 @@ func NewServer(opts ServerOpts) (s *Server, err error) { return s, nil } -func (s *Server) withCSRF(h http.Handler) http.Handler { - csrfProtect := csrf.Protect(s.csrfKey(), csrf.Secure(false)) +// withSecFetchSite wraps a HTTP handler and ensures requests made to it have +// the `Sec-Fetch-Site` header set to same-origin to prevent CSRF attacks. +func (s *Server) withSecFetchSite(h http.Handler) http.Handler { + // Ref https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Sec-Fetch-Site + // Sent by all modern browsers to indicate the relationship between the + // origin from which the request was made and the requested resource. + // Require that the Sec-Fetch-Site header is set to "same-origin" + // for all requests to the web client, preventing CSRF attacks. + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Header.Get("Sec-Fetch-Site") { + case "same-origin": + // valid request + case "cross-site", "same-site": + // consider anything other than the same-origin to be invalid. + http.Error(w, "forbidden non `same-origin` request", http.StatusForbidden) + return + case "": + http.Error(w, "missing required Sec-Fetch-Site request header. You may need to update your browser", http.StatusForbidden) + return + } + // serve all same-site requests. + h.ServeHTTP(w, r) + }) - // ref https://github.com/tailscale/tailscale/pull/14822 - // signal to the CSRF middleware that the request is being served over - // plaintext HTTP to skip TLS-only header checks. - withSetPlaintext := func(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - r = csrf.PlaintextHTTPRequest(r) - h.ServeHTTP(w, r) - }) - } - - // NB: the order of the withSetPlaintext and csrfProtect calls is important - // to ensure that we signal to the CSRF middleware that the request is being - // served over plaintext HTTP and not over TLS as it presumes by default. - return withSetPlaintext(csrfProtect(h)) } func (s *Server) modeAPIHandler(mode ServerMode) (http.Handler, string) { @@ -450,9 +453,8 @@ func (s *Server) authorizeRequest(w http.ResponseWriter, r *http.Request) (ok bo // serveLoginAPI serves requests for the web login client. // It should only be called by Server.ServeHTTP, via Server.apiHandler, -// which protects the handler using gorilla csrf. +// which protects the handler using s.withSecFetchSite. func (s *Server) serveLoginAPI(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-CSRF-Token", csrf.Token(r)) switch { case r.URL.Path == "/api/data" && r.Method == httpm.GET: s.serveGetNodeData(w, r) @@ -575,7 +577,6 @@ func (s *Server) serveAPI(w http.ResponseWriter, r *http.Request) { } } - w.Header().Set("X-CSRF-Token", csrf.Token(r)) path := strings.TrimPrefix(r.URL.Path, "/api") switch { case path == "/data" && r.Method == httpm.GET: @@ -834,12 +835,11 @@ type nodeData struct { KeyExpiry string // time.RFC3339 KeyExpired bool - TUNMode bool - IsSynology bool - DSMVersion int // 6 or 7, if IsSynology=true - IsUnraid bool - UnraidToken string - URLPrefix string // if set, the URL prefix the client is served behind + TUNMode bool + IsSynology bool + DSMVersion int // 6 or 7, if IsSynology=true + IsUnraid bool + URLPrefix string // if set, the URL prefix the client is served behind UsingExitNode *exitNode AdvertisingExitNode bool @@ -899,7 +899,6 @@ func (s *Server) serveGetNodeData(w http.ResponseWriter, r *http.Request) { IsSynology: distro.Get() == distro.Synology || envknob.Bool("TS_FAKE_SYNOLOGY"), DSMVersion: distro.DSMVersion(), IsUnraid: distro.Get() == distro.Unraid, - UnraidToken: os.Getenv("UNRAID_CSRF_TOKEN"), RunningSSHServer: prefs.RunSSH, URLPrefix: strings.TrimSuffix(s.pathPrefix, "/"), ControlAdminURL: prefs.AdminPageURL(), @@ -1276,37 +1275,6 @@ func (s *Server) proxyRequestToLocalAPI(w http.ResponseWriter, r *http.Request) } } -// csrfKey returns a key that can be used for CSRF protection. -// If an error occurs during key creation, the error is logged and the active process terminated. -// If the server is running in CGI mode, the key is cached to disk and reused between requests. -// If an error occurs during key storage, the error is logged and the active process terminated. -func (s *Server) csrfKey() []byte { - csrfFile := filepath.Join(os.TempDir(), "tailscale-web-csrf.key") - - // if running in CGI mode, try to read from disk, but ignore errors - if s.cgiMode { - key, _ := os.ReadFile(csrfFile) - if len(key) == 32 { - return key - } - } - - // create a new key - key := make([]byte, 32) - if _, err := rand.Read(key); err != nil { - log.Fatalf("error generating CSRF key: %v", err) - } - - // if running in CGI mode, try to write the newly created key to disk, and exit if it fails. - if s.cgiMode { - if err := os.WriteFile(csrfFile, key, 0600); err != nil { - log.Fatalf("unable to store CSRF key: %v", err) - } - } - - return key -} - // enforcePrefix returns a HandlerFunc that enforces a given path prefix is used in requests, // then strips it before invoking h. // Unlike http.StripPrefix, it does not return a 404 if the prefix is not present. diff --git a/client/web/web_test.go b/client/web/web_test.go index 2a6bc787a..ba31dbdff 100644 --- a/client/web/web_test.go +++ b/client/web/web_test.go @@ -11,7 +11,6 @@ import ( "fmt" "io" "net/http" - "net/http/cookiejar" "net/http/httptest" "net/netip" "net/url" @@ -21,7 +20,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/gorilla/csrf" "tailscale.com/client/local" "tailscale.com/client/tailscale/apitype" "tailscale.com/ipn" @@ -1491,82 +1489,77 @@ func mockWaitAuthURL(_ context.Context, id string, src tailcfg.NodeID) (*tailcfg } } -func TestCSRFProtect(t *testing.T) { - s := &Server{} - - mux := http.NewServeMux() - mux.HandleFunc("GET /test/csrf-token", func(w http.ResponseWriter, r *http.Request) { - token := csrf.Token(r) - _, err := io.WriteString(w, token) - if err != nil { - t.Fatal(err) - } - }) - mux.HandleFunc("POST /test/csrf-protected", func(w http.ResponseWriter, r *http.Request) { - _, err := io.WriteString(w, "ok") - if err != nil { - t.Fatal(err) - } - }) - h := s.withCSRF(mux) - ser := nettest.NewHTTPServer(nettest.GetNetwork(t), h) - defer ser.Close() - - jar, err := cookiejar.New(nil) - if err != nil { - t.Fatalf("unable to construct cookie jar: %v", err) +func TestSecFetchSiteProtect(t *testing.T) { + tests := []struct { + name string + withSameOrigin bool + withSameSite bool + withCrossSite bool + expectError bool + }{ + { + name: "requests with same-origin pass", + withSameOrigin: true, + }, + { + name: "requests with same-site fail", + withSameSite: true, + expectError: true, + }, + { + name: "requests with cross-site fail", + withCrossSite: true, + expectError: true, + }, + { + name: "requests with no Sec-Fetch-Site fail", + expectError: true, + }, } - client := ser.Client() - client.Jar = jar + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &Server{} + mux := http.NewServeMux() - // make GET request to populate cookie jar - resp, err := client.Get(ser.URL + "/test/csrf-token") - if err != nil { - t.Fatalf("unable to make request: %v", err) - } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - t.Fatalf("unexpected status: %v", resp.Status) - } - tokenBytes, err := io.ReadAll(resp.Body) - if err != nil { - t.Fatalf("unable to read body: %v", err) - } + mux.HandleFunc("POST /protected", func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, "ok") + if err != nil { + t.Fatal(err) + } + }) - csrfToken := strings.TrimSpace(string(tokenBytes)) - if csrfToken == "" { - t.Fatal("empty csrf token") - } + h := s.withSecFetchSite(mux) - // make a POST request without the CSRF header; ensure it fails - resp, err = client.Post(ser.URL+"/test/csrf-protected", "text/plain", nil) - if err != nil { - t.Fatalf("unable to make request: %v", err) - } - if resp.StatusCode != http.StatusForbidden { - t.Fatalf("unexpected status: %v", resp.Status) - } + serv := nettest.NewHTTPServer(nettest.GetNetwork(t), h) + defer serv.Close() - // make a POST request with the CSRF header; ensure it succeeds - req, err := http.NewRequest("POST", ser.URL+"/test/csrf-protected", nil) - if err != nil { - t.Fatalf("error building request: %v", err) - } - req.Header.Set("X-CSRF-Token", csrfToken) - resp, err = client.Do(req) - if err != nil { - t.Fatalf("unable to make request: %v", err) - } - if resp.StatusCode != http.StatusOK { - t.Fatalf("unexpected status: %v", resp.Status) - } - defer resp.Body.Close() - out, err := io.ReadAll(resp.Body) - if err != nil { - t.Fatalf("unable to read body: %v", err) - } - if string(out) != "ok" { - t.Fatalf("unexpected body: %q", out) + client := serv.Client() + + req, err := http.NewRequest("POST", serv.URL+"/protected", nil) + if err != nil { + t.Fatalf("error building request: %v", err) + } + + if tt.withSameOrigin { + req.Header.Set("Sec-Fetch-Site", "same-origin") + } else if tt.withSameSite { + req.Header.Set("Sec-Fetch-Site", "same-site") + } else if tt.withCrossSite { + req.Header.Set("Sec-Fetch-Site", "cross-site") + } + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("unable to make request: %v", err) + } + defer resp.Body.Close() + + if tt.expectError && resp.StatusCode == http.StatusOK { + t.Fatalf("expected error but got ok") + } else if !tt.expectError && resp.StatusCode != http.StatusOK { + t.Fatalf("expected ok got %q", resp.Status) + } + }) } } diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index 4cc4a8d46..481d3321e 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/cmd/k8s-operator/depaware.txt @@ -144,8 +144,6 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+ L github.com/google/nftables/xt from github.com/google/nftables/expr+ github.com/google/uuid from github.com/prometheus-community/pro-bing+ - github.com/gorilla/csrf from tailscale.com/client/web - github.com/gorilla/securecookie from github.com/gorilla/csrf github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+ L 💣 github.com/illarion/gonotify/v3 from tailscale.com/net/dns L github.com/illarion/gonotify/v3/syscallf from github.com/illarion/gonotify/v3 @@ -1131,13 +1129,12 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ W debug/dwarf from debug/pe W debug/pe from github.com/dblohm7/wingoes/pe embed from github.com/tailscale/web-client-prebuilt+ - encoding from encoding/gob+ + encoding from encoding/json+ encoding/asn1 from crypto/x509+ encoding/base32 from github.com/fxamacker/cbor/v2+ encoding/base64 from encoding/json+ encoding/binary from compress/gzip+ encoding/csv from github.com/spf13/pflag - encoding/gob from github.com/gorilla/securecookie encoding/hex from crypto/x509+ encoding/json from expvar+ encoding/pem from crypto/tls+ @@ -1159,7 +1156,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ hash/fnv from google.golang.org/protobuf/internal/detrand hash/maphash from go4.org/mem html from html/template+ - html/template from github.com/gorilla/csrf+ + html/template from tailscale.com/util/eventbus internal/abi from crypto/x509/internal/macos+ internal/asan from internal/runtime/maps+ internal/bisect from internal/godebug @@ -1191,7 +1188,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ internal/runtime/math from internal/runtime/maps+ internal/runtime/sys from crypto/subtle+ L internal/runtime/syscall from runtime+ - internal/saferio from debug/pe+ + W internal/saferio from debug/pe internal/singleflight from net internal/stringslite from embed+ internal/sync from sync+ diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 7f66e7700..473b23fe0 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -27,8 +27,6 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+ L github.com/google/nftables/xt from github.com/google/nftables/expr+ DW github.com/google/uuid from tailscale.com/clientupdate+ - github.com/gorilla/csrf from tailscale.com/client/web - github.com/gorilla/securecookie from github.com/gorilla/csrf github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+ L 💣 github.com/jsimonetti/rtnetlink from tailscale.com/net/netmon L github.com/jsimonetti/rtnetlink/internal/unix from github.com/jsimonetti/rtnetlink @@ -318,12 +316,11 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep W debug/dwarf from debug/pe W debug/pe from github.com/dblohm7/wingoes/pe embed from github.com/peterbourgon/ff/v3+ - encoding from encoding/gob+ + encoding from encoding/json+ encoding/asn1 from crypto/x509+ encoding/base32 from github.com/fxamacker/cbor/v2+ encoding/base64 from encoding/json+ encoding/binary from compress/gzip+ - encoding/gob from github.com/gorilla/securecookie encoding/hex from crypto/x509+ encoding/json from expvar+ encoding/pem from crypto/tls+ @@ -337,7 +334,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep hash/crc32 from compress/gzip+ hash/maphash from go4.org/mem html from html/template+ - html/template from github.com/gorilla/csrf+ + html/template from tailscale.com/util/eventbus image from github.com/skip2/go-qrcode+ image/color from github.com/skip2/go-qrcode+ image/png from github.com/skip2/go-qrcode @@ -371,7 +368,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep internal/runtime/math from internal/runtime/maps+ internal/runtime/sys from crypto/subtle+ L internal/runtime/syscall from runtime+ - internal/saferio from debug/pe+ + W internal/saferio from debug/pe internal/singleflight from net internal/stringslite from embed+ internal/sync from sync+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 329c00e93..af816d5f7 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -116,8 +116,6 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+ L github.com/google/nftables/xt from github.com/google/nftables/expr+ DW github.com/google/uuid from tailscale.com/clientupdate+ - github.com/gorilla/csrf from tailscale.com/client/web - github.com/gorilla/securecookie from github.com/gorilla/csrf github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+ L 💣 github.com/illarion/gonotify/v3 from tailscale.com/net/dns L github.com/illarion/gonotify/v3/syscallf from github.com/illarion/gonotify/v3 @@ -580,12 +578,11 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de W debug/dwarf from debug/pe W debug/pe from github.com/dblohm7/wingoes/pe embed from github.com/tailscale/web-client-prebuilt+ - encoding from encoding/gob+ + encoding from encoding/json+ encoding/asn1 from crypto/x509+ encoding/base32 from github.com/fxamacker/cbor/v2+ encoding/base64 from encoding/json+ encoding/binary from compress/gzip+ - encoding/gob from github.com/gorilla/securecookie encoding/hex from crypto/x509+ encoding/json from expvar+ encoding/pem from crypto/tls+ @@ -599,7 +596,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de hash/crc32 from compress/gzip+ hash/maphash from go4.org/mem html from html/template+ - html/template from github.com/gorilla/csrf+ + html/template from tailscale.com/util/eventbus internal/abi from crypto/x509/internal/macos+ internal/asan from internal/runtime/maps+ internal/bisect from internal/godebug @@ -630,7 +627,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de internal/runtime/math from internal/runtime/maps+ internal/runtime/sys from crypto/subtle+ L internal/runtime/syscall from runtime+ - internal/saferio from debug/pe+ + W internal/saferio from debug/pe internal/singleflight from net internal/stringslite from embed+ internal/sync from sync+