Implement cross-site request forgery prevention

[GoogleCodeExporter]

Take a look at 
http://www.tornadoweb.org/documentation/web.html#tornado.web.RequestHandler.chec
k_xsrf_cookie and adapt this idea for webapp2.

Original issue reported on code.google.com by gmarke...@gmail.com on 12 Aug 2011 at 4:08

[GoogleCodeExporter]

Here is a patch I did during the PyCon 2012 sprints.

Add protection to a Handler;
    def get(self):
        xsrf_token = self.xsrf_token
        # or
        xsrf_token_input_html = self.xsrf_form_html()
        # ... include that in your forms or capture the value from the cookie _xsrf for XHR
    def post(self):
        # xsrf protect
        self.check_xsrf_cookie()
        # ... process request as before

Original comment by johnwloc...@gmail.com on 14 Mar 2012 at 7:59

Attachments:

• xsrf_protection_patch.diff

[GoogleCodeExporter]

Quick comment: I honestly don't like a lot to have extended handlers in the 
core. Maybe it could be put in extras.

Original comment by rodrigo.moraes on 15 Mar 2012 at 12:30

• Changed state: Accepted

[GoogleCodeExporter]

I'm going to be away for a few weeks so I'll have to look at this when I get 
back. I've assigned the bug to myself so that I don't lose track of it.

rodrigo.moraes: I haven't looked at the code in detail but security might be an 
area where you want to compromise on keeping the framework light.

Original comment by bquin...@google.com on 15 Mar 2012 at 5:37

[GoogleCodeExporter]

One thing: checking that the cookie is equal to an HTTP parameter is vulnerable 
to man-in-the-middle attacks. Maybe it does make sense to put this in extras 
and store the token in memcache/datastore.

But I still have to look at the code.

Original comment by bquin...@google.com on 15 Mar 2012 at 5:49

[GoogleCodeExporter]

man-in-the-middle attack, hmmm..., ok, I'll read up on that.
If this is done as an extra do you think it should be done as a subclass of 
RequestHandler?
I'd like it to be easy to add to existing app, without users having to change 
parent classes of their handlers. With reading and writing cookies, it needs 
access to the handler's request and response.
Setting the cookie is helpful for client side js to read the token into an XHR 
header. It could be backed up with memecache or datastore, maybe just as a 
session variable. Isn't a session ID also vulnerable to MITM? 

Original comment by johnwloc...@gmail.com on 15 Mar 2012 at 6:44

[GoogleCodeExporter]

I think there's no MITM as long as cookies are HttpsOnly and all traffic goes 
over HTTPS.

Original comment by a...@cloudware.it on 21 Sep 2012 at 9:06

[GoogleCodeExporter]

The problem is that browsers allow secure cookies to be overwritten by content 
served over HTTP.

So an attacker with control of your network can redirect you to an insecure 
version of the site, set your cookie to a value of their choosing and then make 
requests with the _xsrf form value matching the cookie that you set.

Original comment by bquin...@google.com on 25 Sep 2012 at 1:53

[GoogleCodeExporter]

One possible approach would be to force the developer to provide some unique 
information to produce a verifiable token for use in forms e.g.

token = base64( (hmac( user_id + DELIMITER + action + DELIMITER + time, secret) 
+ DELIMITER + time );

Original comment by bquin...@google.com on 25 Sep 2012 at 2:37

[GoogleCodeExporter]

then the developer would verify the token when the form was submitted? such as 
only that specific action by that user can be performed.

Original comment by johnwloc...@gmail.com on 25 Sep 2012 at 4:09

[GoogleCodeExporter]

Right. I think that it is safe to just provide user_id but action provides some 
optional safety. Action could be a URL.

Original comment by bquin...@google.com on 25 Sep 2012 at 4:37

[GoogleCodeExporter]

Brian, for what you said in c#7 to be possible I, as an attacker, would need: 
(1) insecure version of the site, (2) know how to create a valid cookie.

If I had either (1) or (2), I wouldn't bother with CSRF. I could just steal 
user's session, or do whatever I wanted to that user anyway.

What I'm saying is, if the above was possible, any fix for this issue wouldn't 
save developers from bad things to happen anyway. 

I guess a simple token (I wouldn't do hmac or anything) will do it: 
webapp2_extras.sessions is already using quite secure implementation. I'd just 
rely on that instead of having a separate cookie.

Am I missing something?

Original comment by a...@cloudware.it on 25 Sep 2012 at 6:55

[GoogleCodeExporter]

Alex, did you look at the proposed patch? The generated cookie is a GUID and no 
server-side validation is done so (2) is possible.

But I can't see how you could steal the user's session since you have no way of 
capturing the cookie (assuming that the "Secure" attribute were added).

I haven't looked into webapp2_extras.sessions so I don't know if using that 
module would be appropriate.

Original comment by bquin...@google.com on 25 Sep 2012 at 7:09

[GoogleCodeExporter]

Sorry, I didn't explain myself clearly. 
Yes, I looked at the patch and indeed (2) is possible.

What I meant to say earlier is that 
the patch + your hmac addition is kind of what webapp2_extras.sessions does 
already.

I just don't see a big advantage in using a separate cookie plus its secure 
implememtation when it is already provided by webapp2's extras.

Original comment by a...@cloudware.it on 25 Sep 2012 at 7:21

[GoogleCodeExporter]

I have started an csrf extras module that has a class CsrfProof with a method 
to create a token as Brian describes and a method that verifies it. The action 
is optional. The secret key would be set using the configuration system, 
requiring the user to set the secret key in their app's config. The token would 
be added as input to forms in the template or as a 'X-CSRFToken' header for XHR 
requests. The handler GET method would use a CsrfProof instance to make a token 
to send to the client and the POST method would use a CsrfProof instance to 
verify the token. it is more involved than using a decorator on the handler 
method, but if it's secure, I can add that later.
If a user wanted to use the action arg, they would have to explicitly configure 
it and a single page AJAX app may require multiple tokens, one for each action, 
so a generic 'csrf_token' context variable or a cookie wouldn't be appropriate.

The reason sessions isn't enough is because you don't need a session to get the 
user from google.appengine.api.users.get_current_user()

How does this sound?

Original comment by johnwloc...@gmail.com on 25 Sep 2012 at 5:31

[GoogleCodeExporter]

Sorry but I don't get why you should involve Users API for a simple CSRF guard. 
Plus, it (Users API) uses a cookie behind the scenes anyway. This would mean 
that I, as a developer, will be forced to use Google Accounts (or OpenID) any 
time I wanted to have a CSRF guard. Or implement it myself (let's say I want a 
custom auth or OAuth 2.0 which isn't provided but Users API, etc.)

90% of what you've described is already implemented in webapp2. I'd just hate 
it for this nice framework to have different pieces of code reimplementing the 
same functionality.

Original comment by a...@cloudware.it on 25 Sep 2012 at 7:04

[GoogleCodeExporter]

Actually what I have so far doesn't use the Users API, it only has a user id 
argument which can be whatever the developer wants.
Are you saying: since sessions uses a secure cookie, it makes it invulnerable 
to CSRF? or I should SecureCookieSerializer to serialize/deserialize the user 
id and action into a csrf_token?

Original comment by johnwloc...@gmail.com on 25 Sep 2012 at 8:50

[GoogleCodeExporter]

What I had in mind is something along these lines:

def get(self):
  token = any_random_token_or_generate_from_webapp2_security()
  self.session['xsrf_token'] = token
  # do what this is supposed to do: 
  # render a template, etc.

def post(self):
  token = self.request.get('xsrf_token')
  if token != self.session['xsrf_token']:
    raise InvalidXSRFToken
  # XSRF check pass

Where self.session could be 
webapp2_extras.sessions.get_store(request=self.request).get_session(), or 
whatever a developer wants it to be.

The above doesn't have any timestamp check though. Here's another example, at 
the bottom of this file: 
http://code.google.com/p/gae-simpleauth/source/browse/simpleauth/handler.py#461

Original comment by a...@cloudware.it on 25 Sep 2012 at 9:08

[GoogleCodeExporter]

Just wanted to add that the above is very similar to the original patch, only 
that it uses webapp2_extras.sessions.

Original comment by a...@cloudware.it on 25 Sep 2012 at 9:11

[GoogleCodeExporter]

Alex, I think that your approach is sufficient for XSRF but adding the user_id 
to the token generation/verification has the virtue of also preventing an 
attacker from using their cookie/xsrf_token to act as another user i.e. in your 
code example, I have to do additional security checks to make sure that the 
user is acting on their own data. If the user_id is included in the token 
generation/validation then tokens belonging to another user are automatically 
rejected.

Maybe providing a user_id should be optional?

Original comment by bquin...@google.com on 25 Sep 2012 at 9:30

[GoogleCodeExporter]

Maybe :) I mean, user_id thing seems more like a piece of an app logic/model. 
What if I don't have a user_id. A classic blog post with comments, only that I 
don't require users to sign in to comment, I just don't want a bunch of spam 
posted on my blog using XSRF.

If I understand you correctly, user_id check needs to be done in a case where 
an attacker would do a legitimate registration/sign in only to wait for another 
user to do XSRF on their behalf. In that case the bad user won't be able to 
guess the other's token anyway, if generated using something like 
webapp2_security.generate_random_string().

Besides, webapp2_extras.auth already has a "user_attributes" config param for 
this: 
http://code.google.com/p/webapp-improved/source/browse/webapp2_extras/auth.py#48
 which can be used to store stuff like user_id.

Sorry if I'm being a pain in the ass. I'm not :) I'm just curious.

Original comment by a...@cloudware.it on 25 Sep 2012 at 9:59

[GoogleCodeExporter]

Here is my webapp2_extra.csrf_proof

Original comment by johnwloc...@gmail.com on 27 Sep 2012 at 7:20

Attachments:

• csrf_proof.py

[GoogleCodeExporter]

Alex, you aren't being a pain in the ass. The point of including a user id is 
providing defence-in-depth.

Original comment by bquin...@google.com on 27 Sep 2012 at 7:26

[GoogleCodeExporter]

The csrf_token method automatically sets a cookie. Should it be optional? If 
the site creates a number of action specific tokens, there would be a cookie 
for each one, causing that extra overhead for each request. perhaps they would 
want to just pass the tokens through the response body where needed.

If the developer creates tokens with no user id or action, would that make the 
secret_key vulnerable, or does the sha1 protect it? 

Original comment by johnwloc...@gmail.com on 27 Sep 2012 at 8:59

[GoogleCodeExporter]

Sorry for the slow reply but I've been trying to get my head around this. I 
like the last version! There are some things that I don't understand or might 
change:
1. why are the cookies necessary at all?
2. maybe the token validation logic and the framework integration should be 
decoupled i.e. have a class like:

class XSRFToken(object):
  def __init__(self, user_id, secret, current_time=None): ... # time for unit tests
  def generate_token(self, action=None): ...
  def verify_token(self, token, action=None, timeout=None): ...

and then you can build the higher-level functionality out of this (and some 
people may just use this class).

I'm not sure how the higher-level functionality should be packaged. I think 
that calling handler methods directly might be a bit inflexible but maybe not. 
I'll try experimenting with it a bit.

Original comment by bquin...@google.com on 3 Oct 2012 at 4:24

[GoogleCodeExporter]

I guess cookie gives more flexible approach. For instance, you're doing some 
AJAX calls to interface with a backend. In this case browser should just pick 
up all cookies (including XSRF) automatically when xhr.send(). In fact, I've 
seen other frameworks stuff XSRF token in multiple places: header, <meta> tag 
and hidden <input> in a form to cover most of the use cases.

BTW, I like this high level XSRFToken class with just a couple methods a lot. I 
still don't get why "action" and "user_id" params are there (since they can't 
be forged anyway if signed with hmac or equivalent; I'd just use a random 
number/string instead of user_id+action to make it more flexible). I do get the 
defence-in-depth argument itself though. Anyway, I just probably don't see all 
possible scenarios in my head.

Original comment by a...@cloudware.it on 3 Oct 2012 at 8:08

[GoogleCodeExporter]

I don't understand what you mean by a "random number/string". The string must 
be uniquely associated with a user.

Original comment by bquin...@google.com on 4 Oct 2012 at 1:55

[GoogleCodeExporter]

Brian, the reason I have the time argument in generate token is so the time 
decoded from the token being verified can be used to recreate the token for 
comparison. I also like the XSRFToken being separate from the framework, but 
think the integration should be included in the module, because it is a 
webapp2_extra. Here is a version with XSRFToken as it's own class.

Original comment by johnwloc...@gmail.com on 4 Oct 2012 at 3:18

Attachments:

• csrf_proof.py

[GoogleCodeExporter]

Guys, I'm really sorry for being silent for so long. I've taken John's patch 
and changed a few things:
1. made the style a bit more consistent
2. added documentation
3. verify_token raises instead of returning a bool (I'm worried that people 
won't check and attackers will slip through).

I also removed the CsrfProof class for now since I want to look at it a bit 
before adding it. But I think that XSRFToken is solid and useful enough to be 
committed on its own.

Could anyone interested provide feedback here:
https://codereview.appspot.com/6847123/patch/3/5002

Original comment by bquin...@google.com on 29 Nov 2012 at 5:19

[GoogleCodeExporter]

Sorry, this is a better review URL:
https://codereview.appspot.com/6847123/

Original comment by bquin...@google.com on 30 Nov 2012 at 3:30

[GoogleCodeExporter]

I didn't see any new feedback, so I checked-in my proposed final change:
http://code.google.com/p/webapp-improved/source/browse/webapp2_extras/xsrf.py

Thanks to John for all his hard work!

Original comment by bquin...@google.com on 31 Dec 2012 at 2:37

[GoogleCodeExporter]

It looks pretty good Brian! Thanks for polishing it up and adding the tests. We 
made it before the end of the year. :)

Original comment by johnwloc...@gmail.com on 31 Dec 2012 at 3:16

[GoogleCodeExporter]

I'm glad that timing attack prevention was put in. it took me a minute to 
understand what was going on there. Let me describe in as simple terms as I 
can: 
The attacker enters made up values, and times how long it takes to compare 
against the correct value.  It takes different times to compare different 
values, and if this time is the same as a known time for a known comparison, 
the correct value can be determined.
The change makes comparing any pair of strings of equal length take the same 
amount of time, thus nullifying the timing attack.

Original comment by johnwloc...@gmail.com on 31 Dec 2012 at 4:36

[GoogleCodeExporter]

Great that there is now a XSRF-prevention tool available in webapp2_extras. Are 
there any chances that we will see a webapp 2.5.3 in GAE soon that includes the 
added extra?

Original comment by marc.nieper@gmail.com on 29 Aug 2013 at 7:54