Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. (#19337) Do a refactoring to the CSRF related code, remove most unnecessary functions. Parse the generated token's issue time, regenerate the token every a few minutes.

Changes

10 changed files (+166/-192)

• integrations/csrf_test.go (new)
  • @@ -0,0 +1,52 @@
    + // Copyright 2017 The Gitea Authors. All rights reserved. + // Use of this source code is governed by a MIT-style + // license that can be found in the LICENSE file. + + package integrations + + import ( + "net/http" + "strings" + "testing" + + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + + "github.com/stretchr/testify/assert" + ) + + func TestCsrfProtection(t *testing.T) { + defer prepareTestEnv(t)() + + // test web form csrf via form + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) + session := loginUser(t, user.Name) + req := NewRequestWithValues(t, "POST", "/user/settings", map[string]string{ + "_csrf": "fake_csrf", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + resp := session.MakeRequest(t, req, http.StatusSeeOther) + loc := resp.Header().Get("Location") + assert.Equal(t, setting.AppSubURL+"/", loc) + resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Equal(t, "Bad Request: invalid CSRF token", + strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()), + ) + + // test web form csrf via header. TODO: should use an UI api to test + req = NewRequest(t, "POST", "/user/settings") + req.Header.Add("X-Csrf-Token", "fake_csrf") + session.MakeRequest(t, req, http.StatusSeeOther) + + resp = session.MakeRequest(t, req, http.StatusSeeOther) + loc = resp.Header().Get("Location") + assert.Equal(t, setting.AppSubURL+"/", loc) + resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + assert.Equal(t, "Bad Request: invalid CSRF token", + strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()), + ) + }
• integrations/repo_topic_test.go
  • @@ -10,6 +10,7 @@ import (
    "testing" api "code.gitea.io/gitea/modules/structs" + "github.com/stretchr/testify/assert" )
• modules/context/auth.go
  • @@ -63,7 +63,7 @@ func Toggle(options *ToggleOptions) func(ctx *Context) {
    } if !options.SignOutRequired && !options.DisableCSRF && ctx.Req.Method == "POST" { - Validate(ctx, ctx.csrf) + ctx.csrf.Validate(ctx) if ctx.Written() { return }
• modules/context/context.go
  • @@ -59,7 +59,7 @@ type Context struct {
    Render Render translation.Locale Cache cache.Cache - csrf CSRF + csrf CSRFProtector Flash *middleware.Flash Session session.Store
  • @@ -666,7 +666,9 @@ func Auth(authMethod auth.Method) func(*Context) {
    func Contexter() func(next http.Handler) http.Handler { rnd := templates.HTMLRenderer() csrfOpts := getCsrfOpts() - + if !setting.IsProd { + CsrfTokenRegenerationInterval = 5 * time.Second // in dev, re-generate the tokens more aggressively for debug purpose + } return func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { locale := middleware.Locale(resp, req)
  • @@ -697,7 +699,7 @@ func Contexter() func(next http.Handler) http.Handler {
    ctx.Data["Context"] = &ctx ctx.Req = WithContext(req, &ctx) - ctx.csrf = Csrfer(csrfOpts, &ctx) + ctx.csrf = PrepareCSRFProtector(csrfOpts, &ctx) // Get flash. flashCookie := ctx.GetCookie("macaron_flash")
  • @@ -755,7 +757,7 @@ func Contexter() func(next http.Handler) http.Handler {
    ctx.Resp.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) - ctx.Data["CsrfToken"] = html.EscapeString(ctx.csrf.GetToken()) + ctx.Data["CsrfToken"] = ctx.csrf.GetToken() ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + ctx.Data["CsrfToken"].(string) + `">`) // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these
• modules/context/csrf.go
  • @@ -31,29 +31,19 @@ import (
    "code.gitea.io/gitea/modules/web/middleware" ) - // CSRF represents a CSRF service and is used to get the current token and validate a suspect token. - type CSRF interface { - // Return HTTP header to search for token. + // CSRFProtector represents a CSRF protector and is used to get the current token and validate the token. + type CSRFProtector interface { + // GetHeaderName returns HTTP header to search for token. GetHeaderName() string - // Return form value to search for token. + // GetFormName returns form value to search for token. GetFormName() string - // Return cookie name to search for token. - GetCookieName() string - // Return cookie path - GetCookiePath() string - // Return the flag value used for the csrf token. - GetCookieHTTPOnly() bool - // Return cookie domain - GetCookieDomain() string - // Return the token. + // GetToken returns the token. GetToken() string - // Validate by token. - ValidToken(t string) bool - // Error replies to the request with a custom function when ValidToken fails. - Error(w http.ResponseWriter) + // Validate validates the token in http context. + Validate(ctx *Context) } - type csrf struct { + type csrfProtector struct { // Header name value for setting and getting csrf token. Header string // Form name value for setting and getting csrf token.
  • @@ -72,56 +62,24 @@ type csrf struct {
    ID string // Secret used along with the unique id above to generate the Token. Secret string - // ErrorFunc is the custom function that replies to the request when ValidToken fails. - ErrorFunc func(w http.ResponseWriter) } // GetHeaderName returns the name of the HTTP header for csrf token. - func (c *csrf) GetHeaderName() string { + func (c *csrfProtector) GetHeaderName() string { return c.Header } // GetFormName returns the name of the form value for csrf token. - func (c *csrf) GetFormName() string { + func (c *csrfProtector) GetFormName() string { return c.Form } - // GetCookieName returns the name of the cookie for csrf token. - func (c *csrf) GetCookieName() string { - return c.Cookie - } - - // GetCookiePath returns the path of the cookie for csrf token. - func (c *csrf) GetCookiePath() string { - return c.CookiePath - } - - // GetCookieHTTPOnly returns the flag value used for the csrf token. - func (c *csrf) GetCookieHTTPOnly() bool { - return c.CookieHTTPOnly - } - - // GetCookieDomain returns the flag value used for the csrf token. - func (c *csrf) GetCookieDomain() string { - return c.CookieDomain - } - // GetToken returns the current token. This is typically used // to populate a hidden form in an HTML template. - func (c *csrf) GetToken() string { + func (c *csrfProtector) GetToken() string { return c.Token } - // ValidToken validates the passed token against the existing Secret and ID. - func (c *csrf) ValidToken(t string) bool { - return ValidToken(t, c.Secret, c.ID, "POST") - } - - // Error replies to the request when ValidToken fails. - func (c *csrf) Error(w http.ResponseWriter) { - c.ErrorFunc(w) - } - // CsrfOptions maintains options to manage behavior of Generate. type CsrfOptions struct { // The global secret value used to generate Tokens.
  • @@ -143,7 +101,7 @@ type CsrfOptions struct {
    SessionKey string // oldSessionKey saves old value corresponding to SessionKey. oldSessionKey string - // If true, send token via X-CSRFToken header. + // If true, send token via X-Csrf-Token header. SetHeader bool // If true, send token via _csrf cookie. SetCookie bool
  • @@ -151,20 +109,12 @@ type CsrfOptions struct {
    Secure bool // Disallow Origin appear in request header. Origin bool - // The function called when Validate fails. - ErrorFunc func(w http.ResponseWriter) - // Cookie life time. Default is 0 + // Cookie lifetime. Default is 0 CookieLifeTime int } - func prepareOptions(options []CsrfOptions) CsrfOptions { - var opt CsrfOptions - if len(options) > 0 { - opt = options[0] - } - - // Defaults. - if len(opt.Secret) == 0 { + func prepareDefaultCsrfOptions(opt CsrfOptions) CsrfOptions { + if opt.Secret == "" { randBytes, err := util.CryptoRandomBytes(8) if err != nil { // this panic can be handled by the recover() in http handlers
  • @@ -172,36 +122,30 @@ func prepareOptions(options []CsrfOptions) CsrfOptions {
    } opt.Secret = base32.StdEncoding.EncodeToString(randBytes) } - if len(opt.Header) == 0 { - opt.Header = "X-CSRFToken" + if opt.Header == "" { + opt.Header = "X-Csrf-Token" } - if len(opt.Form) == 0 { + if opt.Form == "" { opt.Form = "_csrf" } - if len(opt.Cookie) == 0 { + if opt.Cookie == "" { opt.Cookie = "_csrf" } - if len(opt.CookiePath) == 0 { + if opt.CookiePath == "" { opt.CookiePath = "/" } - if len(opt.SessionKey) == 0 { + if opt.SessionKey == "" { opt.SessionKey = "uid" } opt.oldSessionKey = "_old_" + opt.SessionKey - if opt.ErrorFunc == nil { - opt.ErrorFunc = func(w http.ResponseWriter) { - http.Error(w, "Invalid csrf token.", http.StatusBadRequest) - } - } - return opt } - // Csrfer maps CSRF to each request. If this request is a Get request, it will generate a new token. + // PrepareCSRFProtector returns a CSRFProtector to be used for every request. // Additionally, depending on options set, generated tokens will be sent via Header and/or Cookie. - func Csrfer(opt CsrfOptions, ctx *Context) CSRF { - opt = prepareOptions([]CsrfOptions{opt}) - x := &csrf{ + func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { + opt = prepareDefaultCsrfOptions(opt) + x := &csrfProtector{ Secret: opt.Secret, Header: opt.Header, Form: opt.Form,
  • @@ -209,7 +153,6 @@ func Csrfer(opt CsrfOptions, ctx *Context) CSRF {
    CookieDomain: opt.CookieDomain, CookiePath: opt.CookiePath, CookieHTTPOnly: opt.CookieHTTPOnly, - ErrorFunc: opt.ErrorFunc, } if opt.Origin && len(ctx.Req.Header.Get("Origin")) > 0 {
  • @@ -229,28 +172,31 @@ func Csrfer(opt CsrfOptions, ctx *Context) CSRF {
    } } - needsNew := false oldUID := ctx.Session.Get(opt.oldSessionKey) - if oldUID == nil || oldUID.(string) != x.ID { - needsNew = true + uidChanged := oldUID == nil || oldUID.(string) != x.ID + cookieToken := ctx.GetCookie(opt.Cookie) + + needsNew := true + if uidChanged { _ = ctx.Session.Set(opt.oldSessionKey, x.ID) - } else { - // If cookie present, map existing token, else generate a new one. - if val := ctx.GetCookie(opt.Cookie); len(val) > 0 { - // FIXME: test coverage. - x.Token = val - } else { - needsNew = true + } else if cookieToken != "" { + // If cookie token presents, re-use existing unexpired token, else generate a new one. + if issueTime, ok := ParseCsrfToken(cookieToken); ok { + dur := time.Since(issueTime) // issueTime is not a monotonic-clock, the server time may change a lot to an early time. + if dur >= -CsrfTokenRegenerationInterval && dur <= CsrfTokenRegenerationInterval { + x.Token = cookieToken + needsNew = false + } } } if needsNew { // FIXME: actionId. - x.Token = GenerateToken(x.Secret, x.ID, "POST") + x.Token = GenerateCsrfToken(x.Secret, x.ID, "POST", time.Now()) if opt.SetCookie { var expires interface{} if opt.CookieLifeTime == 0 { - expires = time.Now().AddDate(0, 0, 1) + expires = time.Now().Add(CsrfTokenTimeout) } middleware.SetCookie(ctx.Resp, opt.Cookie, x.Token, opt.CookieLifeTime,
  • @@ -270,47 +216,31 @@ func Csrfer(opt CsrfOptions, ctx *Context) CSRF {
    return x } - // Validate should be used as a per route middleware. It attempts to get a token from a "X-CSRFToken" - // HTTP header and then a "_csrf" form value. If one of these is found, the token will be validated - // using ValidToken. If this validation fails, custom Error is sent in the reply. - // If neither a header or form value is found, http.StatusBadRequest is sent. - func Validate(ctx *Context, x CSRF) { - if token := ctx.Req.Header.Get(x.GetHeaderName()); len(token) > 0 { - if !x.ValidToken(token) { - // Delete the cookie - middleware.SetCookie(ctx.Resp, x.GetCookieName(), "", - -1, - x.GetCookiePath(), - x.GetCookieDomain()) // FIXME: Do we need to set the Secure, httpOnly and SameSite values too? - if middleware.IsAPIPath(ctx.Req) { - x.Error(ctx.Resp) - return - } + func (c *csrfProtector) validateToken(ctx *Context, token string) { + if !ValidCsrfToken(token, c.Secret, c.ID, "POST", time.Now()) { + middleware.DeleteCSRFCookie(ctx.Resp) + if middleware.IsAPIPath(ctx.Req) { + // currently, there should be no access to the APIPath with CSRF token. because templates shouldn't use the `/api/` endpoints. + http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest) + } else { ctx.Flash.Error(ctx.Tr("error.invalid_csrf")) ctx.Redirect(setting.AppSubURL + "/") } - return } - if token := ctx.Req.FormValue(x.GetFormName()); len(token) > 0 { - if !x.ValidToken(token) { - // Delete the cookie - middleware.SetCookie(ctx.Resp, x.GetCookieName(), "", - -1, - x.GetCookiePath(), - x.GetCookieDomain()) // FIXME: Do we need to set the Secure, httpOnly and SameSite values too? - if middleware.IsAPIPath(ctx.Req) { - x.Error(ctx.Resp) - return - } - ctx.Flash.Error(ctx.Tr("error.invalid_csrf")) - ctx.Redirect(setting.AppSubURL + "/") - } + } + + // Validate should be used as a per route middleware. It attempts to get a token from an "X-Csrf-Token" + // HTTP header and then a "_csrf" form value. If one of these is found, the token will be validated. + // If this validation fails, custom Error is sent in the reply. + // If neither a header nor form value is found, http.StatusBadRequest is sent. + func (c *csrfProtector) Validate(ctx *Context) { + if token := ctx.Req.Header.Get(c.GetHeaderName()); token != "" { + c.validateToken(ctx, token) return } - if middleware.IsAPIPath(ctx.Req) { - http.Error(ctx.Resp, "Bad Request: no CSRF token present", http.StatusBadRequest) + if token := ctx.Req.FormValue(c.GetFormName()); token != "" { + c.validateToken(ctx, token) return } - ctx.Flash.Error(ctx.Tr("error.missing_csrf")) - ctx.Redirect(setting.AppSubURL + "/") + c.validateToken(ctx, "") // no csrf token, use an empty token to respond error }
• modules/context/xsrf.go
  • @@ -28,69 +28,69 @@ import (
    "time" ) - // Timeout represents the duration that XSRF tokens are valid. + // CsrfTokenTimeout represents the duration that XSRF tokens are valid. // It is exported so clients may set cookie timeouts that match generated tokens. - const Timeout = 24 * time.Hour + const CsrfTokenTimeout = 24 * time.Hour - // clean sanitizes a string for inclusion in a token by replacing all ":"s. - func clean(s string) string { - return strings.ReplaceAll(s, ":", "_") - } + // CsrfTokenRegenerationInterval is the interval between token generations, old tokens are still valid before CsrfTokenTimeout + var CsrfTokenRegenerationInterval = 10 * time.Minute - // GenerateToken returns a URL-safe secure XSRF token that expires in 24 hours. - // + var csrfTokenSep = []byte(":") + + // GenerateCsrfToken returns a URL-safe secure XSRF token that expires in CsrfTokenTimeout hours. // key is a secret key for your application. // userID is a unique identifier for the user. // actionID is the action the user is taking (e.g. POSTing to a particular path). - func GenerateToken(key, userID, actionID string) string { - return generateTokenAtTime(key, userID, actionID, time.Now()) - } - - // generateTokenAtTime is like Generate, but returns a token that expires 24 hours from now. - func generateTokenAtTime(key, userID, actionID string, now time.Time) string { + func GenerateCsrfToken(key, userID, actionID string, now time.Time) string { + nowUnixNano := now.UnixNano() + nowUnixNanoStr := strconv.FormatInt(nowUnixNano, 10) h := hmac.New(sha1.New, []byte(key)) - fmt.Fprintf(h, "%s:%s:%d", clean(userID), clean(actionID), now.UnixNano()) - tok := fmt.Sprintf("%s:%d", h.Sum(nil), now.UnixNano()) + h.Write([]byte(strings.ReplaceAll(userID, ":", "_"))) + h.Write(csrfTokenSep) + h.Write([]byte(strings.ReplaceAll(actionID, ":", "_"))) + h.Write(csrfTokenSep) + h.Write([]byte(nowUnixNanoStr)) + tok := fmt.Sprintf("%s:%s", h.Sum(nil), nowUnixNanoStr) return base64.RawURLEncoding.EncodeToString([]byte(tok)) } - // ValidToken returns true if token is a valid, unexpired token returned by Generate. - func ValidToken(token, key, userID, actionID string) bool { - return validTokenAtTime(token, key, userID, actionID, time.Now()) - } - - // validTokenAtTime is like Valid, but it uses now to check if the token is expired. - func validTokenAtTime(token, key, userID, actionID string, now time.Time) bool { - // Decode the token. + func ParseCsrfToken(token string) (issueTime time.Time, ok bool) { data, err := base64.RawURLEncoding.DecodeString(token) if err != nil { - return false + return time.Time{}, false } - // Extract the issue time of the token. - sep := bytes.LastIndex(data, []byte{':'}) - if sep < 0 { - return false + pos := bytes.LastIndex(data, csrfTokenSep) + if pos == -1 { + return time.Time{}, false } - nanos, err := strconv.ParseInt(string(data[sep+1:]), 10, 64) + nanos, err := strconv.ParseInt(string(data[pos+1:]), 10, 64) if err != nil { + return time.Time{}, false + } + return time.Unix(0, nanos), true + } + + // ValidCsrfToken returns true if token is a valid and unexpired token returned by Generate. + func ValidCsrfToken(token, key, userID, actionID string, now time.Time) bool { + issueTime, ok := ParseCsrfToken(token) + if !ok { return false } - issueTime := time.Unix(0, nanos) // Check that the token is not expired. - if now.Sub(issueTime) >= Timeout { + if now.Sub(issueTime) >= CsrfTokenTimeout { return false } // Check that the token is not from the future. - // Allow 1 minute grace period in case the token is being verified on a + // Allow 1-minute grace period in case the token is being verified on a // machine whose clock is behind the machine that issued the token. if issueTime.After(now.Add(1 * time.Minute)) { return false } - expected := generateTokenAtTime(key, userID, actionID, issueTime) + expected := GenerateCsrfToken(key, userID, actionID, issueTime) // Check that the token matches the expected value. // Use constant time comparison to avoid timing attacks.
• modules/context/xsrf_test.go
  • @@ -37,18 +37,18 @@ var (
    func Test_ValidToken(t *testing.T) { t.Run("Validate token", func(t *testing.T) { - tok := generateTokenAtTime(key, userID, actionID, now) - assert.True(t, validTokenAtTime(tok, key, userID, actionID, oneMinuteFromNow)) - assert.True(t, validTokenAtTime(tok, key, userID, actionID, now.Add(Timeout-1*time.Nanosecond))) - assert.True(t, validTokenAtTime(tok, key, userID, actionID, now.Add(-1*time.Minute))) + tok := GenerateCsrfToken(key, userID, actionID, now) + assert.True(t, ValidCsrfToken(tok, key, userID, actionID, oneMinuteFromNow)) + assert.True(t, ValidCsrfToken(tok, key, userID, actionID, now.Add(CsrfTokenTimeout-1*time.Nanosecond))) + assert.True(t, ValidCsrfToken(tok, key, userID, actionID, now.Add(-1*time.Minute))) }) } // Test_SeparatorReplacement tests that separators are being correctly substituted func Test_SeparatorReplacement(t *testing.T) { t.Run("Test two separator replacements", func(t *testing.T) { - assert.NotEqual(t, generateTokenAtTime("foo:bar", "baz", "wah", now), - generateTokenAtTime("foo", "bar:baz", "wah", now)) + assert.NotEqual(t, GenerateCsrfToken("foo:bar", "baz", "wah", now), + GenerateCsrfToken("foo", "bar:baz", "wah", now)) }) }
  • @@ -61,13 +61,13 @@ func Test_InvalidToken(t *testing.T) {
    {"Bad key", "foobar", userID, actionID, oneMinuteFromNow}, {"Bad userID", key, "foobar", actionID, oneMinuteFromNow}, {"Bad actionID", key, userID, "foobar", oneMinuteFromNow}, - {"Expired", key, userID, actionID, now.Add(Timeout)}, + {"Expired", key, userID, actionID, now.Add(CsrfTokenTimeout)}, {"More than 1 minute from the future", key, userID, actionID, now.Add(-1*time.Nanosecond - 1*time.Minute)}, } - tok := generateTokenAtTime(key, userID, actionID, now) + tok := GenerateCsrfToken(key, userID, actionID, now) for _, itt := range invalidTokenTests { - assert.False(t, validTokenAtTime(tok, itt.key, itt.userID, itt.actionID, itt.t)) + assert.False(t, ValidCsrfToken(tok, itt.key, itt.userID, itt.actionID, itt.t)) } }) }
  • @@ -84,7 +84,7 @@ func Test_ValidateBadData(t *testing.T) {
    } for _, bdt := range badDataTests { - assert.False(t, validTokenAtTime(bdt.tok, key, userID, actionID, oneMinuteFromNow)) + assert.False(t, ValidCsrfToken(bdt.tok, key, userID, actionID, oneMinuteFromNow)) } }) }
• modules/web/middleware/cookie.go
  • @@ -98,17 +98,6 @@ func DeleteRedirectToCookie(resp http.ResponseWriter) {
    SameSite(setting.SessionConfig.SameSite)) } - // DeleteSesionConfigPathCookie convenience function to delete SessionConfigPath cookies consistently - func DeleteSesionConfigPathCookie(resp http.ResponseWriter, name string) { - SetCookie(resp, name, "", - -1, - setting.SessionConfig.CookiePath, - setting.SessionConfig.Domain, - setting.SessionConfig.Secure, - true, - SameSite(setting.SessionConfig.SameSite)) - } - // DeleteCSRFCookie convenience function to delete SessionConfigPath cookies consistently func DeleteCSRFCookie(resp http.ResponseWriter) { SetCookie(resp, setting.CSRFCookieName, "",
  • @@ -117,7 +106,7 @@ func DeleteCSRFCookie(resp http.ResponseWriter) {
    setting.SessionConfig.Domain) // FIXME: Do we need to set the Secure, httpOnly and SameSite values too? } - // SetCookie set the cookies + // SetCookie set the cookies. (name, value, lifetime, path, domain, secure, httponly, expires, {sameSite, ...}) // TODO: Copied from gitea.com/macaron/macaron and should be improved after macaron removed. func SetCookie(resp http.ResponseWriter, name, value string, others ...interface{}) { cookie := http.Cookie{}
• routers/web/auth/auth.go
  • @@ -345,7 +345,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe
    ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req) } - // Clear whatever CSRF has right now, force to generate a new one + // Clear whatever CSRF cookie has right now, force to generate a new one middleware.DeleteCSRFCookie(ctx.Resp) // Register last login
• routers/web/auth/oauth.go
  • @@ -1007,7 +1007,7 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
    log.Error("Error storing session: %v", err) } - // Clear whatever CSRF has right now, force to generate a new one + // Clear whatever CSRF cookie has right now, force to generate a new one middleware.DeleteCSRFCookie(ctx.Resp) // Register last login