From 9c5b27b5f6acb62b09fc3bfc6b9b33230c35f138 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 6 Apr 2022 21:05:31 +0800 Subject: [PATCH 1/7] Refactor CSRF --- modules/context/api.go | 7 +- modules/context/auth.go | 2 +- modules/context/context.go | 6 +- modules/context/csrf.go | 199 +++++++++++-------------------- modules/context/xsrf.go | 66 +++++----- modules/context/xsrf_test.go | 20 ++-- modules/web/middleware/cookie.go | 2 +- routers/api/v1/api.go | 2 +- routers/web/auth/auth.go | 2 +- routers/web/auth/oauth.go | 2 +- 10 files changed, 124 insertions(+), 184 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index ae516503e4a23..1e8d8ee887011 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -8,7 +8,6 @@ package context import ( "context" "fmt" - "html" "net/http" "net/url" "strings" @@ -212,7 +211,7 @@ func (ctx *APIContext) RequireCSRF() { headerToken := ctx.Req.Header.Get(ctx.csrf.GetHeaderName()) formValueToken := ctx.Req.FormValue(ctx.csrf.GetFormName()) if len(headerToken) > 0 || len(formValueToken) > 0 { - Validate(ctx.Context, ctx.csrf) + ctx.csrf.Validate(ctx.Context) } else { ctx.Context.Error(http.StatusUnauthorized, "Missing CSRF token.") } @@ -289,7 +288,7 @@ func APIContexter() func(http.Handler) http.Handler { } ctx.Req = WithAPIContext(WithContext(req, ctx.Context), &ctx) - ctx.csrf = Csrfer(csrfOpts, ctx.Context) + ctx.csrf = NewCSRFProtector(csrfOpts, ctx.Context) // If request sends files, parse them here otherwise the Query() can't be parsed and the CsrfToken will be invalid. if ctx.Req.Method == "POST" && strings.Contains(ctx.Req.Header.Get("Content-Type"), "multipart/form-data") { @@ -301,7 +300,7 @@ func APIContexter() func(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["Context"] = &ctx next.ServeHTTP(ctx.Resp, ctx.Req) diff --git a/modules/context/auth.go b/modules/context/auth.go index 1a46ab586a607..09c22954550c7 100644 --- a/modules/context/auth.go +++ b/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 } diff --git a/modules/context/context.go b/modules/context/context.go index f73b5f19c0c67..5c397919f33bc 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -57,7 +57,7 @@ type Context struct { Render Render translation.Locale Cache cache.Cache - csrf CSRF + csrf CSRFProtector Flash *middleware.Flash Session session.Store @@ -679,7 +679,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 = NewCSRFProtector(csrfOpts, &ctx) // Get flash. flashCookie := ctx.GetCookie("macaron_flash") @@ -737,7 +737,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(``) // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these diff --git a/modules/context/csrf.go b/modules/context/csrf.go index 4fc92705048c9..f668dda3d5239 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -31,40 +31,30 @@ 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 { - // Header name value for setting and getting csrf token. +type csrfProtector struct { + // Header name value for setting and getting CSRF token. Header string - // Form name value for setting and getting csrf token. + // Form name value for setting and getting CSRF token. Form string - // Cookie name value for setting and getting csrf token. + // Cookie name value for setting and getting CSRF token. Cookie string // Cookie domain CookieDomain string // Cookie path CookiePath string - // Cookie HttpOnly flag value used for the csrf token. + // Cookie HttpOnly flag value used for the CSRF token. CookieHTTPOnly bool // Token generated to pass via header, cookie, or hidden form value. Token string @@ -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 { +// GetHeaderName returns the name of the HTTP header for CSRF token. +func (c *csrfProtector) GetHeaderName() string { return c.Header } -// GetFormName returns the name of the form value for csrf token. -func (c *csrf) GetFormName() string { +// GetFormName returns the name of the form value for CSRF token. +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. +// NewCSRFProtector 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 NewCSRFProtector(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 { @@ -236,17 +179,25 @@ func Csrfer(opt CsrfOptions, ctx *Context) CSRF { _ = 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 + if val := ctx.GetCookie(opt.Cookie); val != "" { + x.Token = val // FIXME: test coverage. } else { needsNew = true } } + if !needsNew { + if issueTime, ok := ParseCsrfToken(x.Token); ok { + dur := time.Since(issueTime) + if dur < -CsrfTokenRegenerationDuration || dur > CsrfTokenRegenerationDuration { + needsNew = true + } + } + } + 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 { @@ -270,47 +221,39 @@ 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) bool { + if !ValidCsrfToken(token, c.Secret, c.ID, "POST", time.Now()) { + // Delete the cookie + middleware.SetCookie(ctx.Resp, c.Cookie, "", + -1, + c.CookiePath, + c.CookieDomain) // FIXME: Do we need to set the Secure, httpOnly and SameSite values too? + + if middleware.IsAPIPath(ctx.Req) { + http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest) + } else { ctx.Flash.Error(ctx.Tr("error.invalid_csrf")) ctx.Redirect(setting.AppSubURL + "/") } - return + return false } - 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 + "/") + return true +} + +// 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 +// using ValidToken. 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 != "" { + if c.validateToken(ctx, token) { + return } - return } - if middleware.IsAPIPath(ctx.Req) { - http.Error(ctx.Resp, "Bad Request: no CSRF token present", http.StatusBadRequest) - return + if token := ctx.Req.FormValue(c.GetFormName()); token != "" { + if 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 } diff --git a/modules/context/xsrf.go b/modules/context/xsrf.go index 10e63a4180c61..f94ae66d652c5 100644 --- a/modules/context/xsrf.go +++ b/modules/context/xsrf.go @@ -28,69 +28,67 @@ 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 +const CsrfTokenRegenerationDuration = 1 * time.Minute -// clean sanitizes a string for inclusion in a token by replacing all ":"s. -func clean(s string) string { - return strings.ReplaceAll(s, ":", "_") -} +var csrfTokenSep = []byte(":") -// GenerateToken returns a URL-safe secure XSRF token that expires in 24 hours. -// +// 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. diff --git a/modules/context/xsrf_test.go b/modules/context/xsrf_test.go index c0c711bf07cdc..ef42d61d5a8dd 100644 --- a/modules/context/xsrf_test.go +++ b/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)) } }) } diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 80fe302137364..99056ba5cf7b0 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -117,7 +117,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{} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 2c2926389021d..f32864481dac3 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -609,7 +609,7 @@ func Routes(sessioner func(http.Handler) http.Handler) *web.Route { // setting.CORSConfig.AllowSubdomain // FIXME: the cors middleware needs allowSubdomain option AllowedMethods: setting.CORSConfig.Methods, AllowCredentials: setting.CORSConfig.AllowCredentials, - AllowedHeaders: []string{"Authorization", "X-CSRFToken", "X-Gitea-OTP"}, + AllowedHeaders: []string{"Authorization", "X-Csrf-Token", "X-Gitea-OTP"}, MaxAge: int(setting.CORSConfig.MaxAge.Seconds()), })) } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index ab538f0e5f603..c82fde49eb2f4 100644 --- a/routers/web/auth/auth.go +++ b/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 diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 4369c333ac0d5..12de208ad7bc1 100644 --- a/routers/web/auth/oauth.go +++ b/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 From abc4e6d1557763feb12bff3107a4463e08b9d96f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 6 Apr 2022 21:43:57 +0800 Subject: [PATCH 2/7] use middleware.DeleteCSRFCookie to replace duplicate code --- modules/context/csrf.go | 7 +------ modules/web/middleware/cookie.go | 11 ----------- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/modules/context/csrf.go b/modules/context/csrf.go index f668dda3d5239..2c2d685c2c8de 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -223,12 +223,7 @@ func NewCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { func (c *csrfProtector) validateToken(ctx *Context, token string) bool { if !ValidCsrfToken(token, c.Secret, c.ID, "POST", time.Now()) { - // Delete the cookie - middleware.SetCookie(ctx.Resp, c.Cookie, "", - -1, - c.CookiePath, - c.CookieDomain) // FIXME: Do we need to set the Secure, httpOnly and SameSite values too? - + middleware.DeleteCSRFCookie(ctx.Resp) if middleware.IsAPIPath(ctx.Req) { http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest) } else { diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 99056ba5cf7b0..b5904d6713a66 100644 --- a/modules/web/middleware/cookie.go +++ b/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, "", From 70f87103b47637a302f4dd675cda1548faa535aa Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 6 Apr 2022 22:00:21 +0800 Subject: [PATCH 3/7] refactor --- modules/context/csrf.go | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/modules/context/csrf.go b/modules/context/csrf.go index 2c2d685c2c8de..c0f3ac135e48b 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -172,25 +172,20 @@ func NewCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { } } - needsNew := false oldUID := ctx.Session.Get(opt.oldSessionKey) - if oldUID == nil || oldUID.(string) != x.ID { - needsNew = true - _ = 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); val != "" { - x.Token = val // FIXME: test coverage. - } else { - needsNew = true - } - } + uidChanged := oldUID == nil || oldUID.(string) != x.ID + cookieToken := ctx.GetCookie(opt.Cookie) - if !needsNew { + needsNew := true + if uidChanged { + _ = ctx.Session.Set(opt.oldSessionKey, x.ID) + } else if cookieToken != "" { + // If cookie token presents, re-use existing unexpired token, else generate a new one. if issueTime, ok := ParseCsrfToken(x.Token); ok { dur := time.Since(issueTime) - if dur < -CsrfTokenRegenerationDuration || dur > CsrfTokenRegenerationDuration { - needsNew = true + if dur >= -CsrfTokenRegenerationDuration && dur <= CsrfTokenRegenerationDuration { + x.Token = cookieToken + needsNew = false } } } From 850a9fd13c6bd7dce4221529363eab5e9fffc400 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 7 Apr 2022 11:10:58 +0800 Subject: [PATCH 4/7] fine tune and add comments --- modules/context/api.go | 2 +- modules/context/context.go | 6 ++++-- modules/context/csrf.go | 24 ++++++++++++------------ modules/context/xsrf.go | 4 +++- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index 1e8d8ee887011..7769ab359105c 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -288,7 +288,7 @@ func APIContexter() func(http.Handler) http.Handler { } ctx.Req = WithAPIContext(WithContext(req, ctx.Context), &ctx) - ctx.csrf = NewCSRFProtector(csrfOpts, ctx.Context) + ctx.csrf = PrepareCSRFProtector(csrfOpts, ctx.Context) // If request sends files, parse them here otherwise the Query() can't be parsed and the CsrfToken will be invalid. if ctx.Req.Method == "POST" && strings.Contains(ctx.Req.Header.Get("Content-Type"), "multipart/form-data") { diff --git a/modules/context/context.go b/modules/context/context.go index 5c397919f33bc..04f94f3bcbcde 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -648,7 +648,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) @@ -679,7 +681,7 @@ func Contexter() func(next http.Handler) http.Handler { ctx.Data["Context"] = &ctx ctx.Req = WithContext(req, &ctx) - ctx.csrf = NewCSRFProtector(csrfOpts, &ctx) + ctx.csrf = PrepareCSRFProtector(csrfOpts, &ctx) // Get flash. flashCookie := ctx.GetCookie("macaron_flash") diff --git a/modules/context/csrf.go b/modules/context/csrf.go index c0f3ac135e48b..712a1609e8e9f 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -44,17 +44,17 @@ type CSRFProtector interface { } type csrfProtector struct { - // Header name value for setting and getting CSRF token. + // Header name value for setting and getting csrf token. Header string - // Form name value for setting and getting CSRF token. + // Form name value for setting and getting csrf token. Form string - // Cookie name value for setting and getting CSRF token. + // Cookie name value for setting and getting csrf token. Cookie string // Cookie domain CookieDomain string // Cookie path CookiePath string - // Cookie HttpOnly flag value used for the CSRF token. + // Cookie HttpOnly flag value used for the csrf token. CookieHTTPOnly bool // Token generated to pass via header, cookie, or hidden form value. Token string @@ -64,12 +64,12 @@ type csrfProtector struct { Secret string } -// GetHeaderName returns the name of the HTTP header for CSRF token. +// GetHeaderName returns the name of the HTTP header for csrf token. func (c *csrfProtector) GetHeaderName() string { return c.Header } -// GetFormName returns the name of the form value for CSRF token. +// GetFormName returns the name of the form value for csrf token. func (c *csrfProtector) GetFormName() string { return c.Form } @@ -141,9 +141,9 @@ func prepareDefaultCsrfOptions(opt CsrfOptions) CsrfOptions { return opt } -// NewCSRFProtector returns a CSRFProtector to be used for every request. +// 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 NewCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { +func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { opt = prepareDefaultCsrfOptions(opt) x := &csrfProtector{ Secret: opt.Secret, @@ -181,9 +181,9 @@ func NewCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { _ = ctx.Session.Set(opt.oldSessionKey, x.ID) } else if cookieToken != "" { // If cookie token presents, re-use existing unexpired token, else generate a new one. - if issueTime, ok := ParseCsrfToken(x.Token); ok { - dur := time.Since(issueTime) - if dur >= -CsrfTokenRegenerationDuration && dur <= CsrfTokenRegenerationDuration { + 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 } @@ -196,7 +196,7 @@ func NewCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { 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, diff --git a/modules/context/xsrf.go b/modules/context/xsrf.go index f94ae66d652c5..e3ecc82f6d75c 100644 --- a/modules/context/xsrf.go +++ b/modules/context/xsrf.go @@ -31,7 +31,9 @@ import ( // CsrfTokenTimeout represents the duration that XSRF tokens are valid. // It is exported so clients may set cookie timeouts that match generated tokens. const CsrfTokenTimeout = 24 * time.Hour -const CsrfTokenRegenerationDuration = 1 * time.Minute + +// CsrfTokenRegenerationInterval is the interval between token generations, old tokens are still valid before CsrfTokenTimeout +var CsrfTokenRegenerationInterval = 10 * time.Minute var csrfTokenSep = []byte(":") From 909e82d7a4f445fb26d334e5bd4c48b0b59cf16f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 8 Apr 2022 12:01:09 +0800 Subject: [PATCH 5/7] fine tune CSRF response, tested --- modules/context/csrf.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/modules/context/csrf.go b/modules/context/csrf.go index 712a1609e8e9f..0c99dc422a2a0 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -216,7 +216,7 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { return x } -func (c *csrfProtector) validateToken(ctx *Context, token string) bool { +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) { @@ -225,25 +225,21 @@ func (c *csrfProtector) validateToken(ctx *Context, token string) bool { ctx.Flash.Error(ctx.Tr("error.invalid_csrf")) ctx.Redirect(setting.AppSubURL + "/") } - return false } - return true } // 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 -// using ValidToken. If this validation fails, custom Error is sent in the reply. +// 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 != "" { - if c.validateToken(ctx, token) { - return - } + c.validateToken(ctx, token) + return } if token := ctx.Req.FormValue(c.GetFormName()); token != "" { - if c.validateToken(ctx, token) { - return - } + c.validateToken(ctx, token) + return } c.validateToken(ctx, "") // no csrf token, use an empty token to respond error } From c55c5ea0a085b2f56fb2cf071aef97a86a0ad28d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 8 Apr 2022 12:42:26 +0800 Subject: [PATCH 6/7] add csrf test --- integrations/csrf_test.go | 52 +++++++++++++++++++++++++++++++++ integrations/repo_topic_test.go | 1 + 2 files changed, 53 insertions(+) create mode 100644 integrations/csrf_test.go diff --git a/integrations/csrf_test.go b/integrations/csrf_test.go new file mode 100644 index 0000000000000..5bfc97bbd136f --- /dev/null +++ b/integrations/csrf_test.go @@ -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()), + ) +} diff --git a/integrations/repo_topic_test.go b/integrations/repo_topic_test.go index 146f90e710f21..e049afdd7c19d 100644 --- a/integrations/repo_topic_test.go +++ b/integrations/repo_topic_test.go @@ -10,6 +10,7 @@ import ( "testing" api "code.gitea.io/gitea/modules/structs" + "github.com/stretchr/testify/assert" ) From b1babfefc63b65538ba2a538c16988ec8ec2cb2c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 8 Apr 2022 12:49:28 +0800 Subject: [PATCH 7/7] add a comment for APIPath with CSRF token --- modules/context/csrf.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/context/csrf.go b/modules/context/csrf.go index 0c99dc422a2a0..df775048cb62c 100644 --- a/modules/context/csrf.go +++ b/modules/context/csrf.go @@ -220,6 +220,7 @@ 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"))