From 4a39a472487d37b34b379d7dabcc95fea8a72b2e Mon Sep 17 00:00:00 2001 From: Patrick O'Doherty Date: Thu, 17 Apr 2025 17:24:48 -0700 Subject: [PATCH] safeweb: replace gorilla with Sec-Fetch-Site check Require that all non-(GET|OPTIONS|HEAD) requests to the browser mux specify Sec-Fetch-Site=same-origin to prohibit cross-origin requests. Optionally allow for requests to specify "same-site" indicating a cross-origin request from an origin that shares a root domain with the application's own. Updates tailscale/corp#25340 --- safeweb/http.go | 95 ++++++++++++++--------------- safeweb/http_test.go | 141 +++++++++++++++++++------------------------ 2 files changed, 108 insertions(+), 128 deletions(-) diff --git a/safeweb/http.go b/safeweb/http.go index 143c4dcee..4d4d38874 100644 --- a/safeweb/http.go +++ b/safeweb/http.go @@ -15,11 +15,9 @@ // // # Browser Routes // -// All routes in the browser mux enforce CSRF protection using the gorilla/csrf -// package. The application must template the CSRF token into its forms using -// the [TemplateField] and [TemplateTag] APIs. Applications that are served in a -// secure context (over HTTPS) should also set the SecureContext field to true -// to ensure that the the CSRF cookies are marked as Secure. +// All routes in the browser mux are protected against CSRF attacks by +// requiring non-(GET|HEAD|OPTIONS) requests to specify the Sec-Fetch-Site header +// with the value "same-origin". // // In addition, browser routes will also have the following applied: // - Content-Security-Policy header that disallows inline scripts, framing, and third party resources. @@ -64,15 +62,11 @@ // if err := s.Serve(ln); err != nil && err != http.ErrServerClosed { // log.Fatalf("failed to serve: %v", err) // } -// -// [TemplateField]: https://pkg.go.dev/github.com/gorilla/csrf#TemplateField -// [TemplateTag]: https://pkg.go.dev/github.com/gorilla/csrf#TemplateTag package safeweb import ( "cmp" "context" - crand "crypto/rand" "fmt" "log" "maps" @@ -82,8 +76,6 @@ import ( "path" "slices" "strings" - - "github.com/gorilla/csrf" ) // CSP is the value of a Content-Security-Policy header. Keys are CSP @@ -150,10 +142,6 @@ var DefaultStrictTransportSecurityOptions = "max-age=31536000" // Config contains the configuration for a safeweb server. type Config struct { - // SecureContext specifies whether the Server is running in a secure (HTTPS) context. - // Setting this to true will cause the Server to set the Secure flag on CSRF cookies. - SecureContext bool - // BrowserMux is the HTTP handler for any routes in your application that // should only be served to browsers in a primary origin context. These // requests will be subject to CSRF protection and will have @@ -174,12 +162,6 @@ type Config struct { // No headers will be sent if no methods are provided. AccessControlAllowMethods []string - // CSRFSecret is the secret used to sign CSRF tokens. It must be 32 bytes long. - // This should be considered a sensitive value and should be kept secret. - // If this is not provided, the Server will generate a random CSRF secret on - // startup. - CSRFSecret []byte - // CSP is the Content-Security-Policy header to return with BrowserMux // responses. CSP CSP @@ -188,10 +170,6 @@ type Config struct { // inline CSS. CSPAllowInlineStyles bool - // CookiesSameSiteLax specifies whether to use SameSite=Lax in cookies. The - // default is to set SameSite=Strict. - CookiesSameSiteLax bool - // StrictTransportSecurityOptions specifies optional directives for the // Strict-Transport-Security header sent in response to requests made to the // BrowserMux when SecureContext is true. @@ -203,6 +181,16 @@ type Config struct { // Do not use the Handler field of http.Server, as it will be ignored. // Instead, set your handlers using APIMux and BrowserMux. HTTPServer *http.Server + + // ServeHSTS specifies whether to serve the Strict-Transport-Security header + // in response to requests made to the BrowserMux. + ServeHSTS bool + + // AllowSecFetchSiteSameSite specifies whether to allow requests with the + // Sec-Fetch-Site header set to "same-site " indicating that they are + // cross-origin but that their origin shares the same site (gTLD+1) with + // that of the request. + AllowSecFetchSiteSameSite bool } func (c *Config) setDefaults() error { @@ -214,13 +202,6 @@ func (c *Config) setDefaults() error { c.APIMux = &http.ServeMux{} } - if c.CSRFSecret == nil || len(c.CSRFSecret) == 0 { - c.CSRFSecret = make([]byte, 32) - if _, err := crand.Read(c.CSRFSecret); err != nil { - return fmt.Errorf("failed to generate CSRF secret: %w", err) - } - } - if c.CSP == nil { c.CSP = DefaultCSP() } @@ -231,9 +212,8 @@ func (c *Config) setDefaults() error { // Server is a safeweb server. type Server struct { Config - h *http.Server - csp string - csrfProtect func(http.Handler) http.Handler + h *http.Server + csp string } // NewServer creates a safeweb server with the provided configuration. It will @@ -252,10 +232,6 @@ func NewServer(config Config) (*Server, error) { return nil, fmt.Errorf("failed to set defaults: %w", err) } - sameSite := csrf.SameSiteStrictMode - if config.CookiesSameSiteLax { - sameSite = csrf.SameSiteLaxMode - } if config.CSPAllowInlineStyles { if _, ok := config.CSP["style-src"]; ok { config.CSP.Add("style-src", "unsafe-inline") @@ -266,9 +242,6 @@ func NewServer(config Config) (*Server, error) { s := &Server{ Config: config, csp: config.CSP.String(), - // only set Secure flag on CSRF cookies if we are in a secure context - // as otherwise the browser will reject the cookie - csrfProtect: csrf.Protect(config.CSRFSecret, csrf.Secure(config.SecureContext), csrf.SameSite(sameSite)), } s.h = cmp.Or(config.HTTPServer, &http.Server{}) if s.h.Handler != nil { @@ -318,12 +291,6 @@ func checkHandlerType(apiPattern, browserPattern string) handlerType { } func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { - // if we are not in a secure context, signal to the CSRF middleware that - // TLS-only header checks should be skipped - if !s.Config.SecureContext { - r = csrf.PlaintextHTTPRequest(r) - } - _, bp := s.BrowserMux.Handler(r) _, ap := s.APIMux.Handler(r) switch { @@ -376,10 +343,38 @@ func (s *Server) serveBrowser(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Security-Policy", s.csp) w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("Referer-Policy", "same-origin") - if s.SecureContext { + if s.ServeHSTS { w.Header().Set("Strict-Transport-Security", cmp.Or(s.StrictTransportSecurityOptions, DefaultStrictTransportSecurityOptions)) } - s.csrfProtect(s.BrowserMux).ServeHTTP(w, r) + + // Allow GET, HEAD, and OPTIONS requests to the browser mux without + // Sec-Fetch-Site header checks. + switch r.Method { + case "GET", "HEAD", "OPTIONS": + s.BrowserMux.ServeHTTP(w, r) + return + default: + } + + switch r.Header.Get("Sec-Fetch-Site") { + case "same-origin": + // allow same-origin requests + case "same-site": + // allow cross-origin, but same-site requests if configured + if !s.AllowSecFetchSiteSameSite { + http.Error(w, "forbidden cross-origin request", http.StatusForbidden) + return + } + case "cross-site", "none": + // deny cross-site requests or direct navigation non-GET requests. + http.Error(w, "forbidden cross-origin request", http.StatusForbidden) + return + default: + // deny requests with no Sec-Fetch-Site header + http.Error(w, "missing required Sec-Fetch-Site header. You might need to update your browser.", http.StatusForbidden) + return + } + s.BrowserMux.ServeHTTP(w, r) } // ServeRedirectHTTP serves a single HTTP handler on the provided listener that diff --git a/safeweb/http_test.go b/safeweb/http_test.go index 852ce326b..7644eb81d 100644 --- a/safeweb/http_test.go +++ b/safeweb/http_test.go @@ -13,7 +13,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/gorilla/csrf" ) func TestCompleteCORSConfig(t *testing.T) { @@ -156,28 +155,62 @@ func TestAPIMuxCrossOriginResourceSharingHeaders(t *testing.T) { func TestCSRFProtection(t *testing.T) { tests := []struct { - name string - apiRoute bool - passCSRFToken bool - wantStatus int + name string + httpMethod string + apiRoute bool + secFetchSiteNone bool + secFetchSiteSameOrigin bool + secFetchSiteSameSite bool + secFetchSiteCrossSite bool + permitSameSite bool + wantStatus int }{ { - name: "POST requests to non-API routes require CSRF token and fail if not provided", - apiRoute: false, - passCSRFToken: false, - wantStatus: http.StatusForbidden, + name: "GET requests to browser routes do not require Sec-Fetch-Site header", + httpMethod: http.MethodGet, + apiRoute: false, + wantStatus: http.StatusOK, }, { - name: "POST requests to non-API routes require CSRF token and pass if provided", - apiRoute: false, - passCSRFToken: true, - wantStatus: http.StatusOK, + name: "POST requests to browser routes require Sec-Fetch-Site=same-origin and fail if not provided", + httpMethod: http.MethodPost, + apiRoute: false, + wantStatus: http.StatusForbidden, }, { - name: "POST requests to /api/ routes do not require CSRF token", - apiRoute: true, - passCSRFToken: false, - wantStatus: http.StatusOK, + name: "POST requests to browser routes require Sec-Fetch-Site=same-origin and pass if provided", + secFetchSiteSameOrigin: true, + httpMethod: http.MethodPost, + apiRoute: false, + wantStatus: http.StatusOK, + }, + { + name: "POST requests to browser routes with Sec-Fetch-Site=none fail", + secFetchSiteNone: true, + httpMethod: http.MethodPost, + apiRoute: false, + wantStatus: http.StatusForbidden, + }, + { + name: "POST requests to browser routes with Sec-Fetch-Site=same-site fail by default", + secFetchSiteSameSite: true, + httpMethod: http.MethodPost, + apiRoute: false, + wantStatus: http.StatusForbidden, + }, + { + name: "POST requests to browser routes with Sec-Fetch-Site=same-site pass if configured", + secFetchSiteSameSite: true, + permitSameSite: true, + httpMethod: http.MethodPost, + apiRoute: false, + wantStatus: http.StatusOK, + }, + { + name: "POST requests to API routes do not require Sec-Fetch-Site header", + httpMethod: http.MethodPost, + apiRoute: true, + wantStatus: http.StatusOK, }, } for _, tt := range tests { @@ -199,7 +232,7 @@ func TestCSRFProtection(t *testing.T) { defer s.Close() // construct the test request - req := httptest.NewRequest("POST", "/", nil) + req := httptest.NewRequest(tt.httpMethod, "/", nil) // send JSON for API routes, form data for browser routes if tt.apiRoute { @@ -208,25 +241,19 @@ func TestCSRFProtection(t *testing.T) { req.Header.Set("Content-Type", "application/x-www-form-urlencoded") } - // retrieve CSRF cookie & pass it in the test request - // ref: https://github.com/gorilla/csrf/blob/main/csrf_test.go#L344-L347 - var token string - if tt.passCSRFToken { - h.Handle("/csrf", http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - token = csrf.Token(r) - })) - get := httptest.NewRequest("GET", "/csrf", nil) - w := httptest.NewRecorder() - s.h.Handler.ServeHTTP(w, get) - resp := w.Result() - - // pass the token & cookie in our subsequent test request - req.Header.Set("X-CSRF-Token", token) - for _, c := range resp.Cookies() { - req.AddCookie(c) - } + if tt.permitSameSite { + s.Config.AllowSecFetchSiteSameSite = true } + if tt.secFetchSiteNone { + req.Header.Set("Sec-Fetch-Site", "none") + } else if tt.secFetchSiteSameOrigin { + req.Header.Set("Sec-Fetch-Site", "same-origin") + } else if tt.secFetchSiteSameSite { + req.Header.Set("Sec-Fetch-Site", "same-site") + } else if tt.secFetchSiteCrossSite { + req.Header.Set("Sec-Fetch-Site", "cross-site") + } w := httptest.NewRecorder() s.h.Handler.ServeHTTP(w, req) resp := w.Result() @@ -294,48 +321,6 @@ func TestContentSecurityPolicyHeader(t *testing.T) { } } -func TestCSRFCookieSecureMode(t *testing.T) { - tests := []struct { - name string - secureMode bool - wantSecure bool - }{ - { - name: "CSRF cookie should be secure when server is in secure context", - secureMode: true, - wantSecure: true, - }, - { - name: "CSRF cookie should not be secure when server is not in secure context", - secureMode: false, - wantSecure: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - h := &http.ServeMux{} - h.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("ok")) - })) - s, err := NewServer(Config{BrowserMux: h, SecureContext: tt.secureMode}) - if err != nil { - t.Fatal(err) - } - defer s.Close() - - req := httptest.NewRequest("GET", "/", nil) - w := httptest.NewRecorder() - s.h.Handler.ServeHTTP(w, req) - resp := w.Result() - - cookie := resp.Cookies()[0] - if (cookie.Secure == tt.wantSecure) == false { - t.Fatalf("csrf cookie secure flag want: %v; got: %v", tt.wantSecure, cookie.Secure) - } - }) - } -} - func TestRefererPolicy(t *testing.T) { tests := []struct { name string @@ -607,7 +592,7 @@ func TestStrictTransportSecurityOptions(t *testing.T) { h.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("ok")) })) - s, err := NewServer(Config{BrowserMux: h, SecureContext: tt.secureContext, StrictTransportSecurityOptions: tt.options}) + s, err := NewServer(Config{BrowserMux: h, ServeHSTS: tt.secureContext, StrictTransportSecurityOptions: tt.options}) if err != nil { t.Fatal(err) }