In the previous post, I discussed the importance of well-written documentation and uncomplicated APIs suggesting that poor documentation and negligence should be considered as silent threats.
Almost a year ago, I reported the following issue to the Node.js Connect's maintainers. To me, this is a perfect example of the risks of an incomplete API documentation that doesn't clearly warn the user of potential side-effects. Please note that in the recent releases of Express, connect-csrf is now called csurf and methodOverride is now method-override. Different names, same API.
Disclosure timeline
This issue was reported to Senchalabs on 07/25/2013. Despite my requests to add a warning in the online documentation, there's still no indication of potential side-effects in Connect MethodOverride. On 09/07/2013, this advisory was also published by the NodeSecurity community. Unfortunately, I don't think that the issue raised the adequate level of attention as suggested by the many vulnerable applications that I've encountered.
Technical details
Connect’s methodOverride middleware allows an HTTP request to override the HTTP verb with the value of the _method post parameter or with the x-http-method-override header. As the declaration order of middlewares determines the execution stack in Connect, it is possible to abuse this functionality in order to bypass the standard Connect’s anti-CSRF protection.
Considering the following code:
... app.use express.csrf() ... app.use express.methodOverride()
Connect’s CSRF middleware does not check CSRF tokens in case of idempotent verbs (GET/HEAD/OPTIONS, see csurf/index.js). As a result, it is possible to bypass the security control by sending a GET request with a POST MethodOverride header or parameter.
GET / HTTP/1.1 [..] _method=POST
The workaround is clearly to disable methodOverride or make sure that it takes precedence over other middleware declarations.
Adam Baldwin made an eslint plugin that you can use to identify this issue.
Update 06/04: Douglas W. pointed out that it's probably a good idea to move to method-override version 2+ (https://www.npmjs.org/package/method-override#readme). The documentation has been updated with a reference to this issue.
Update 06/04: Douglas W. pointed out that it's probably a good idea to move to method-override version 2+ (https://www.npmjs.org/package/method-override#readme). The documentation has been updated with a reference to this issue.
8 comments:
May 6, 2014 at 8:52 AM
I'm curious, what would be the use case for allowing the verb to be overridden in the first place?
May 6, 2014 at 9:42 AM
Senchalabs doesn't even maintain Connect anymore.
When I searched connect's repo, I could not find this issue.
Plus, connect / express have been in a transition to use modules for all middleware.
All the middleware now resides in the express.js org: https://github.com/expressjs
May 6, 2014 at 10:44 AM
@Jake
methodOverride is generally used to simulate DELETE and PUT, but I've seen it as a way to build click-able form posting (instead of POST with multipart/form-data, etc.)
@Jeremiah
As I've mentioned in the post, they renamed the middleware but it's exactly the same code. Not sure who is currently the official maintainer for Express
May 7, 2014 at 11:59 AM
@Luca I'm a maintainer as a part of the organization. As fas as I can tell, we weren't notified of this.
We'll document security more in csurf and methodOverride. Extra checks may be added.
As a general suggestion, people probably shouldn't be using methodOverride, if possible. (Even though we provide it if people do happen to need it.)
May 7, 2014 at 12:56 PM
@Jeremiah
It's unfortunate that you haven't received any communication. https://github.com/visionmedia acknowledged the issue on July 25, 2013 saying "Good find! We should definitely add a note in the documentation at very least". After that, a few other emails but no changes in the code/doc.
Anyway, thanks for getting back on it. Much appreciated!
May 18, 2014 at 11:15 AM
As a side note, it makes me a bit happy to see a simple eslint rule to find a trivial issue is just mildly unreadable and extremely hard to understand, instead of an absolutely impossible spaghetti mess.
Progress!
September 12, 2015 at 4:09 AM
September 12, 2015 at 4:09 AM
Post a Comment