From 2ad356979d2ceb74798abab3840476b6513f54fd Mon Sep 17 00:00:00 2001 From: Anders Pitman <&ers@apitman.com> Date: Sat, 7 Sep 2024 17:45:49 -0600 Subject: [PATCH] Make login cookie SameSite=None This is currently necessary for FedCM. In order to try and protect against unanticipated security issues from this, I added a new lax cookie that is set on all requests. I then gated all non-FedCM access to the login cookie behind the getLoginCookie function. This function verifies that the lax cookie is present, which indicates the request is not cross-site. We can't use the Origin header for this because FedCM doesn't send it for privacy reasons. Hopefully FedCM eventually changes to allow SameSite=lax so we don't have to worry about this. See https://github.com/w3c-fedid/FedCM/issues/587 --- add_identity_email.go | 12 ++++--- add_identity_fedcm.go | 7 +--- add_identity_gaml.go | 4 +-- add_identity_oauth2.go | 12 ++++--- fedcm.go | 4 +-- obligator.go | 28 ++++++++++----- qr.go | 7 ++-- utils.go | 78 ++++++++++++++++++++++++++++++------------ 8 files changed, 96 insertions(+), 56 deletions(-) diff --git a/add_identity_email.go b/add_identity_email.go index d7d9b41..80c48ff 100644 --- a/add_identity_email.go +++ b/add_identity_email.go @@ -65,8 +65,6 @@ func NewAddIdentityEmailHandler(db Database, cluster *Cluster, tmpl *template.Te prefix, err := db.GetPrefix() checkErr(err) - loginKeyName := prefix + "login_key" - // Periodically clean up expired shares go func() { for { @@ -410,7 +408,7 @@ func NewAddIdentityEmailHandler(db Database, cluster *Cluster, tmpl *template.Te email := pendingLogin.Email cookieValue := "" - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err == nil { cookieValue = loginKeyCookie.Value } @@ -431,8 +429,12 @@ func NewAddIdentityEmailHandler(db Database, cluster *Cluster, tmpl *template.Te return } - w.Header().Add("Set-Login", "logged-in") - http.SetCookie(w, cookie) + err = setLoginCookie(w, cookie) + if err != nil { + w.WriteHeader(500) + fmt.Fprintf(os.Stderr, err.Error()) + return + } returnUri, err := getReturnUriCookie(db, r) if err != nil { diff --git a/add_identity_fedcm.go b/add_identity_fedcm.go index bcc6871..c9126e5 100644 --- a/add_identity_fedcm.go +++ b/add_identity_fedcm.go @@ -23,11 +23,6 @@ func NewAddIdentityFedCmHandler(db Database, tmpl *template.Template, jose *JOSE mux: mux, } - prefix, err := db.GetPrefix() - checkErr(err) - - loginKeyName := prefix + "login_key" - mux.HandleFunc("/login-fedcm", func(w http.ResponseWriter, r *http.Request) { r.ParseForm() @@ -127,7 +122,7 @@ func NewAddIdentityFedCmHandler(db Database, tmpl *template.Template, jose *JOSE } cookieValue := "" - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err == nil { cookieValue = loginKeyCookie.Value } diff --git a/add_identity_gaml.go b/add_identity_gaml.go index 04b2362..218403f 100644 --- a/add_identity_gaml.go +++ b/add_identity_gaml.go @@ -31,8 +31,6 @@ func NewAddIdentityGamlHandler(db Database, cluster *Cluster, tmpl *template.Tem prefix, err := db.GetPrefix() checkErr(err) - loginKeyName := prefix + "login_key" - mux.HandleFunc("/login-gaml", func(w http.ResponseWriter, r *http.Request) { templateData := struct { @@ -193,7 +191,7 @@ func NewAddIdentityGamlHandler(db Database, cluster *Cluster, tmpl *template.Tem } cookieValue := "" - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err == nil { cookieValue = loginKeyCookie.Value } diff --git a/add_identity_oauth2.go b/add_identity_oauth2.go index a5b1e89..b6027d8 100644 --- a/add_identity_oauth2.go +++ b/add_identity_oauth2.go @@ -66,8 +66,6 @@ func NewAddIdentityOauth2Handler(db Database, oauth2MetaMan *OAuth2MetadataManag prefix, err := db.GetPrefix() checkErr(err) - loginKeyName := prefix + "login_key" - mux.HandleFunc("/login-oauth2", func(w http.ResponseWriter, r *http.Request) { r.ParseForm() @@ -333,7 +331,7 @@ func NewAddIdentityOauth2Handler(db Database, oauth2MetaMan *OAuth2MetadataManag deleteReturnUriCookie(r.Host, db, w) cookieValue := "" - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err == nil { cookieValue = loginKeyCookie.Value } @@ -354,8 +352,12 @@ func NewAddIdentityOauth2Handler(db Database, oauth2MetaMan *OAuth2MetadataManag return } - w.Header().Add("Set-Login", "logged-in") - http.SetCookie(w, cookie) + err = setLoginCookie(w, cookie) + if err != nil { + w.WriteHeader(500) + fmt.Fprintf(os.Stderr, err.Error()) + return + } redirUrl := fmt.Sprintf("%s", returnUri) diff --git a/fedcm.go b/fedcm.go index a92285f..423e2c0 100644 --- a/fedcm.go +++ b/fedcm.go @@ -117,7 +117,7 @@ func NewFedCmHandler(db Database, loginEndpoint string, jose *JOSE) *FedCmHandle return } - idents, _ := getIdentities(db, r) + idents, _ := getIdentitiesFedCm(db, r) if len(idents) == 0 { w.WriteHeader(401) @@ -213,7 +213,7 @@ func NewFedCmHandler(db Database, loginEndpoint string, jose *JOSE) *FedCmHandle return } - idents, _ := getIdentities(db, r) + idents, _ := getIdentitiesFedCm(db, r) accountId := r.Form.Get("account_id") pkceCodeChallenge := r.Form.Get("nonce") diff --git a/obligator.go b/obligator.go index 250c73c..c7adb99 100644 --- a/obligator.go +++ b/obligator.go @@ -164,6 +164,25 @@ func (s *ObligatorMux) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + cookieDomain, err := buildCookieDomain(r.Host) + if err != nil { + w.WriteHeader(500) + io.WriteString(w, err.Error()) + return + } + + crossSiteDetectorCookie := &http.Cookie{ + Domain: cookieDomain, + Name: "obligator_not_cross_site", + Value: "true", + Secure: true, + HttpOnly: true, + MaxAge: 86400 * 365, + Path: "/", + SameSite: http.SameSiteLaxMode, + } + http.SetCookie(w, crossSiteDetectorCookie) + fmt.Println(fmt.Sprintf("%s\t%s\t%s\t%s\t%s", timestamp, remoteIp, r.Method, r.Host, r.URL.Path)) s.mux.ServeHTTP(w, r) } @@ -468,14 +487,7 @@ func validate(db Database, r *http.Request, jose *JOSE) (*Validation, error) { return nil, err } - prefix, err := db.GetPrefix() - if err != nil { - return nil, err - } - - loginKeyName := prefix + "login_key" - - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err != nil { return checkErrPassthrough(err, passthrough) } diff --git a/qr.go b/qr.go index e32412f..dee4671 100644 --- a/qr.go +++ b/qr.go @@ -50,10 +50,7 @@ func NewQrHandler(db Database, cluster *Cluster, tmpl *template.Template, jose * const ShareTimeout = 2 * time.Minute - prefix, err := db.GetPrefix() - checkErr(err) - - loginKeyName := prefix + "login_key" + var err error // Periodically clean up expired shares go func() { @@ -272,7 +269,7 @@ func NewQrHandler(db Database, cluster *Cluster, tmpl *template.Template, jose * } cookie := &http.Cookie{} - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err == nil { cookie = loginKeyCookie } diff --git a/utils.go b/utils.go index 7302b60..b0a2594 100644 --- a/utils.go +++ b/utils.go @@ -138,6 +138,38 @@ func validUser(id string, users []*User) bool { return false } +func getLoginCookie(db Database, r *http.Request) (*http.Cookie, error) { + + prefix, err := db.GetPrefix() + if err != nil { + return nil, err + } + + loginKeyName := prefix + "login_key" + + loginKeyCookie, err := r.Cookie(loginKeyName) + if err != nil { + return nil, err + } + + // This cookie unlocks the main one. This is necessary for FedCM + crossSiteDetectorCookieName := "obligator_not_cross_site" + _, err = r.Cookie(crossSiteDetectorCookieName) + if err != nil { + return nil, err + } + + return loginKeyCookie, nil +} + +func setLoginCookie(w http.ResponseWriter, cookie *http.Cookie) error { + + w.Header().Add("Set-Login", "logged-in") + http.SetCookie(w, cookie) + + return nil +} + func addIdentToCookie(domain string, db Database, cookieValue string, newIdent *Identity, jose *JOSE) (*http.Cookie, error) { idents := []*Identity{newIdent} @@ -203,12 +235,6 @@ func addIdentToCookie(domain string, db Database, cookieValue string, newIdent * loginKeyName := prefix + "login_key" - sameSiteMode := http.SameSiteLaxMode - - if os.Getenv("FEDCM_ENABLED") == "true" { - sameSiteMode = http.SameSiteNoneMode - } - cookie := &http.Cookie{ Domain: cookieDomain, Name: loginKeyName, @@ -217,9 +243,7 @@ func addIdentToCookie(domain string, db Database, cookieValue string, newIdent * HttpOnly: true, MaxAge: 86400 * 365, Path: "/", - SameSite: sameSiteMode, - //SameSite: http.SameSiteLaxMode, - //SameSite: http.SameSiteStrictMode, + SameSite: http.SameSiteNoneMode, } return cookie, nil @@ -236,7 +260,7 @@ func addLoginToCookie(db Database, r *http.Request, clientId string, newLogin *L loginKeyName := prefix + "login_key" - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err != nil { return nil, errors.New("Only logged-in users can access this endpoint") } @@ -462,9 +486,9 @@ func getRemoteIp(r *http.Request, behindProxy bool) (string, error) { return remoteIp, nil } -func getIdentities(db Database, r *http.Request) ([]*Identity, error) { - - identities := []*Identity{} +// This function doesn't check against cross site requests as compared to +// the normal version +func getIdentitiesFedCm(db Database, r *http.Request) ([]*Identity, error) { prefix, err := db.GetPrefix() if err != nil { @@ -475,9 +499,26 @@ func getIdentities(db Database, r *http.Request) ([]*Identity, error) { loginKeyCookie, err := r.Cookie(loginKeyName) if err != nil { - return identities, err + return nil, err + } + + return getIdentitiesCommon(db, r, loginKeyCookie) +} + +func getIdentities(db Database, r *http.Request) ([]*Identity, error) { + + loginKeyCookie, err := getLoginCookie(db, r) + if err != nil { + return []*Identity{}, err } + return getIdentitiesCommon(db, r, loginKeyCookie) +} + +func getIdentitiesCommon(db Database, r *http.Request, loginKeyCookie *http.Cookie) ([]*Identity, error) { + + identities := []*Identity{} + jwtStr := loginKeyCookie.Value if jwtStr == "" { @@ -503,14 +544,7 @@ func getIdentities(db Database, r *http.Request) ([]*Identity, error) { func getLogins(db Database, r *http.Request) (map[string][]*Login, error) { - prefix, err := db.GetPrefix() - if err != nil { - return nil, err - } - - loginKeyName := prefix + "login_key" - - loginKeyCookie, err := r.Cookie(loginKeyName) + loginKeyCookie, err := getLoginCookie(db, r) if err != nil { return nil, err }