From c40ad64604f6be9a30ee864a6a34bd1c3ed15fc2 Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Thu, 15 Sep 2022 15:30:19 -0500 Subject: [PATCH] feat(security): set SameSite=strict on session cookie (#23723) * feat(security): set SameSite=strict on session cookie Use SameSite=Strict as a hardening measure against cross-origin attacks. While browsers have been moving to default to SameSite=Lax, explicitly setting SameSite ensures that all browsers enforce it consistently. While 'lax' is a reasonable hardening choice, the cookie is only required for requests to '/api/...' and we don't expect 3rd party links into '/api/...', so this stricter setting should be safe in terms of usability. Furthermore, while our GET APIs are not state-changing, using 'strict' future-proofs us in case we add a state-changing GET API ('lax' allows cross-origin 'GET' requests for increased usability for read-only requests). Also add a comment to SetCORS() lack of Access-Control-Allow-Credentials as a reminder that its omission is intentional for defense in depth on when to attach the cookie to a request. * chore: mention that Lax sends the cookie with other safe HTTP methods --- kit/transport/http/middleware.go | 12 ++++++++ session/http_server.go | 47 +++++++++++++++++++++++++++++--- session/http_server_test.go | 2 +- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/kit/transport/http/middleware.go b/kit/transport/http/middleware.go index 2dd0889b78..f3d45daddb 100644 --- a/kit/transport/http/middleware.go +++ b/kit/transport/http/middleware.go @@ -20,6 +20,18 @@ import ( // Middleware constructor. type Middleware func(http.Handler) http.Handler +// SetCORS configures various headers to relax CORS browser checks +// +// Browsers implement Cross-Origin Resource Sharing (CORS) checks to limit +// cross-origin accesses in HTTP requests. Various CORS headers can be used to +// relax the standard, strict browser behavior. For details on CORS, see: +// https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS +// +// IMPORTANT: default CORS checks disallows attaching credentials (eg, session +// cookie) with cross-origin requests (even when Access-Control-Allow-Origin is +// set to the Origin) and we omit the 'Access-Control-Allow-Credentials' header +// here to preserve this behavior. Omitting this header provides security +// defense-in-depth. func SetCORS(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { if origin := r.Header.Get("Origin"); origin != "" { diff --git a/session/http_server.go b/session/http_server.go index 2b758aa1fa..98a07ffe8e 100644 --- a/session/http_server.go +++ b/session/http_server.go @@ -164,11 +164,50 @@ func decodeSignoutRequest(ctx context.Context, r *http.Request) (*signoutRequest const cookieSessionName = "influxdb-oss-session" func encodeCookieSession(w http.ResponseWriter, s *influxdb.Session) { + // We only need the session cookie for accesses to "/api/...", so limit + // it to that using "Path". + // + // Since the cookie is limited to "/api/..." and we don't expect any + // links directly into /api/..., use SameSite=Strict as a hardening + // measure. This works because external links into the UI have the form + // https:///orgs//..., https:///signin, etc and don't + // need the cookie sent. By the time the UI itself calls out to + // /api/..., the location bar matches the cookie's domain and + // everything is 1st party and Strict's restriction work fine. + // + // SameSite=Lax would also be safe to use (modern browser's default if + // unset) since it only sends the cookie with GET (and other safe HTTP + // methods like HEAD and OPTIONS as defined in RFC6264) requests when + // the location bar matches the domain of the cookie and we know that + // our APIs do not perform state-changing actions with GET and other + // safe methods. Using SameSite=Strict helps future-proof us against + // that changing (ie, we add a state-changing GET API). + // + // Note: it's generally recommended that SameSite should not be relied + // upon (particularly Lax) because: + // a) SameSite doesn't work with (cookie-less) Basic Auth. We don't + // share browser session BasicAuth with accesses to to /api/... so + // this isn't a problem + // b) SameSite=lax allows GET (and other safe HTTP methods) and some + // services might allow state-changing requests via GET. Our API + // doesn't support state-changing GETs and SameSite=strict doesn't + // allow GETs from 3rd party sites at all, so this isn't a problem + // c) similar to 'b', some frameworks will accept HTTP methods for + // other handlers. Eg, the application is designed for POST but it + // will accept requests converted to the GET method. Golang does not + // do this itself and our route mounts explicitly map the HTTP + // method to the specific handler and thus we are not susceptible to + // this + // d) SameSite could be bypassed if the attacker were able to + // manipulate the location bar in the browser (a serious browser + // bug; it is reasonable for us to expect browsers to enforce their + // SameSite restrictions) c := &http.Cookie{ - Name: cookieSessionName, - Value: s.Key, - Path: "/api/", - Expires: s.ExpiresAt, + Name: cookieSessionName, + Value: s.Key, + Path: "/api/", // since UI doesn't need it, limit cookie usage to API requests + Expires: s.ExpiresAt, + SameSite: http.SameSiteStrictMode, } http.SetCookie(w, c) diff --git a/session/http_server_test.go b/session/http_server_test.go index 75705eb379..b22e4c357e 100644 --- a/session/http_server_test.go +++ b/session/http_server_test.go @@ -58,7 +58,7 @@ func TestSessionHandler_handleSignin(t *testing.T) { password: "supersecret", }, wants: wants{ - cookie: "influxdb-oss-session=abc123xyz; Path=/api/; Expires=Thu, 26 Sep 2030 00:00:00 GMT", + cookie: "influxdb-oss-session=abc123xyz; Path=/api/; Expires=Thu, 26 Sep 2030 00:00:00 GMT; SameSite=Strict", code: http.StatusNoContent, }, },