This has the same vulnerability as csurf
psibean opened this issue · 7 comments
csurf was not deprecated with, "doubtful" reasoning.
The problem with csurf was that it's default implementation was vulnerable and insecure. And so is this fork as you have not fixed the problem and you have not updated the README with a security warning advising users that the package is only secure if they actually configure it correctly.
In order for this to be secure, the defaultValue function needs to be completely overridden, secure and signed need to be set to true, and sameSite needs to be set to strict (lax in some cases, but only if people actually know what they're doing).
And therein lies the problem, the packages default configuration should be secure in the first place.
You need to issue a security advisory via github here. This way npm will mark the package as insecure via npm audit
Please see this official OWASP video which details the csurf problem - the full video provides context, but the csurf specific part is at 11:32.
Hi @psibean , this is a shameless promo on your side! Many would just ban you from commenting in the repo, as happened to me a few times, but I am more open-minded towards a healthy discussion.
csurf was not deprecated with, "doubtful" reasoning
It was; the reasoning being, quote "This npm module is currently deprecated due to the large influx of security vulunerability reports received, most of which are simply exploiting the underlying limitations of CSRF itself."
The problem with csurf was that ... and therein lies the problem, the packages default configuration should be secure in the first place.
In other words, you agree there is no bugs in csurf, and thus neither this fork; it is just if people use it incorrectly (i.e. not reading carefully its README), with incorrect assumptions and expectations, it won't fulfil their expectations.
Please see this official OWASP video which details the csurf problem
To summaries the video in a few words: if a website is vulnerable to XSS, served without HTTPS, there is a middleman, and/or DNS poisoning, in these cases an evil dude can workaround CSRF protection done with double-submited cookie. I tell you: if these problems are present, the evil dude will hack users no matter whether there is CSRF protection or not, and "secure configuration" / alternative double-submit cookie implementation, etc. do not fix the problem entirely, just make it somewhat more difficult to hack. The aim of double-submit cookie CSRF is to protect against much less complicated exploit (completely independent site making request to your api from your browser); and against that this package protects just fine, even with default config.
You need to issue a security advisory via github here.
Nope, there is no security problem, as long as people know what are they doing and what are they protecting themselves from.
I would highly recommend people use my modern and secure package csrf-csrf for the double submit pattern (already 50k downloads/week), or csrf-sync (which was recently adopted by NodeBB over csurf) for the synchronised token pattern.
Yeah, if somebody wants to spend their time on adopting a different CSRF package, and hoping that you have not created any bugs when implementing it from scratch... fine for me. If somebody does not want to waste their time, and just want to use a long-established package, which just works, and occasionally receive updates of dependencies, etc. — here is my fork. By the way, the original, deprecated csurf still has ~550k downloads a week, the number seems larger than that before the deprecation). So, your package has a long way to go 🤣
Also, I just had a look at "Dependents" page of your csrf-csrf repo, and it really makes me curious, how do you arrive to ~50k NPM downloads / week with a package that has 35 stars, ~64 dependent repos / 3 dependent packages found by GitHub?
Say, here I have my babel-plugin-react-css-modules: 29 stars, ~94 dependent repos / 16 dependent packages ⇒ ~3.5k NPM downloads / week.
Hi @psibean , this is a shameless promo on your side! Many would just ban you from commenting in the repo, as happened to me a few times, but I am more open-minded towards a healthy discussion.
Agreed. I removed that, you can remove the links from your quote too.
In other words, you agree there is no bugs in csurf, and thus neither this fork; it is just if people use it incorrectly (i.e. not reading carefully its README), with incorrect assumptions and expectations, it won't fulfil their expectations.
Yes, and it should be on the maintainers to educate their users about how that should be done with the packages they maintain. If you'll accept a README PR, I'd be happy to make one within the few weeks.
Yeah, if somebody wants to spend their time on adopting a different CSRF package, and hoping that you have not created any bugs when implementing it from scratch... fine for me.
it's a plugin replacement with csurf really, they share the same API. The code is there, it's comparable to next-auth's implementation (among others), and people can have their system penetration tested while using it.
As for this being "secure by default" - I would still disagree. Cookie settings are not secure or signed by default, making susceptible to MITM without proper configuration (and there is no indication of this in the documentation).
It's true that certain things may be unnecessary due to other protections, or that the attacks aren't worth protecting against in this way, but you don't leave your safe unlocked just because your doors and windows are locked. You don't leave your car unlocked just because it's locked in a garage. There is always room for new attack vectors to be discovered in the future that are protected against by older means, making them kind of obsolete, but not if you just skip some of the best practices.
Check your own default settings in the README:
signed - indicates if the cookie should be signed (defaults to false).
secure - marks the cookie to be used with HTTPS only (defaults to false).
maxAge - the number of seconds after which the cookie will expire (defaults to session length).
httpOnly - flags the cookie to be accessible only by the web server (defaults to false).
sameSite - sets the same site policy for the cookie(defaults to false). This can be set to 'strict',
How are these default settings secure by default, exactly? These defaults 100% go against ALL OWASP recommendations. How can you argue these defaults are reasonable? The defaults, and the recommended prod settings should be:
signed: true,
secure: true,
httpOnly: true,
sameSite: 'strict' // or 'lax' if you know what you're doing
With these defaults, people will realise that it likely won't work by default on their localhost, forcing them to figure out why, and forcing them down a path which educates them about these settings, what they do, and what they are for. if people choose to set them to false, and not put them behind an environment detection, they have explicitly opted in to making that choice, rather than the package making the choice for them.
The second biggest issue with csurf, and no fault of yours, is that it also makes zero distinction in it's documentation on how to utilise csurf via double submit vs synchronised token pattern, and people often get confused between them as well.
Also, I just had a look at "Dependents" page of your csrf-csrf repo, and it really makes me curious, how do you arrive to ~50k NPM downloads / week with a package that has 35 stars, ~64 dependent repos / 3 dependent packages found by GitHub?
Say, here I have my babel-plugin-react-css-modules: 29 stars, ~94 dependent repos / 16 dependent packages ⇒ ~3.5k NPM downloads / week.
I'm just going by the stats npm themselves show - I know it's not entirely accurate / representative.
Edit: I do apologise for the initial impression I gave you. I came in a tad hostile, because I'm used to people who don't know what they're doing being the ones that make these forks.
you don't leave your safe unlocked just because your doors and windows are locked...
Well, a security is as secure as is its weakest link; and I believe, CSRF is not a "safe", more like an extra lock on your entrance door, and it makes little difference to put the most expensive fancy lock there, if anybody can easily break your window to enter.
How are these default settings secure by default, exactly?
Well... the default assumption in my head is that a website is served via HTTPS, which makes MITM unlikely, and these cookie settings are irrelevant. That unless you run it on localhost for dev purposes, where you use HTTP, and the next thing you say just causes unnecessary troubles for a dev:
With these defaults, people will realise that it likely won't work by default on their localhost
I guess, then one may think at which point HTTPS is enabled... I imagine in the majority of scenarios these days, HTTPS is enabled by the cloud's forefront, which then passes the traffic to the app itself (i.e. locally on the machine, inside the app container) via HTTP; so the app itself does not have to care about environment detection, HTTPS redirection, and other stuff (and among other stuff it does not want to use those most secure cookie settings, as they won't work). At this point one can do better in terms of security, but I'd say 99% of developers never encounter a project for which it will matter, and if they happen to work on such a project, they are probably experienced enough to understand all nuances themselves; without any particular package's README trying to additionally educate them.
These told, probably the README can be enhanced with some additional warnings, and stuff... not sure I have much desire to spend my time on it.
Though, yeah, I'll probably add a note regarding this stuff to README within a few days.