Refactor CSRF protector (#32057) (#32069)

#32057 improves the CSRF handling and is worth to backport
pull/32074/head
wxiaoguang 2 months ago committed by GitHub
parent 8dbe83d205
commit 2891edbbcb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 2
      options/locale/locale_en-US.ini
  2. 2
      routers/web/web.go
  3. 6
      services/context/context.go
  4. 182
      services/context/csrf.go
  5. 3
      tests/integration/attachment_test.go
  6. 26
      tests/integration/csrf_test.go
  7. 12
      tests/integration/repo_branch_test.go

@ -218,8 +218,6 @@ string.desc = Z - A
[error] [error]
occurred = An error occurred occurred = An error occurred
report_message = If you believe that this is a Gitea bug, please search for issues on <a href="https://github.com/go-gitea/gitea/issues" target="_blank">GitHub</a> or open a new issue if necessary. report_message = If you believe that this is a Gitea bug, please search for issues on <a href="https://github.com/go-gitea/gitea/issues" target="_blank">GitHub</a> or open a new issue if necessary.
missing_csrf = Bad Request: no CSRF token present
invalid_csrf = Bad Request: invalid CSRF token
not_found = The target couldn't be found. not_found = The target couldn't be found.
network_error = Network error network_error = Network error

@ -129,6 +129,8 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) {
// ensure the session uid is deleted // ensure the session uid is deleted
_ = ctx.Session.Delete("uid") _ = ctx.Session.Delete("uid")
} }
ctx.Csrf.PrepareForSessionUser(ctx)
} }
} }

@ -138,10 +138,8 @@ func Contexter() func(next http.Handler) http.Handler {
csrfOpts := CsrfOptions{ csrfOpts := CsrfOptions{
Secret: hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), Secret: hex.EncodeToString(setting.GetGeneralTokenSigningSecret()),
Cookie: setting.CSRFCookieName, Cookie: setting.CSRFCookieName,
SetCookie: true,
Secure: setting.SessionConfig.Secure, Secure: setting.SessionConfig.Secure,
CookieHTTPOnly: setting.CSRFCookieHTTPOnly, CookieHTTPOnly: setting.CSRFCookieHTTPOnly,
Header: "X-Csrf-Token",
CookieDomain: setting.SessionConfig.Domain, CookieDomain: setting.SessionConfig.Domain,
CookiePath: setting.SessionConfig.CookiePath, CookiePath: setting.SessionConfig.CookiePath,
SameSite: setting.SessionConfig.SameSite, SameSite: setting.SessionConfig.SameSite,
@ -167,7 +165,7 @@ func Contexter() func(next http.Handler) http.Handler {
ctx.Base.AppendContextValue(WebContextKey, ctx) ctx.Base.AppendContextValue(WebContextKey, ctx)
ctx.Base.AppendContextValueFunc(gitrepo.RepositoryContextKey, func() any { return ctx.Repo.GitRepo }) ctx.Base.AppendContextValueFunc(gitrepo.RepositoryContextKey, func() any { return ctx.Repo.GitRepo })
ctx.Csrf = PrepareCSRFProtector(csrfOpts, ctx) ctx.Csrf = NewCSRFProtector(csrfOpts)
// Get the last flash message from cookie // Get the last flash message from cookie
lastFlashCookie := middleware.GetSiteCookie(ctx.Req, CookieNameFlash) lastFlashCookie := middleware.GetSiteCookie(ctx.Req, CookieNameFlash)
@ -204,8 +202,6 @@ func Contexter() func(next http.Handler) http.Handler {
ctx.Resp.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) ctx.Resp.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions)
ctx.Data["SystemConfig"] = setting.Config() ctx.Data["SystemConfig"] = setting.Config()
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 // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these
ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations

@ -20,64 +20,42 @@
package context package context
import ( import (
"encoding/base32" "html/template"
"fmt"
"net/http" "net/http"
"strconv" "strconv"
"time" "time"
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware" )
const (
CsrfHeaderName = "X-Csrf-Token"
CsrfFormName = "_csrf"
) )
// CSRFProtector represents a CSRF protector and is used to get the current token and validate the token. // CSRFProtector represents a CSRF protector and is used to get the current token and validate the token.
type CSRFProtector interface { type CSRFProtector interface {
// GetHeaderName returns HTTP header to search for token. // PrepareForSessionUser prepares the csrf protector for the current session user.
GetHeaderName() string PrepareForSessionUser(ctx *Context)
// GetFormName returns form value to search for token. // Validate validates the csrf token in http context.
GetFormName() string
// GetToken returns the token.
GetToken() string
// Validate validates the token in http context.
Validate(ctx *Context) Validate(ctx *Context)
// DeleteCookie deletes the cookie // DeleteCookie deletes the csrf cookie
DeleteCookie(ctx *Context) DeleteCookie(ctx *Context)
} }
type csrfProtector struct { type csrfProtector struct {
opt CsrfOptions opt CsrfOptions
// Token generated to pass via header, cookie, or hidden form value. // id must be unique per user.
Token string id string
// This value must be unique per user. // token is the valid one which wil be used by end user and passed via header, cookie, or hidden form value.
ID string token string
}
// GetHeaderName returns the name of the HTTP header for csrf token.
func (c *csrfProtector) GetHeaderName() string {
return c.opt.Header
}
// GetFormName returns the name of the form value for csrf token.
func (c *csrfProtector) GetFormName() string {
return c.opt.Form
}
// GetToken returns the current token. This is typically used
// to populate a hidden form in an HTML template.
func (c *csrfProtector) GetToken() string {
return c.Token
} }
// CsrfOptions maintains options to manage behavior of Generate. // CsrfOptions maintains options to manage behavior of Generate.
type CsrfOptions struct { type CsrfOptions struct {
// The global secret value used to generate Tokens. // The global secret value used to generate Tokens.
Secret string Secret string
// HTTP header used to set and get token.
Header string
// Form value used to set and get token.
Form string
// Cookie value used to set and get token. // Cookie value used to set and get token.
Cookie string Cookie string
// Cookie domain. // Cookie domain.
@ -87,103 +65,64 @@ type CsrfOptions struct {
CookieHTTPOnly bool CookieHTTPOnly bool
// SameSite set the cookie SameSite type // SameSite set the cookie SameSite type
SameSite http.SameSite SameSite http.SameSite
// Key used for getting the unique ID per user.
SessionKey string
// oldSessionKey saves old value corresponding to SessionKey.
oldSessionKey string
// If true, send token via X-Csrf-Token header.
SetHeader bool
// If true, send token via _csrf cookie.
SetCookie bool
// Set the Secure flag to true on the cookie. // Set the Secure flag to true on the cookie.
Secure bool Secure bool
// Disallow Origin appear in request header. // sessionKey is the key used for getting the unique ID per user.
Origin bool sessionKey string
// Cookie lifetime. Default is 0 // oldSessionKey saves old value corresponding to sessionKey.
CookieLifeTime int oldSessionKey string
}
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
panic(fmt.Errorf("failed to generate random bytes: %w", err))
}
opt.Secret = base32.StdEncoding.EncodeToString(randBytes)
}
if opt.Header == "" {
opt.Header = "X-Csrf-Token"
}
if opt.Form == "" {
opt.Form = "_csrf"
}
if opt.Cookie == "" {
opt.Cookie = "_csrf"
}
if opt.CookiePath == "" {
opt.CookiePath = "/"
}
if opt.SessionKey == "" {
opt.SessionKey = "uid"
}
if opt.CookieLifeTime == 0 {
opt.CookieLifeTime = int(CsrfTokenTimeout.Seconds())
}
opt.oldSessionKey = "_old_" + opt.SessionKey
return opt
} }
func newCsrfCookie(c *csrfProtector, value string) *http.Cookie { func newCsrfCookie(opt *CsrfOptions, value string) *http.Cookie {
return &http.Cookie{ return &http.Cookie{
Name: c.opt.Cookie, Name: opt.Cookie,
Value: value, Value: value,
Path: c.opt.CookiePath, Path: opt.CookiePath,
Domain: c.opt.CookieDomain, Domain: opt.CookieDomain,
MaxAge: c.opt.CookieLifeTime, MaxAge: int(CsrfTokenTimeout.Seconds()),
Secure: c.opt.Secure, Secure: opt.Secure,
HttpOnly: c.opt.CookieHTTPOnly, HttpOnly: opt.CookieHTTPOnly,
SameSite: c.opt.SameSite, SameSite: opt.SameSite,
} }
} }
// PrepareCSRFProtector returns a CSRFProtector to be used for every request. func NewCSRFProtector(opt CsrfOptions) CSRFProtector {
// Additionally, depending on options set, generated tokens will be sent via Header and/or Cookie. if opt.Secret == "" {
func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector { panic("CSRF secret is empty but it must be set") // it shouldn't happen because it is always set in code
opt = prepareDefaultCsrfOptions(opt) }
x := &csrfProtector{opt: opt} opt.Cookie = util.IfZero(opt.Cookie, "_csrf")
opt.CookiePath = util.IfZero(opt.CookiePath, "/")
if opt.Origin && len(ctx.Req.Header.Get("Origin")) > 0 { opt.sessionKey = "uid"
return x opt.oldSessionKey = "_old_" + opt.sessionKey
return &csrfProtector{opt: opt}
} }
x.ID = "0" func (c *csrfProtector) PrepareForSessionUser(ctx *Context) {
uidAny := ctx.Session.Get(opt.SessionKey) c.id = "0"
if uidAny != nil { if uidAny := ctx.Session.Get(c.opt.sessionKey); uidAny != nil {
switch uidVal := uidAny.(type) { switch uidVal := uidAny.(type) {
case string: case string:
x.ID = uidVal c.id = uidVal
case int64: case int64:
x.ID = strconv.FormatInt(uidVal, 10) c.id = strconv.FormatInt(uidVal, 10)
default: default:
log.Error("invalid uid type in session: %T", uidAny) log.Error("invalid uid type in session: %T", uidAny)
} }
} }
oldUID := ctx.Session.Get(opt.oldSessionKey) oldUID := ctx.Session.Get(c.opt.oldSessionKey)
uidChanged := oldUID == nil || oldUID.(string) != x.ID uidChanged := oldUID == nil || oldUID.(string) != c.id
cookieToken := ctx.GetSiteCookie(opt.Cookie) cookieToken := ctx.GetSiteCookie(c.opt.Cookie)
needsNew := true needsNew := true
if uidChanged { if uidChanged {
_ = ctx.Session.Set(opt.oldSessionKey, x.ID) _ = ctx.Session.Set(c.opt.oldSessionKey, c.id)
} else if cookieToken != "" { } else if cookieToken != "" {
// If cookie token presents, re-use existing unexpired token, else generate a new one. // If cookie token presents, re-use existing unexpired token, else generate a new one.
if issueTime, ok := ParseCsrfToken(cookieToken); ok { 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. 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 { if dur >= -CsrfTokenRegenerationInterval && dur <= CsrfTokenRegenerationInterval {
x.Token = cookieToken c.token = cookieToken
needsNew = false needsNew = false
} }
} }
@ -191,42 +130,33 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
if needsNew { if needsNew {
// FIXME: actionId. // FIXME: actionId.
x.Token = GenerateCsrfToken(x.opt.Secret, x.ID, "POST", time.Now()) c.token = GenerateCsrfToken(c.opt.Secret, c.id, "POST", time.Now())
if opt.SetCookie { cookie := newCsrfCookie(&c.opt, c.token)
cookie := newCsrfCookie(x, x.Token)
ctx.Resp.Header().Add("Set-Cookie", cookie.String()) ctx.Resp.Header().Add("Set-Cookie", cookie.String())
} }
}
if opt.SetHeader { ctx.Data["CsrfToken"] = c.token
ctx.Resp.Header().Add(opt.Header, x.Token) ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + template.HTMLEscapeString(c.token) + `">`)
}
return x
} }
func (c *csrfProtector) validateToken(ctx *Context, token string) { func (c *csrfProtector) validateToken(ctx *Context, token string) {
if !ValidCsrfToken(token, c.opt.Secret, c.ID, "POST", time.Now()) { if !ValidCsrfToken(token, c.opt.Secret, c.id, "POST", time.Now()) {
c.DeleteCookie(ctx) c.DeleteCookie(ctx)
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. // currently, there should be no access to the APIPath with CSRF token. because templates shouldn't use the `/api/` endpoints.
// FIXME: distinguish what the response is for: HTML (web page) or JSON (fetch)
http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest) http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest)
} else {
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" // 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. // 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 this validation fails, http.StatusBadRequest is sent.
// If neither a header nor form value is found, http.StatusBadRequest is sent.
func (c *csrfProtector) Validate(ctx *Context) { func (c *csrfProtector) Validate(ctx *Context) {
if token := ctx.Req.Header.Get(c.GetHeaderName()); token != "" { if token := ctx.Req.Header.Get(CsrfHeaderName); token != "" {
c.validateToken(ctx, token) c.validateToken(ctx, token)
return return
} }
if token := ctx.Req.FormValue(c.GetFormName()); token != "" { if token := ctx.Req.FormValue(CsrfFormName); token != "" {
c.validateToken(ctx, token) c.validateToken(ctx, token)
return return
} }
@ -234,9 +164,7 @@ func (c *csrfProtector) Validate(ctx *Context) {
} }
func (c *csrfProtector) DeleteCookie(ctx *Context) { func (c *csrfProtector) DeleteCookie(ctx *Context) {
if c.opt.SetCookie { cookie := newCsrfCookie(&c.opt, "")
cookie := newCsrfCookie(c, "")
cookie.MaxAge = -1 cookie.MaxAge = -1
ctx.Resp.Header().Add("Set-Cookie", cookie.String()) ctx.Resp.Header().Add("Set-Cookie", cookie.String())
} }
}

@ -59,7 +59,8 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri
func TestCreateAnonymousAttachment(t *testing.T) { func TestCreateAnonymousAttachment(t *testing.T) {
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
session := emptyTestSession(t) session := emptyTestSession(t)
createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusSeeOther) // this test is not right because it just doesn't pass the CSRF validation
createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest)
} }
func TestCreateIssueAttachment(t *testing.T) { func TestCreateIssueAttachment(t *testing.T) {

@ -5,12 +5,10 @@ package integration
import ( import (
"net/http" "net/http"
"strings"
"testing" "testing"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/tests" "code.gitea.io/gitea/tests"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@ -25,28 +23,12 @@ func TestCsrfProtection(t *testing.T) {
req := NewRequestWithValues(t, "POST", "/user/settings", map[string]string{ req := NewRequestWithValues(t, "POST", "/user/settings", map[string]string{
"_csrf": "fake_csrf", "_csrf": "fake_csrf",
}) })
session.MakeRequest(t, req, http.StatusSeeOther) resp := session.MakeRequest(t, req, http.StatusBadRequest)
assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
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 // test web form csrf via header. TODO: should use an UI api to test
req = NewRequest(t, "POST", "/user/settings") req = NewRequest(t, "POST", "/user/settings")
req.Header.Add("X-Csrf-Token", "fake_csrf") req.Header.Add("X-Csrf-Token", "fake_csrf")
session.MakeRequest(t, req, http.StatusSeeOther) resp = session.MakeRequest(t, req, http.StatusBadRequest)
assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
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()),
)
} }

@ -17,7 +17,6 @@ import (
repo_model "code.gitea.io/gitea/models/repo" repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/modules/translation"
@ -146,15 +145,8 @@ func TestCreateBranchInvalidCSRF(t *testing.T) {
"_csrf": "fake_csrf", "_csrf": "fake_csrf",
"new_branch_name": "test", "new_branch_name": "test",
}) })
resp := session.MakeRequest(t, req, http.StatusSeeOther) resp := session.MakeRequest(t, req, http.StatusBadRequest)
loc := resp.Header().Get("Location") assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
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()),
)
} }
func prepareBranch(t *testing.T, session *TestSession, repo *repo_model.Repository) { func prepareBranch(t *testing.T, session *TestSession, repo *repo_model.Repository) {

Loading…
Cancel
Save