Always check CSRF token if request has body

[dougwilson]

This is a placeholder for the idea to alter this module to check for the CSRF token when the request has a body, regardless of the method. Any request containing a body should fall under CSRF checking (since it is likely modifying state).

@jonathanong @Fishrock123 @defunctzombie thoughts?

[jonathanong]

meh. a lot of methods like GET, HEAD, and DELETE shouldn't even have bodies.

[dougwilson]

Right, but they can, which may lead to someone sending a body with a GET request and (if the user incorrectly placed method-override after this module) could be interpreted as a POST. I'm proposing the check to be

if (hasbody(req) || ('GET' !== req.method && 'HEAD' !== req.method && 'OPTIONS' !== req.method)) {
  checkCSRF()
}

[Fishrock123]

FWIW this is a response to http://blog.nibblesec.org/2014/05/nodejs-connect-csrf-bypass-abusing.html

From email, since yahoo's being stupid and not sending to @jonathanong

(dougwilson)

I would like to draft a security section into the csurf readme, any objections? My thoughts are to outline the general problem of placing the middleware above logic that alters req.method (and specifically calling out method-override).

In addition, what do you think about altering the module such that instead of just ignoring specific methods, we also never ignore requests that have a body?

Just because they shouldn't have bodies doesn't mean that they won't.

[jonathanong]

my general philosophy is to educate vs. handle all these cases. i'd rather add that link to the readme and tell developers what they should and shouldn't be doing.

but if we add this in and somebody complains, we could always tell them, "stop". haha

[dougwilson]

Unfortunately when it comes to security, people can shoot themselves in the foot, but we should at least make them try harder to do this. Currently all it takes is

app.use(csrf()) // #1
app.use(methodOverride()) // #2
// explode!

We can always go around chanting "be sure method-override comes before csrf" but that doesn't seem very proactive to me. If all it took was education, those blog articles may not exist.

but if we add this in and somebody complains, we could always tell them, "stop". haha

people never ask questions and just copy-pasta all day long. once one example on the interweb does csrf before method-override, it'll just be everywhere.

[dougwilson]

In fact, the only times I see people asking security-related questions in projects is because they are wondering how to do something that is not secure at all and wondering why they are not being allowed to (or how it is hard to make it insecure).

[jonathanong]

yup. what we can do is just 403 if the original method is idempotent and not support request bodies on these methods. if they want to do something insecure, they should do it themselves.

we could also change body-parser to do the same.

[dougwilson]

what we can do is just 403 if the original method is idempotent

I think I like this better than just checking the CSRF token and carrying on, though if a method is idempotent would need to be a whitelist instead of a blacklist to be safe...

[jonathanong]

@visionmedia koajs/koa#274 can you move in the methods lib here so we can do some awesome over engineering?

[dougwilson]

Once issue I can see, though, is that DELETE and PUT is defined to be idempotent, but I'm sure people would like to CSRF token check on those methods; I'm no longer sure that method idempotentency directly maps to what should check the CSRF token.

[dougwilson]

I feel like I have circled back around to liking my original proposal better again...

[dougwilson]

lol, I just thought of a crazy new idea: check the CSRF token how it is, but then set req.method to no longer be a writable property :P

[Fishrock123]

I think I like the idea of the original check returning a 403 the most.

[jonathanong]

i'm so confused now. haha. i'm down for that too. haha

[dougwilson]

i'm so confused now. haha. i'm down for that too. haha

lol, sorry. just brainstroming over here :P

I think I like the idea of the original check returning a 403 the most.

@Fishrock123 which check was that (you can always quote it is link to it)?

[Fishrock123]

@dougwilson -- checking for a body.

if (hasbody(req) || ('GET' !== req.method && 'HEAD' !== req.method && 'OPTIONS' !== req.method) {
  checkCSRF()
}

[dougwilson]

So to continue this real quick, if I were to say add the hasbody check, should there be an option to turn it off? i.m.o no, because people already cannot influence that list of methods. Does this make sense to you guys?

[Fishrock123]

If any option, I'd add an option to still checkCRSF() or just 403. I dunno if we should be that giving even that leeway though.

This is CSRF prevention, it's going to be strict.

[dougwilson]

Fair enough, @Fishrock123 that sounds good to me. csurf 2.0, here we come!

[dougwilson]

By the way, having these modules not included in connect/express is AWESOME. We can make breaking changes in them, bump the major version, and not care!

[Fishrock123]

If we are talking 2.0, docs needs to be landed first for sure. #7

[dougwilson]

@Fishrock123 I'll make a milestone in here.