From 7cc66ee1d45c27bceddbd9f4f2869b307aff2c8d Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Thu, 31 Jan 2019 20:39:02 +0100 Subject: [PATCH] [MM-10346] CSRF Token Implementation + Tests (#10067) * CSRF Token Implementation + Tests Remove debug statements Implement requested changes * Fix non-cookie authentication methods stripping auth data from requests * Fail when CSRF cookie is not returned as part of login --- api4/user_test.go | 18 +++++++- app/diagnostics.go | 1 + app/login.go | 11 +++++ app/plugin_requests.go | 35 +++++++++----- config/default.json | 3 +- model/client4.go | 1 + model/config.go | 5 ++ model/session.go | 1 + web/handlers.go | 61 ++++++++++++++---------- web/handlers_test.go | 102 +++++++++++++++++++++++++++++++++++++++++ 10 files changed, 202 insertions(+), 36 deletions(-) diff --git a/api4/user_test.go b/api4/user_test.go index efa9f2988e..a881ce624a 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -6,6 +6,7 @@ package api4 import ( "net/http" "strconv" + "strings" "testing" "time" @@ -27,7 +28,22 @@ func TestCreateUser(t *testing.T) { CheckNoError(t, resp) CheckCreatedStatus(t, resp) - th.Client.Login(user.Email, user.Password) + _, resp = th.Client.Login(user.Email, user.Password) + session, _ := th.App.GetSession(th.Client.AuthToken) + expectedCsrf := "MMCSRF=" + session.GetCSRF() + actualCsrf := "" + + for _, cookie := range resp.Header["Set-Cookie"] { + if strings.HasPrefix(cookie, "MMCSRF") { + cookieParts := strings.Split(cookie, ";") + actualCsrf = cookieParts[0] + break + } + } + + if expectedCsrf != actualCsrf { + t.Errorf("CSRF Mismatch - Expected %s, got %s", expectedCsrf, actualCsrf) + } if ruser.Nickname != user.Nickname { t.Fatal("nickname didn't match") diff --git a/app/diagnostics.go b/app/diagnostics.go index feba5f6563..1a9d0dc937 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -278,6 +278,7 @@ func (a *App) trackConfig() { "allow_cookies_for_subdomains": *cfg.ServiceSettings.AllowCookiesForSubdomains, "enable_api_team_deletion": *cfg.ServiceSettings.EnableAPITeamDeletion, "experimental_enable_hardened_mode": *cfg.ServiceSettings.ExperimentalEnableHardenedMode, + "experimental_strict_csrf_enforcement": *cfg.ServiceSettings.ExperimentalStrictCSRFEnforcement, "enable_email_invitations": *cfg.ServiceSettings.EnableEmailInvitations, "experimental_channel_organization": *cfg.ServiceSettings.ExperimentalChannelOrganization, "experimental_ldap_group_sync": *cfg.ServiceSettings.ExperimentalLdapGroupSync, diff --git a/app/login.go b/app/login.go index 01385e5429..1b5a67758b 100644 --- a/app/login.go +++ b/app/login.go @@ -189,8 +189,19 @@ func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, Secure: secure, } + csrfCookie := &http.Cookie{ + Name: model.SESSION_COOKIE_CSRF, + Value: session.GetCSRF(), + Path: "/", + MaxAge: maxAge, + Expires: expiresAt, + Domain: domain, + Secure: secure, + } + http.SetCookie(w, sessionCookie) http.SetCookie(w, userCookie) + http.SetCookie(w, csrfCookie) return session, nil } diff --git a/app/plugin_requests.go b/app/plugin_requests.go index 18c72c4d62..672237bccb 100644 --- a/app/plugin_requests.go +++ b/app/plugin_requests.go @@ -65,21 +65,34 @@ func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler r.Header.Del("Mattermost-User-Id") if token != "" { session, err := a.GetSession(token) - csrfCheckPassed := true + csrfCheckPassed := false - if err == nil && cookieAuth && r.Method != "GET" && r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML { - bodyBytes, _ := ioutil.ReadAll(r.Body) - r.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) - r.ParseForm() - sentToken := r.FormValue("csrf") - expectedToken := session.GetCSRF() + if err == nil && cookieAuth && r.Method != "GET" { + sentToken := "" - if sentToken != expectedToken { - csrfCheckPassed = false + if r.Header.Get(model.HEADER_CSRF_TOKEN) == "" { + bodyBytes, _ := ioutil.ReadAll(r.Body) + r.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) + r.ParseForm() + sentToken = r.FormValue("csrf") + r.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) + } else { + sentToken = r.Header.Get(model.HEADER_CSRF_TOKEN) } - // Set Request Body again, since otherwise form values aren't accessible in plugin handler - r.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) + expectedToken := session.GetCSRF() + + if sentToken == expectedToken { + csrfCheckPassed = true + } + + // ToDo(DSchalla) 2019/01/04: Remove after deprecation period and only allow CSRF Header (MM-13657) + if !*a.Config().ServiceSettings.ExperimentalStrictCSRFEnforcement && r.Header.Get(model.HEADER_REQUESTED_WITH) == model.HEADER_REQUESTED_WITH_XML && !csrfCheckPassed { + a.Log.Warn("CSRF Check failed for request - Please migrate your plugin to either send a CSRF Header or Form Field, XMLHttpRequest is deprecated") + csrfCheckPassed = true + } + } else { + csrfCheckPassed = true } if session != nil && err == nil && csrfCheckPassed { diff --git a/config/default.json b/config/default.json index 98012a5596..0a010016e3 100644 --- a/config/default.json +++ b/config/default.json @@ -74,7 +74,8 @@ "EnableAPITeamDeletion": false, "ExperimentalEnableHardenedMode": false, "EnableEmailInvitations": false, - "ExperimentalLdapGroupSync": false + "ExperimentalLdapGroupSync": false, + "ExperimentalStrictCSRFEnforcement": false }, "TeamSettings": { "SiteName": "Mattermost", diff --git a/model/client4.go b/model/client4.go index 18e8933a26..44cd43d46d 100644 --- a/model/client4.go +++ b/model/client4.go @@ -27,6 +27,7 @@ const ( HEADER_REAL_IP = "X-Real-IP" HEADER_FORWARDED_PROTO = "X-Forwarded-Proto" HEADER_TOKEN = "token" + HEADER_CSRF_TOKEN = "X-CSRF-Token" HEADER_BEARER = "BEARER" HEADER_AUTH = "Authorization" HEADER_REQUESTED_WITH = "X-Requested-With" diff --git a/model/config.go b/model/config.go index 0c043de41a..d7b30a9aa4 100644 --- a/model/config.go +++ b/model/config.go @@ -283,6 +283,7 @@ type ServiceSettings struct { DEPRECATED_DO_NOT_USE_ImageProxyOptions *string `json:"ImageProxyOptions"` // This field is deprecated and must not be used. EnableAPITeamDeletion *bool ExperimentalEnableHardenedMode *bool + ExperimentalStrictCSRFEnforcement *bool EnableEmailInvitations *bool ExperimentalLdapGroupSync *bool } @@ -611,6 +612,10 @@ func (s *ServiceSettings) SetDefaults() { if s.ExperimentalLdapGroupSync == nil { s.ExperimentalLdapGroupSync = NewBool(false) } + + if s.ExperimentalStrictCSRFEnforcement == nil { + s.ExperimentalStrictCSRFEnforcement = NewBool(false) + } } type ClusterSettings struct { diff --git a/model/session.go b/model/session.go index d59e9b1835..cb7189a666 100644 --- a/model/session.go +++ b/model/session.go @@ -12,6 +12,7 @@ import ( const ( SESSION_COOKIE_TOKEN = "MMAUTHTOKEN" SESSION_COOKIE_USER = "MMUSERID" + SESSION_COOKIE_CSRF = "MMCSRF" SESSION_CACHE_SIZE = 35000 SESSION_PROP_PLATFORM = "platform" SESSION_PROP_OS = "os" diff --git a/web/handlers.go b/web/handlers.go index 93669ccf24..dbff4ceaf9 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -62,16 +62,6 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { c.App.Path = r.URL.Path c.Log = c.App.Log - token, tokenLocation := app.ParseAuthTokenFromRequest(r) - - // CSRF Check - if tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester { - if r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML { - c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized) - token = "" - } - } - subpath, _ := utils.GetSubpathFromConfig(c.App.Config()) siteURLHeader := app.GetProtocol(r) + "://" + r.Host + subpath c.SetSiteURLHeader(siteURLHeader) @@ -97,26 +87,51 @@ func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } + token, tokenLocation := app.ParseAuthTokenFromRequest(r) + if len(token) != 0 { session, err := c.App.GetSession(token) + csrfCheckPassed := false - if err != nil { - c.Log.Info("Invalid session", mlog.Err(err)) - if err.StatusCode == http.StatusInternalServerError { - c.Err = err - } else if h.RequireSession { - c.RemoveSessionCookie(w, r) - c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token, http.StatusUnauthorized) + // CSRF Check + if tokenLocation == app.TokenLocationCookie && h.RequireSession && !h.TrustRequester && r.Method != "GET" { + csrfHeader := r.Header.Get(model.HEADER_CSRF_TOKEN) + if csrfHeader == session.GetCSRF() { + csrfCheckPassed = true + } else if !*c.App.Config().ServiceSettings.ExperimentalStrictCSRFEnforcement && r.Header.Get(model.HEADER_REQUESTED_WITH) == model.HEADER_REQUESTED_WITH_XML { + // ToDo(DSchalla) 2019/01/04: Remove after deprecation period and only allow CSRF Header (MM-13657) + c.Log.Warn("CSRF Header check failed for request - Please upgrade your web application or custom app to set a CSRF Header") + csrfCheckPassed = true + } + + if !csrfCheckPassed { + token = "" + session = nil + c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized) } - } else if !session.IsOAuth && tokenLocation == app.TokenLocationQueryString { - c.Err = model.NewAppError("ServeHTTP", "api.context.token_provided.app_error", nil, "token="+token, http.StatusUnauthorized) } else { - c.App.Session = *session + csrfCheckPassed = true } - // Rate limit by UserID - if c.App.Srv.RateLimiter != nil && c.App.Srv.RateLimiter.UserIdRateLimit(c.App.Session.UserId, w) { - return + if csrfCheckPassed { + if err != nil { + c.Log.Info("Invalid session", mlog.Err(err)) + if err.StatusCode == http.StatusInternalServerError { + c.Err = err + } else if h.RequireSession { + c.RemoveSessionCookie(w, r) + c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token, http.StatusUnauthorized) + } + } else if !session.IsOAuth && tokenLocation == app.TokenLocationQueryString { + c.Err = model.NewAppError("ServeHTTP", "api.context.token_provided.app_error", nil, "token="+token, http.StatusUnauthorized) + } else { + c.App.Session = *session + } + + // Rate limit by UserID + if c.App.Srv.RateLimiter != nil && c.App.Srv.RateLimiter.UserIdRateLimit(c.App.Session.UserId, w) { + return + } } } diff --git a/web/handlers_test.go b/web/handlers_test.go index 9741241818..252745e974 100644 --- a/web/handlers_test.go +++ b/web/handlers_test.go @@ -108,3 +108,105 @@ func TestHandlerServeHTTPSecureTransport(t *testing.T) { t.Errorf("Strict-Transport-Security header is not expected, but returned") } } + + +func handlerForCSRFToken(c *Context, w http.ResponseWriter, r *http.Request) { +} + +func TestHandlerServeCSRFToken(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + session :=&model.Session{ + UserId: th.BasicUser.Id, + CreateAt: model.GetMillis(), + Roles: model.SYSTEM_USER_ROLE_ID, + IsOAuth: false, + } + session.GenerateCSRF() + session.SetExpireInDays(1) + session, err := th.App.CreateSession(session) + if err != nil { + t.Errorf("Expected nil, got %s", err) + } + + web := New(th.Server, th.Server.AppOptions, th.Server.Router) + + handler := Handler{ + GetGlobalAppOptions: web.GetGlobalAppOptions, + HandleFunc: handlerForCSRFToken, + RequireSession: true, + TrustRequester: false, + RequireMfa: false, + IsStatic: false, + } + + cookie := &http.Cookie{ + Name: model.SESSION_COOKIE_USER, + Value: th.BasicUser.Username, + } + cookie2 := &http.Cookie{ + Name: model.SESSION_COOKIE_TOKEN, + Value: session.Token, + } + cookie3 := &http.Cookie{ + Name: model.SESSION_COOKIE_CSRF, + Value: session.GetCSRF(), + } + + // CSRF Token Used - Success Expected + + request := httptest.NewRequest("POST", "/api/v4/test", nil) + request.AddCookie(cookie) + request.AddCookie(cookie2) + request.AddCookie(cookie3) + request.Header.Add(model.HEADER_CSRF_TOKEN, session.GetCSRF()) + response := httptest.NewRecorder() + handler.ServeHTTP(response, request) + + if response.Code != 200 { + t.Errorf("Expected status 200, got %d", response.Code) + } + + // No CSRF Token Used - Failure Expected + + request = httptest.NewRequest("POST", "/api/v4/test", nil) + request.AddCookie(cookie) + request.AddCookie(cookie2) + request.AddCookie(cookie3) + response = httptest.NewRecorder() + handler.ServeHTTP(response, request) + + if response.Code != 401 { + t.Errorf("Expected status 401, got %d", response.Code) + } + + // Fallback Behavior Used - Success expected + // ToDo (DSchalla) 2019/01/04: Remove once legacy CSRF Handling is removed + th.App.UpdateConfig(func(config *model.Config){ + *config.ServiceSettings.ExperimentalStrictCSRFEnforcement = false + }) + request = httptest.NewRequest("POST", "/api/v4/test", nil) + request.AddCookie(cookie) + request.AddCookie(cookie2) + request.AddCookie(cookie3) + request.Header.Add(model.HEADER_REQUESTED_WITH, model.HEADER_REQUESTED_WITH_XML) + response = httptest.NewRecorder() + handler.ServeHTTP(response, request) + + if response.Code != 200 { + t.Errorf("Expected status 200, got %d", response.Code) + } + + // Fallback Behavior Used with Strict Enforcement - Failure Expected + // ToDo (DSchalla) 2019/01/04: Remove once legacy CSRF Handling is removed + th.App.UpdateConfig(func(config *model.Config){ + *config.ServiceSettings.ExperimentalStrictCSRFEnforcement = true + }) + response = httptest.NewRecorder() + handler.ServeHTTP(response, request) + + if response.Code != 401 { + t.Errorf("Expected status 200, got %d", response.Code) + } +}