Security: refactor 'redirect_to' cookie to use 'Secure' flag (#19787)

* Refactor redirect_to cookie with secure flag in middleware

* Refactor redirect_to cookie with secure flag in api/login

* Refactor redirect_to cookie with secure flag in api/login_oauth

* Removed the deletion of 'Set-Cookie' header to prevent logout

* Removed the deletion of 'Set-Cookie' at top of api/login.go

* Add HttpOnly flag on redirect_to cookies where missing

* Refactor duplicated code

* Add tests

* Refactor cookie options

* Replace local function for deleting cookie

* Delete redundant calls

Co-authored-by: Sofia Papagiannaki <papagian@users.noreply.github.com>

pkg/api/login.go

@@ -2,7 +2,6 @@ package api
 
 import (
 	"encoding/hex"
-	"net/http"
 	"net/url"
 	"strings"
 
@@ -28,7 +27,7 @@ var getViewIndex = func() string {
 	return ViewIndex
 }
 
-func validateRedirectTo(redirectTo string) error {
+func (hs *HTTPServer) validateRedirectTo(redirectTo string) error {
 	to, err := url.Parse(redirectTo)
 	if err != nil {
 		return login.ErrInvalidRedirectTo
@@ -36,12 +35,20 @@ func validateRedirectTo(redirectTo string) error {
 	if to.IsAbs() {
 		return login.ErrAbsoluteRedirectTo
 	}
-	if setting.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+setting.AppSubUrl) {
+	if hs.Cfg.AppSubUrl != "" && !strings.HasPrefix(to.Path, "/"+hs.Cfg.AppSubUrl) {
 		return login.ErrInvalidRedirectTo
 	}
 	return nil
 }
 
+func (hs *HTTPServer) cookieOptionsFromCfg() middleware.CookieOptions {
+	return middleware.CookieOptions{
+		Path:     hs.Cfg.AppSubUrl + "/",
+		Secure:   hs.Cfg.CookieSecure,
+		SameSite: hs.Cfg.CookieSameSite,
+	}
+}
+
 func (hs *HTTPServer) LoginView(c *models.ReqContext) {
 	viewData, err := setIndexViewData(hs, c)
 	if err != nil {
@@ -62,7 +69,7 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
 		//therefore the loginError should be passed to the view data
 		//and the view should return immediately before attempting
 		//to login again via OAuth and enter to a redirect loop
-		deleteCookie(c, LoginErrorCookieName)
+		middleware.DeleteCookie(c.Resp, LoginErrorCookieName, hs.cookieOptionsFromCfg)
 		viewData.Settings["loginError"] = loginError
 		c.HTML(200, getViewIndex(), viewData)
 		return
@@ -79,13 +86,13 @@ func (hs *HTTPServer) LoginView(c *models.ReqContext) {
 		}
 
 		if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
-			if err := validateRedirectTo(redirectTo); err != nil {
+			if err := hs.validateRedirectTo(redirectTo); err != nil {
 				viewData.Settings["loginError"] = err.Error()
 				c.HTML(200, getViewIndex(), viewData)
-				c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
+				middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg)
 				return
 			}
-			c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
+			middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg)
 			c.Redirect(redirectTo)
 			return
 		}
@@ -168,12 +175,12 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
 	}
 
 	if redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to")); len(redirectTo) > 0 {
-		if err := validateRedirectTo(redirectTo); err == nil {
+		if err := hs.validateRedirectTo(redirectTo); err == nil {
 			result["redirectUrl"] = redirectTo
 		} else {
 			log.Info("Ignored invalid redirect_to cookie value: %v", redirectTo)
 		}
-		c.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
+		middleware.DeleteCookie(c.Resp, "redirect_to", hs.cookieOptionsFromCfg)
 	}
 
 	metrics.MApiLoginPost.Inc()
@@ -223,28 +230,13 @@ func tryGetEncryptedCookie(ctx *models.ReqContext, cookieName string) (string, b
 	return string(decryptedError), err == nil
 }
 
-func deleteCookie(ctx *models.ReqContext, cookieName string) {
-	ctx.SetCookie(cookieName, "", -1, setting.AppSubUrl+"/")
-}
-
 func (hs *HTTPServer) trySetEncryptedCookie(ctx *models.ReqContext, cookieName string, value string, maxAge int) error {
 	encryptedError, err := util.Encrypt([]byte(value), setting.SecretKey)
 	if err != nil {
 		return err
 	}
 
-	cookie := http.Cookie{
+	middleware.WriteCookie(ctx.Resp, cookieName, hex.EncodeToString(encryptedError), 60, hs.cookieOptionsFromCfg)
-		Name:     cookieName,
-		MaxAge:   60,
-		Value:    hex.EncodeToString(encryptedError),
-		HttpOnly: true,
-		Path:     setting.AppSubUrl + "/",
-		Secure:   hs.Cfg.CookieSecure,
-	}
-	if hs.Cfg.CookieSameSite != http.SameSiteDefaultMode {
-		cookie.SameSite = hs.Cfg.CookieSameSite
-	}
-	http.SetCookie(ctx.Resp, &cookie)
 
 	return nil
 }

pkg/api/login_oauth.go

@@ -20,6 +20,7 @@ import (
 	"github.com/grafana/grafana/pkg/infra/metrics"
 	"github.com/grafana/grafana/pkg/login"
 	"github.com/grafana/grafana/pkg/login/social"
+	"github.com/grafana/grafana/pkg/middleware"
 	m "github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 )
@@ -69,7 +70,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {
 		}
 
 		hashedState := hashStatecode(state, setting.OAuthService.OAuthInfos[name].ClientSecret)
-		hs.writeCookie(ctx.Resp, OauthStateCookieName, hashedState, 60, hs.Cfg.CookieSameSite)
+		middleware.WriteCookie(ctx.Resp, OauthStateCookieName, hashedState, 60, hs.cookieOptionsFromCfg)
 		if setting.OAuthService.OAuthInfos[name].HostedDomain == "" {
 			ctx.Redirect(connect.AuthCodeURL(state, oauth2.AccessTypeOnline))
 		} else {
@@ -81,8 +82,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {
 	cookieState := ctx.GetCookie(OauthStateCookieName)
 
 	// delete cookie
-	ctx.Resp.Header().Del("Set-Cookie")
+	middleware.DeleteCookie(ctx.Resp, OauthStateCookieName, hs.cookieOptionsFromCfg)
-	hs.deleteCookie(ctx.Resp, OauthStateCookieName, hs.Cfg.CookieSameSite)
 
 	if cookieState == "" {
 		ctx.Handle(500, "login.OAuthLogin(missing saved state)", nil)
@@ -217,7 +217,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {
 	metrics.MApiLoginOAuth.Inc()
 
 	if redirectTo, _ := url.QueryUnescape(ctx.GetCookie("redirect_to")); len(redirectTo) > 0 {
-		ctx.SetCookie("redirect_to", "", -1, setting.AppSubUrl+"/")
+		middleware.DeleteCookie(ctx.Resp, "redirect_to", hs.cookieOptionsFromCfg)
 		ctx.Redirect(redirectTo)
 		return
 	}
@@ -225,25 +225,6 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {
 	ctx.Redirect(setting.AppSubUrl + "/")
 }
 
-func (hs *HTTPServer) deleteCookie(w http.ResponseWriter, name string, sameSite http.SameSite) {
-	hs.writeCookie(w, name, "", -1, sameSite)
-}
-
-func (hs *HTTPServer) writeCookie(w http.ResponseWriter, name string, value string, maxAge int, sameSite http.SameSite) {
-	cookie := http.Cookie{
-		Name:     name,
-		MaxAge:   maxAge,
-		Value:    value,
-		HttpOnly: true,
-		Path:     setting.AppSubUrl + "/",
-		Secure:   hs.Cfg.CookieSecure,
-	}
-	if sameSite != http.SameSiteDefaultMode {
-		cookie.SameSite = sameSite
-	}
-	http.SetCookie(w, &cookie)
-}
-
 func hashStatecode(code, seed string) string {
 	hashBytes := sha256.Sum256([]byte(code + setting.SecretKey + seed))
 	return hex.EncodeToString(hashBytes[:])

pkg/api/login_test.go

@@ -3,6 +3,7 @@ package api
 import (
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -134,6 +135,7 @@ func TestLoginViewRedirect(t *testing.T) {
 		Cfg:     setting.NewCfg(),
 		License: models.OSSLicensingService{},
 	}
+	hs.Cfg.CookieSecure = true
 
 	sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) {
 		c.IsSignedIn = true
@@ -192,15 +194,15 @@ func TestLoginViewRedirect(t *testing.T) {
 	}
 
 	for _, c := range redirectCases {
-		setting.AppUrl = c.appURL
+		hs.Cfg.AppUrl = c.appURL
-		setting.AppSubUrl = c.appSubURL
+		hs.Cfg.AppSubUrl = c.appSubURL
 		t.Run(c.desc, func(t *testing.T) {
 			cookie := http.Cookie{
 				Name:     "redirect_to",
 				MaxAge:   60,
 				Value:    c.url,
 				HttpOnly: true,
-				Path:     setting.AppSubUrl + "/",
+				Path:     hs.Cfg.AppSubUrl + "/",
 				Secure:   hs.Cfg.CookieSecure,
 				SameSite: hs.Cfg.CookieSameSite,
 			}
@@ -211,6 +213,19 @@ func TestLoginViewRedirect(t *testing.T) {
 				location, ok := sc.resp.Header()["Location"]
 				assert.True(t, ok)
 				assert.Equal(t, location[0], c.url)
+
+				setCookie, ok := sc.resp.Header()["Set-Cookie"]
+				assert.True(t, ok, "Set-Cookie exists")
+				assert.Greater(t, len(setCookie), 0)
+				var redirectToCookieFound bool
+				expCookieValue := fmt.Sprintf("redirect_to=%v; Path=%v; Max-Age=60; HttpOnly; Secure", c.url, hs.Cfg.AppSubUrl+"/")
+				for _, cookieValue := range setCookie {
+					if cookieValue == expCookieValue {
+						redirectToCookieFound = true
+						break
+					}
+				}
+				assert.True(t, redirectToCookieFound)
 			}
 
 			responseString, err := getBody(sc.resp)
@@ -235,6 +250,7 @@ func TestLoginPostRedirect(t *testing.T) {
 		License:          models.OSSLicensingService{},
 		AuthTokenService: auth.NewFakeUserAuthTokenService(),
 	}
+	hs.Cfg.CookieSecure = true
 
 	sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response {
 		cmd := dtos.LoginCommand{
@@ -286,15 +302,15 @@ func TestLoginPostRedirect(t *testing.T) {
 	}
 
 	for _, c := range redirectCases {
-		setting.AppUrl = c.appURL
+		hs.Cfg.AppUrl = c.appURL
-		setting.AppSubUrl = c.appSubURL
+		hs.Cfg.AppSubUrl = c.appSubURL
 		t.Run(c.desc, func(t *testing.T) {
 			cookie := http.Cookie{
 				Name:     "redirect_to",
 				MaxAge:   60,
 				Value:    c.url,
 				HttpOnly: true,
-				Path:     setting.AppSubUrl + "/",
+				Path:     hs.Cfg.AppSubUrl + "/",
 				Secure:   hs.Cfg.CookieSecure,
 				SameSite: hs.Cfg.CookieSameSite,
 			}
@@ -310,6 +326,19 @@ func TestLoginPostRedirect(t *testing.T) {
 			} else {
 				assert.Equal(t, c.url, redirectURL)
 			}
+			// assert redirect_to cookie is deleted
+			setCookie, ok := sc.resp.Header()["Set-Cookie"]
+			assert.True(t, ok, "Set-Cookie exists")
+			assert.Greater(t, len(setCookie), 0)
+			var redirectToCookieFound bool
+			expCookieValue := fmt.Sprintf("redirect_to=; Path=%v; Max-Age=0; HttpOnly; Secure", hs.Cfg.AppSubUrl+"/")
+			for _, cookieValue := range setCookie {
+				if cookieValue == expCookieValue {
+					redirectToCookieFound = true
+					break
+				}
+			}
+			assert.True(t, redirectToCookieFound)
 		})
 	}
 }

pkg/middleware/auth.go

@@ -47,7 +47,7 @@ func notAuthorized(c *m.ReqContext) {
 		return
 	}
 
-	c.SetCookie("redirect_to", url.QueryEscape(setting.AppSubUrl+c.Req.RequestURI), 0, setting.AppSubUrl+"/", nil, false, true)
+	WriteCookie(c.Resp, "redirect_to", url.QueryEscape(setting.AppSubUrl+c.Req.RequestURI), 0, newCookieOptions)
 
 	c.Redirect(setting.AppSubUrl + "/login")
 }

pkg/middleware/cookie.go

@@ -0,0 +1,43 @@
+package middleware
+
+import (
+	"net/http"
+
+	"github.com/grafana/grafana/pkg/setting"
+)
+
+type CookieOptions struct {
+	Path     string
+	Secure   bool
+	SameSite http.SameSite
+}
+
+func newCookieOptions() CookieOptions {
+	return CookieOptions{
+		Path:     setting.AppSubUrl + "/",
+		Secure:   setting.CookieSecure,
+		SameSite: setting.CookieSameSite,
+	}
+}
+
+type GetCookieOptionsFunc func() CookieOptions
+
+func DeleteCookie(w http.ResponseWriter, name string, getCookieOptionsFunc GetCookieOptionsFunc) {
+	WriteCookie(w, name, "", -1, getCookieOptionsFunc)
+}
+
+func WriteCookie(w http.ResponseWriter, name string, value string, maxAge int, getCookieOptionsFunc GetCookieOptionsFunc) {
+	options := getCookieOptionsFunc()
+	cookie := http.Cookie{
+		Name:     name,
+		MaxAge:   maxAge,
+		Value:    value,
+		HttpOnly: true,
+		Path:     options.Path,
+		Secure:   options.Secure,
+	}
+	if options.SameSite != http.SameSiteDefaultMode {
+		cookie.SameSite = options.SameSite
+	}
+	http.SetCookie(w, &cookie)
+}

pkg/middleware/middleware.go

@@ -2,7 +2,6 @@ package middleware
 
 import (
 	"fmt"
-	"net/http"
 	"net/url"
 	"strconv"
 	"strings"
@@ -253,20 +252,7 @@ func WriteSessionCookie(ctx *models.ReqContext, value string, maxLifetimeDays in
 		maxAge = int(maxAgeHours.Seconds())
 	}
 
-	ctx.Resp.Header().Del("Set-Cookie")
+	WriteCookie(ctx.Resp, setting.LoginCookieName, url.QueryEscape(value), maxAge, newCookieOptions)
-	cookie := http.Cookie{
-		Name:     setting.LoginCookieName,
-		Value:    url.QueryEscape(value),
-		HttpOnly: true,
-		Path:     setting.AppSubUrl + "/",
-		Secure:   setting.CookieSecure,
-		MaxAge:   maxAge,
-	}
-	if setting.CookieSameSite != http.SameSiteDefaultMode {
-		cookie.SameSite = setting.CookieSameSite
-	}
-
-	http.SetCookie(ctx.Resp, &cookie)
 }
 
 func AddDefaultResponseHeaders() macaron.Handler {