[MM-10346] CSRF Token Implementation for Plugins #9192

DSchalla · 2018-07-30T22:13:36Z

Summary

This PR implements CSRF token which are being attached to users sessions. The CSRF tokens are rolled out and can be enforced as alternative to XHR checks in the plugin request system. The plugin API was extended to allow a simple implementation in the plugins code base.

The PR lays the foundation for the further roll out in the MM core.

Ticket Link

https://mattermost.atlassian.net/browse/MM-10346

Checklist

Added or updated unit tests (required for all new features)
Touches critical sections of the codebase (auth, upgrade, etc.)

crspeller

Looks good! Just a couple comments.

crspeller · 2018-07-31T17:13:18Z

app/plugin_api.go

+		return nil, err
+	}
+
+	session.Token = ""


Lets give the plugins access to the session token.

crspeller · 2018-07-31T18:21:08Z

app/plugin_requests.go

+		session, err := a.GetSession(token)
+		csrfCheck := true
+
+		if err == nil && r.Method != "GET" && r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML {


We check the header for GET requests as well on the server. Let's match that behavior. (But don't check the token for GET requests.)

Never mind this is handled upstream.

DSchalla · 2018-07-31T19:13:32Z

Thanks for the review @crspeller!

lieut-data

Nice work, @DSchalla! A few comments below.

Also, @jwilander and @crspeller, it worries me that our Props fields have fixed lengths: 1000 in this case of the Session object. Have we run into issues before where new props (+ encoding overhead) overran this buffer size? Perhaps without being directly detected?

lieut-data · 2018-08-01T12:02:19Z

model/session.go

+		return ""
+	}
+
+	val, ok := me.Props["csrf"]


Props appears to be a map[string]string, so you can just return me.Props["csrf"] directly here and rely on the nil value of "".

Fixed - ty!

lieut-data · 2018-08-01T12:05:04Z

model/session_test.go

+	token := s.GetCSRF()
+
+	if token != "" {
+		t.Errorf("Expected empty, got %s", token)


nit: Might consider just using assert.Empty and assert.NotEqual below for simplicity.

Good call, thanks!

lieut-data · 2018-08-01T12:09:42Z

app/login.go

@@ -126,7 +126,7 @@ func (a *App) GetUserForLogin(id, loginId string) (*model.User, *model.AppError)

 func (a *App) DoLogin(w http.ResponseWriter, r *http.Request, user *model.User, deviceId string) (*model.Session, *model.AppError) {
 	session := &model.Session{UserId: user.Id, Roles: user.GetRawRoles(), DeviceId: deviceId, IsOAuth: false}
-
+	session.GenerateCSRF()


How do we decide whether or not to generate a CSRF token? For example, there's createSessionForUserAccessToken and oAuth's newSession -- should we be generating tokens in those cases as well?

Good call, I added it for now to oauth ~~and user access tokens as well~~ to have all cases covered. For user access tokens it could be discussed for sure.

lieut-data · 2018-08-01T12:13:09Z

plugin/api.go

+	// GetSession returns the session object for the Session ID
+	GetSession(sessionId string) (*model.Session, *model.AppError)
+
+	// GetCSRF returns the CSRF token for the session Id


Is GetCSRF strictly required, given one can call GetCSRF on the result of GetSession above? Hoping we can keep the plugin API lean, and introduce helpers as needed (e.g. Session.GetCSRF could check if the pointer receiver is nil and return "" directly).

The idea was to provide a as convenient integration as possible for the plugin developer, it's certainly not much more than a helper. That way you can get the CSRF token directly via the session id without fetching the session object itself.

lieut-data · 2018-08-01T12:16:36Z

plugin/context.go

@@ -7,4 +7,13 @@ package plugin
 //
 // It is currently a placeholder while the implementation details are sorted out.
 type Context struct {
+	SessionId string


I seem to recall @crspeller mentioning something about wanting to be able to change the wire encoding without breaking backwards compatibility. I'm not sure how that actually works, in practice, since the plugin will have compiled with this definition of Context and would need the field to be deserialized onto it /somehow/. Does that make the accessors below redundant, given it's a public field anyway?

I think the idea was to be able to make Context later on an interface without a lot of interface adaption, which won't work with fields. I am not 100% sure though, any comments @crspeller? Making the struct field private won't work because of the encoding for gob.

Ya, now that I think about this, I don't think we are going to be able to accomplish that. I think we can just remove the unnecessary helpers.

Helpers got removed in the latest version.

lieut-data

I hadn't caught up on the CSRF discussion on pre-release, so some of this might already be answered, but I threw a few questions to clarify my own understanding of the new restrictions.

Since Christopher's reviewed this code, I feel comfortable approving it, but welcome the continued discussion :)

lieut-data · 2018-08-01T19:57:39Z

app/plugin_requests.go

 		token = cookie.Value
 	} else {
 		token = r.URL.Query().Get("access_token")
 	}

 	r.Header.Del("Mattermost-User-Id")
 	if token != "" {
-		if session, err := a.GetSession(token); session != nil && err == nil {
+		session, err := a.GetSession(token)
+		csrfCheck := true


nit: csrfCheck read to me as check CSRF not csrfCheckPassed or some such.

Fixed, good point.

lieut-data · 2018-08-01T20:07:00Z

app/plugin_requests.go

@@ -38,22 +40,42 @@ func (a *App) ServePluginRequest(w http.ResponseWriter, r *http.Request) {

 func (a *App) servePluginRequest(w http.ResponseWriter, r *http.Request, handler func(*plugin.Context, http.ResponseWriter, *http.Request)) {
 	token := ""
+	context := &plugin.Context{}

 	authHeader := r.Header.Get(model.HEADER_AUTH)
 	if strings.HasPrefix(strings.ToUpper(authHeader), model.HEADER_BEARER+" ") {


This block of code interests me. I'm not sure why I missed it in the first round of reviews, but just some clarifying questions:

Will I now be prohibited from making authenticated requests through a plugin's API unless I manually send the HEADER_REQUESTED_WITH header with the requisite value? I think the webapp does this automatically for XMLHttpRequests, but what if I'm using curl/http to hit the plugin's API, or integrating with another application that interfaces with the API? (This might be by design and/or a requirement of CSRF protection, but just want to make sure.)

What if my plugin's API endpoint isn't parsing forms, but accepts JSON? How do I POST a JSON payload to my plugin endpoint /and/ set the csrf token in the body as a form value /and/ still have an authenticated request? I guess this boils down to the previous question requiring me to set this special header? Is it a standard header, or something custom we've adopted?

It looks like we support getting an access_token from the query string. Ignoring any security issues with that, I guess this happens to work because we don't issue access tokens corresponding csrf tokens. Of course, someone could pass their current session's token /as/ an access token of sorts, but I suppose we don't support that.

If you don't have the XMLHttpRequest header set you will be required to send a CSRF Token, that should not be the target when cookie authentication is used and was planned for a migration period, since that was the previous behavior. In the long-term I'd like to enforce CSRF tokens for all requests that were made with cookie auth.

You can send a csrf token via GET parameters as well, we could add a header option optionally. The url query parameter is supported ootb by FormValue in Go, the header would be an additional code enhancement.

That code snippet isn't from me, but according to my understanding access_token == session token != csrf token - Hope that helps?

Important: I added a new check that the CSRF enforcement is only applied for cookie authentication after our chat in pre-release. FYI @crspeller

deleted test config fix test config Dont wipe the session token for plugins Simplified Tokens; Generate CSRF for other sessions Remove CSRF from Access Token; Remove Getter/Setter from Context fix removed setter remove getcsrf helper from plugin api enforce csrf only for cookie auth

DSchalla · 2018-08-01T22:08:17Z

I think we got now everything covered, thanks to both of you @lieut-data and @crspeller for the quick review! Looking forward to release the first alpha of Claptrap, with this PR being the last blocker for the remaining features. 🎉

crspeller · 2018-08-01T22:16:00Z

@DSchalla Looking forward to the first release of Claptrap!

DSchalla force-pushed the plugin_csrftoken branch 2 times, most recently from f426725 to fc53647 Compare July 30, 2018 22:16

jasonblais requested review from jwilander and crspeller July 30, 2018 22:18

jasonblais added the 2: Dev Review Requires review by a developer label Jul 30, 2018

crspeller requested changes Jul 31, 2018

View reviewed changes

DSchalla force-pushed the plugin_csrftoken branch from fc53647 to 24cd0fe Compare July 31, 2018 19:12

crspeller approved these changes Jul 31, 2018

View reviewed changes

jasonblais requested a review from lieut-data July 31, 2018 22:22

lieut-data requested changes Aug 1, 2018

View reviewed changes

DSchalla force-pushed the plugin_csrftoken branch 4 times, most recently from addc58c to 7340cfd Compare August 1, 2018 19:48

lieut-data approved these changes Aug 1, 2018

View reviewed changes

DSchalla force-pushed the plugin_csrftoken branch from 7340cfd to 697d963 Compare August 1, 2018 20:25

crspeller merged commit 2936dc8 into mattermost:master Aug 1, 2018

lindalumitchell added the Tests/Not Needed New release tests not required label Aug 2, 2018

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Aug 24, 2018

lindy65 removed the 2: Dev Review Requires review by a developer label Feb 20, 2019

harshilsharma63 mentioned this pull request May 23, 2019

Migrate plugin from XMLHttpRequest standup-raven/standup-raven#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-10346] CSRF Token Implementation for Plugins #9192

[MM-10346] CSRF Token Implementation for Plugins #9192

DSchalla commented Jul 30, 2018

crspeller left a comment

crspeller Jul 31, 2018

DSchalla Jul 31, 2018

crspeller Jul 31, 2018

crspeller Jul 31, 2018

DSchalla commented Jul 31, 2018

lieut-data left a comment

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018 •

edited

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018 •

edited

lieut-data Aug 1, 2018 •

edited

DSchalla Aug 1, 2018 •

edited

crspeller Aug 1, 2018

DSchalla Aug 1, 2018

lieut-data left a comment

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018

lieut-data Aug 1, 2018

DSchalla Aug 1, 2018

DSchalla commented Aug 1, 2018

crspeller commented Aug 1, 2018

[MM-10346] CSRF Token Implementation for Plugins #9192

[MM-10346] CSRF Token Implementation for Plugins #9192

Conversation

DSchalla commented Jul 30, 2018

Summary

Ticket Link

Checklist

crspeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchalla commented Jul 31, 2018

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchalla Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchalla Aug 1, 2018 • edited

Choose a reason for hiding this comment

lieut-data Aug 1, 2018 • edited

Choose a reason for hiding this comment

DSchalla Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchalla commented Aug 1, 2018

crspeller commented Aug 1, 2018

DSchalla Aug 1, 2018 •

edited

DSchalla Aug 1, 2018 •

edited

lieut-data Aug 1, 2018 •

edited

DSchalla Aug 1, 2018 •

edited