Cross-site Request Forgery (CSRF) found in csurf package

[IdanAdar]

Do we have alternative packages to csurf? it seems unmaintained, and recently a vulnerability was discovered.
https://snyk.io/vuln/SNYK-JS-CSURF-3021144

Given the popularity of this package, the impact radius is large...

[dougwilson]

Hi @IdanAdar sorry about that. The tracker was flooded and I had to limit it while I got some time to add a message. You'll need to choose another module, unfortunately.

[IdanAdar]

@dougwilson, is this the official decision of the ExpressJS org which houses this package?
This package is popular and I am not aware of alternatives, so please re-open this discussion. It cannot be one-sided decision.

[dougwilson]

Hi @IdanAdar yes, it is. Do you have any suggestions for fixing the underlying issue? The module gets almost a monthly report about the fact that you can do exactly what the OAWASP csrf articles says is possible. If you would like to take over the module, the license allows that and we can direct everyone to the version you have 👍

[IdanAdar]

What alternative module are you suggesting the world to use, please?

[dougwilson]

One which is not "vulnerable". I have not evaluated others at this time so am not looking to make a specific suggestion and opening myself to liability. If you have a suggestion that would be awesome and I can list it 👍 You may also try contacting that security researcher and see if they have a suggested module; you're also welcome to fork the module and fix it such that it is secure and I'd be happy to point to your module. I'm not sure what else you're looking for. Any help regarding that answer is appreciated, as I don't have an answer for ya.

Edit: you can also try reaching out to Snyk for a suggestion, as they are the ones who first blacklisted the module, and so I am just putting notice up on the repo and stuff now that it's deprecated, as that is the word from Snyk.

[dougwilson]

FWIW I just sent Snyk a request for what specific code changes would solve the issue at hand and remove the blacklist.

[dougwilson]

Hi @IdanAdar I just wanted to follow up here since you gave the above a thumbs up that I have just got a response back that the request is being escalated, but nothing further as of yet.

[dougwilson]

The only response I have gotten was simply to point to their own Synk page and it took a bit to even have them realize they were talking to the package maintainer, and then still no code guidance. I'm going to close this as it does not seem like they are willing to help and I don't have the energy to fight them about it.

[dougwilson]

And to help make it clear the frustration here: it states that the package does not value the XSRF-Token cookie, but if you have used the package you will find that it sets no such cookie name, so why would it even look for that cookie name? It sets a cookie named _csrf, which is also listed in the blurb and it does indeed validate that cookie value against the token submitted from the front end, which is does though a query param, body param, or custom header. That makes up the two pieces of the double-submit pattern. There is no third piece (XSRF-Token cookie) involved. I have not received any PoC app I can run that actually demonstrates an issue in the package, so I have no way to actually figure anything out, either.

After the Snyk blacklist of the package, I have archived and deprecated out of frustration as it will just be endless issues opened in the package from users who don't know what to do, and I have no PoC to work against to solve anything. The security side of these reports is very one sided and places like Snyk wield large power to effectively blacklist a module and never even contact the author about anything. Then when one tries to contact them, you just get some low level support who doesn't understand the technical details and no help. And since Snyk lists the version range as * even releasing a new version won't make the vulun go away; it will turn in to another round of begging and pleading with their support staff; I had to do this wither another module and it was just hair-pulling experience I'm not going to bother with any longer.

[dougwilson]

I will add that I still have not been able to reach beyond the front line support in Snyk which has not been helpful in any way, just proving the link above. Also if this package were really that popular, one would think that lots of folks would be reaching out/offering to help/anything. This is the only one in over a week so far, so either it's just not very popular or no one actually cares to help, which is a demonstration for why it was simply deprecated after Snyk blacklisted the module.

[IdanAdar]

I’m trying some internal contacts to get proper response from Snyk. I agree it’s irresponsible from them.

[dougwilson]

Thanks, it is appreciated. For more context (sorry this all came down fast and is just very upsetting -- especially Snyk blacklisting the module without even reaching out before hand), the security article they reference I did have correspondence with, but I asked over and over again for a PoC to demonstrate the issue and they refused, saying it was the client's code. I gave them the example from the README and I asked for them to alter it (as necessary) and provide the steps to reproduce the issue and they told me they were just a product manager and didn't know how to run the Node.js code. I helped get it running, but still no instructions. I then made multiple changes and provided the person them asking if they addressed the issues or not, but no. I was left just I guess trying to guess what I was supposed to do and after that going on for 2 months, I threw up my hands and said if he thinks it's vulnerable, then I guess it is and left it as that, as I have no idea what exactly I'm supposed to be changing to fix whatever. I'm not CSRF expect and didn't even write the module. I took it over as that author left the project, but I don't have time to play 1 million questions with some security researchers who won't even provide a PoC or a patch. Once Snyk blacklisted it, my experience interactive with Snyk tells me just to abandon the module at that point. I didn't even deprecate it until you filed this issue bringing up that Snyk blacklisted it.

[IdanAdar]

@dougwilson See here: https://snyk.io/blog/explaining-the-csurf-vulnerability-csrf-attacks-on-all-versions/

[dougwilson]

Thank you, I already saw that. It unfortunately doesn't provide any PoC that demos the issue(s) and ultimately if someone else can decipher the fixes from that, they are 100% welcome to fork the package, make the fixes, and I can update the readme to point there. Also if that was there to explain it, why did they not send that to me before blacklisting it? They have still never reached out, and I never deprecated the module until after they blacklisted it. I'm over Snyk at this point. I still have not gotten their support to help. Theyclearly do not care about fixing anything and just want to write blogs. I mean, they end with "just use this ither framework instead of Express".

[jntnmoses]

@dougwilson Hi there! I'm from the Security Operations Room at Snyk, who published the advisory after we'd been alerted to the vulnerability from this blog post. We understood the description there, including the timeline indicating responsible disclosure with maintainer participation, to mean that the exploit vector described was duly confirmed and public knowledge. I apologize for our not communicating with you for direct confirmation, which is our standard practice when we are the discoverers or initial reporters of a vulnerability. Having been pointed to the current thread by a user, we'd be happy to discuss the issue itself, clarify its scope, exploitability, and affected versions, and share an in-house PoC (currently being built) with you - here or via [email protected]. We by no means intended to blacklist the package and very much appreciate the work put in by voluntary maintainers like yourself to keep open source going!

[dougwilson]

Hi @jntnmoses thank you for that. I have emailed there and been in contact with an agent for many weeks now and got no where. How will emailing again actually get the response you are mentioning? Is there a different email I can use to get to someone more direct? I don't have further time or care at this point to do anything further. It has been a month now, and the damage is done. I am no longer interested in futher discussion on this.

[benjifin]

@dougwilson [email protected] is a direct link to the security division at Snyk - I think that you might have run into some technical issues interfacing with [email protected] which is our support team. But as @jntnmoses mentioned above we're very happy to discuss the issue further either here or in a direct and private comms - in general this would be the way we'd like to communicate with maintainers on a vulnerability disclosure, and it appears we were misled by what seemed to be a responsible disclosure of this vulnerability prior to our publication. Sorry that you have been frustrated up till now but if you were interested in discussing further and either limiting the existing impact of this vulnerability, or discussing ways to fix it - we would be more than happy to help out rather then you needing to abandon this project which we know provides value to the open source community 🙏

[IdanAdar]

@dougwilson Can we take another swing at this in the spirit of collaboration?

[benjifin]

To update - we have updated our advisory based on the feedback provided by @dougwilson so far to change the severity of this vulnerability from medium to low reflecting the numerous conditions required to exploit the vulnerability, limited it's impact to version 1.2.0 and greater, and added context to explain the necessary pre-conditions required to exploit.

We're happy to suggest from experience various ways to solve the issue, but I would also think it a reasonable stance to document a low-severity issue as a non-fix, and continue development of the library with that documentation noting users of the theoretical low risk involved in using it.

[SorinGFS]

@benjifin

numerous conditions required to exploit the vulnerability

No no no no no, I'm not happy with that! You didn't provide a single proof in which csurf library is "vulnerable". Actually your "experts" didn't even know the difference between xss and csrf attacks. Take that so called "vulnerable" scenario and provide us a single successful attack in which the csrf token cookie is sent along with the malicious code and then we can talk! Until then I will consider your company as an unprofessional one that makes totally unfounded allegations.

If you fail to provide that evidence, I expect the deletion of that article and apologies.

NB: for a moment an idea flashed through my mind is that you could think that some bank would allow transactions on a GET request in 2022... If so, your advices must go to them, not to this library!

[benjifin]

I think there is some confusion here. We are not the authors of the original article, we saw the article after it had already been published and seemingly acknowledge and responsibly disclosed - and published a corresponding advisory based on that. I think your criticism here would be better placed with the authors and vendors behind that article, rather then with us. My team and I were first made aware of the issues behind this article a week or so ago after we were made aware of this thread, from which point we've been trying to take on board the feedback @dougwilson has been providing and attempting to adapt to it. We're happy to continue to engage and adapt as well - but please try and trust that there are people acting in good faith here, and engage with us accordingly - I promise that we will be very happy to revoke the advisory in it's entirety if that's indeed the best case. Note: edited after confusion on my part 😅

[SorinGFS]

@benjifin

I'm just an csurf library user very happy with it. It is a library which does what is sais, cristal clear documentation in which please check this out: ignored methods.

ignoreMethods
An array of the methods for which CSRF token checking will disabled. Defaults to ['GET', 'HEAD', 'OPTIONS'].

As a csurf user I am informed that GET HEAD and OPTIONS http methods are not subject of csrf protection, meaning that I would never allow sensitive transaction using those methods. Of course, there are ways to hack my veins, but by default this library is safe.

[SorinGFS]

@benjifin

I promise that we will be very happy to revoke the advisory in it's entirety if that's indeed the best case.

Your advice lead to deprecation of this library. I am amazed by the fact that you made this decision based on an article without trying to produce a valid proof based strictly on the existing documentation. If you had done the same to a commercial company you probably would have lost many dollars in court.

Looking forward for your good faith (and I hope that you are aware that now 150k+ projects and probably millions of installations receive in the console this deprecation status, which since we talk about security can only mean one thing: PANIC!)

[SorinGFS]

@benjifin

My previous comments were started having in mind an article written by snyk company (is now erased which is a good start), later I read all the comments on this page and I realized that in fact everything is based on another article written on fortbridge.co.uk. My apologies for not giving the link, this way the confusion would be avoided. The fortbridge article doesn't also provide any proof for their allegations simply because they doesn't mention anything about the implementation. They simply start to disect the raw code without any context, which is the first proof of lack of professionalism. I will disect their article later but first let's agree a few things:

CSURF library (as documentation states in its first row) is a Node.js CSRF protection middleware, and depending on requirements CSURF may work in conjunction with another middlewares like cookie-parser, body-parser, express-session, cookie-session and so on...

The S from SOLID principles stands for singularity, or Single-responsibility Principle and according this principle CSURF library does not solve xss, cookie tossing, authentication or other kind of concerns but csrf (cross-site request forgery).

CSURF library is highly configurable and offers to the user the posibility to combine the settings according to its needs. Despite the fact that CSURF default settings are safe an inapropriate use of this library can lead to insecurity. In comparison, soap is a good thing, but if you eat it you will die! Would this make the soap a dangerous product? Absolutely not! We just have to teach the kids not to eat it! In CSURF's case the documentation does exactly that! Of course, it doesn't specify that this library is for 12+ audience and I guess the banks wouldn't hire kids to build their security system... Ultimately, the responsability goes to the user which should make own penetration tests for the whole setup.

A quick recap:

• incorrect implementations does not make this library vulnerable
• inapropriate expectations (xss, cookie tossing, authentication ...) does not make this library vulnerable
• defaults are safe
• configuration examples from documentation are also safe (eg. app.use(csrf({ cookie: true }))))
• there is no need for stricter defaults since this library is complying the standards (eg. CSURF does not enforce cookie: {sameSite: 'strict'} because the IETF standard is lax, and lax by design have a robust defense against all cross-site request forgery (CSRF) attacks made through http methods other than GET, HEAD, OPTIONS and CSURF library explicitly specifies in the documentation the default ignoreMethods: GET HEAD and OPTIONS warning in this way the user not to allow sensitive transactions using these methods)
• in years of use and millions of installations there are no reported csrf protection issues regarding malfunctions of CSURF documented behaviour (and this is the point from where your investigation should start, not some blog article)

If we agree about the above points now we can start another analysis to see if a correct implementation with apropriate expectations can be found "vulnerable". Now let's disect the fortbridge article. I already mentioned first flaw, they don't give us details about the code used in implementation. Is like a user is reporting a bug without giving any details about his implementation, usually the mentainers doesn't even read the details of such reports. Their lack of professionalism goes further when they try to prove the vulnerability of CSURF by using an external tool (BurpSuite) instead of browser. As OWASP Cross-Site Request Forgery Prevention Cheat Sheet states right in the introduction Cross-Site Request Forgery (CSRF) is a type of attack that occurs when a malicious web site, email, blog, instant message, or program causes a user's web browser to perform an unwanted action on a trusted site when the user is authenticated.

Basically, what they say is that knowing the token generarion mechanism combined with xss vulnerability on a subdomain an attacker would be able to bypass the csrf protection. First of all, as OWASP states in the document above any Cross-Site Scripting (XSS) can be used to defeat all CSRF mitigation techniques! So, in that particular case we don't talk about csrf vulnerability anymore, it's xss vulnerability and in this case the csrf would be our last problem!
Then, fortbridge article author ignores all the OWASP recommendations for csrf prevention on the basis of which the CSURF library was written. As you can see in the pictures bellow, in order to have a functional csrf protection the token generated by the CSURF library must work in conjunction with something from the server side: a session keeper, or a session identifier used by another security layer. So, calculating a potentially valid secret - token pair is useless because the result won't be validated server side!

OWASP general recommendations

OWASP Double submit cookie recommendations

Conclusions from the fortbridge article

Missleading picture from the fortbridge article

So, what I expect from snyk advisors team is:

• respect the CSURF documentation to the letter
• honor the OWASP recommendations regarding csrf prevention
• build a verifiable implementation of this library and post it somewhere so others can test it
• try to prove a flaw in the above implementation, and if you can't please remove your report asap (any delay just extends the already produced damages)

[dougwilson]

Thank you @SorinGFS for your through post above. I'll add to that that (a) I asked fortbridge over and over again to provide a PoC and one was never provide -- they actually ask me to provide a PoC, like, what? (b) they asserted over and over again that this magic xsrf-token cookie was not validated -- but i tried to explain that the double-submit is one piece of data in a cookie (the _csrf cookie in this instance) and one piece in the request itself (query string, body, header -- it doesn't matter just somewhere that is not automatic in the web browser that a foreign origin doesn't have access to like the cookie header is) but it seemed to fall of deaf ears just repeating that for some reason the module needs to also validate a magic xsrf-token cookie in addition to those two pieces of data (??) and (c) they were dismissive in every way and only repeated in asking how to get credit for discovering a vulnerability and that they were going to publish a blog post.

as you have noted that at best it relies on xss (which breaks csrf protections as noted in owasp) or cookie tossing (which just set the cookie to __Host-_csurf maybe?) which are separate in a way from csrf itself, but are ways to break it and need to be mitigated. session fixation would i'm sure be yet another method. but the biggest issue is multifold here that (a) anyone can publish a blog post online and there is nothing i can do about it and (b) security companies looking to have many vulnerabilities under their database will happily pick up these without any verification (either internally or requesting comment from the package when there is nothing written about it on the package's own docs) and of course (c) further sensational articles spin off from it from security blogs and such.

i deprecated it because of these social reasons, not because i believe there is any true vulnerabilities in it. csurf being so popular attracts people just looking to get their name out there on vulnerability discoveries and as it seems you are aware csrf in particular is very nuanced and reports i receive have all been unclear without any evidence in showing an attach working (like this one) or have been a form of xss or similar. getting 1-2 reports like this every single month for the past 14 months now is just exhausting and i would say 90% of them start off with a closed mind and are not interested in hearing me say what they report is not a vulnerability. and there is a lot of external social pressure when before i even know, multiple blogs are out on security sites about an issue (like this one) and users flood in and are not very interested in hearing me explain there is no vulnerability, as the security scanners and sites say there is -- how i am a more reliable source, as i probably have an interest in saying nothing is wrong /shrug

anyway, there you go, i have laid out a lot there and folks can take it as they like. it is unclear why the snyk blog post has been removed but i guess that is progress ? ultimately bringing the module back to life now is only going to result in more questions by all these folks who read about how it is broken. if it were to be reinstated, it would ideally need to have some kind of link to snyk about it so folks would know what to do now, which i think is also what you suggested @SorinGFS

[SorinGFS]

@dougwilson

getting 1-2 reports like this every single month for the past 14 months now is just exhausting

I can imagine the pressure, but I really think you should ignore any report that doesn't come with a proof that respects the procedure I mentioned above. Even those in the present case, if they had followed these procedures, they would not have put themselves in the ridiculous situation of admitting that they were wrong. And the cases of real vulnerabilities, if there were such things, I think we would be happy to know and fix them. But luckily they don't exist. When I decided to use this library I studied tons of materials, lots of libraries and I made every test possible to ensure that I made a good choice for my project. Here is how I implemented the csrf protection. Cyber security is not child's play, that's why it's hard for me to understand how these companies hired people without the right qualifications. The only explanation can be that the motivation behind it is different than finding real vulnerabilities.
That's why I think you shouldn't be influenced by the large number of people manipulated into asking questions, but you should prepare a standard answer for them and post a link to that answer in the documentation. And of course, in the absence of concrete proof, you should put the library back online. Ask those manipulated to present proof of the library's vulnerability themselves if they are asking based on reports posted somewhere else.
And finally, if you still feel obliged to save the planet, I'll give you some news: you can't! Be grateful with the hundreds of thousands of users who appreciate this library! 😄

[SorinGFS]

@dougwilson

I started an investigation of so called fortbridge company to ask them for explanations for what they do... Well, even a ghost is more visible than they are, see for yourself:

[dougwilson]

I'm not sure what that really means, but I'm not sure it's worth the effort. The Snyk advisory I see still stands and ultimately I deprecated the module due to it being noted as vulnerable. Again, that decision is not due to only that, simply the final cause. I took over maintaining the module after an absence of the original author, but I am not an expert in CSRF, and a module like that probably needs to be run by such an expert. I have mostly left the module working exactlt as I received it as I don't want to make unnecessary changes since I don't know all the particulars of the subject matter of CSRF. Really, I think ultimately the best thing for everyone is if someone very knowledgeable in CSRF wants to do the goodwill of publishing an Express.js CSRF middleware module to npm.

[SorinGFS]

@dougwilson

I'm not sure what that really means, but I'm not sure it's worth the effort. The Snyk advisory I see still stands and ultimately I deprecated the module due to it being noted as vulnerable.

What this really means is this: that fortbridge company is a ghost and cannot be held accountable for what they do on that tiny website. This is how individuals write defamatory articles for money and want to avoid being held accountable for what they do.
On the other side, those from snyk who issued that report based on that article can be held accountable because they are a big company. I'm running out of patience with them too. "Good faith" my ass! Is been 8 days since that colleague of @benjifin said that they are working for that PoC, nothing happens! If they really had good faith they should remove that report in the first place, and publish another one just after they would be able to hand over that PoC! I will wait until the end of this week, and if there is still no change, I will contact their company to see if they agree with such behavior on the part of their employees. If I won't be successful even with this, I can go even further.

I think ultimately the best thing for everyone is if someone very knowledgeable in CSRF wants to do the goodwill of publishing an Express.js CSRF middleware module to npm.

Is not that easy you know, to replace a basic library like this one in hundreds of thousands of projects and millions of installations.... It wouldn't be easy even if the later library would contain exact same code and just to replace the reference to it... It would take years until every single dependent project would be able to replace it. And all of this for what? For the lack of knowledge or good faith of a bunch of guys calling themselves "security advisors"? Come on! If it were that easy, I would be the first to help you, it would be an honor for me. But it's not easy! You have a reputation to defend! What will you do if in the future these attacks would also appear at Express? Will you also give up Express too? I know that you know the guys from Koa team. What they say about this matter?

[jntnmoses]

@dougwilson We've decided on the basis of our research that this isn't exploitable without having achieved an additional exploit, nor on domains that have no subdomains that can be leveraged, nor on implementations using stateful session middleware such as express-session or cookie-session. Rather, it is a consequence of the double submit pattern in general, and shouldn't be considered a vulnerability. We will therefore be removing the advisory we'd published, and advising the adoption of secure anti-CSRF practices like those below.

We very much appreciate your open source work and that of the other maintainers in the open source world, and apologize for the trouble this caused for you personally. We would be happy to work with you or other developers on the implementation of the hardening suggestions below, via [email protected].

Recommendations:

1. In addition to the specific conditions for the vulnerability to be exploitable, likelihood of exploitation is decreased due to:

• SameSite=Lax being the default cookie value in modern browsers.
• If CSP is configured on the subdomain, its policy may prevent the ability to 'toss' the _csrf cookie via XSS.

2. One key piece in the putative attack as originally stated was the wide locations in which the defaultValue (req) function searches for the Xsrf-Token - specifically, allowing the token to be set within GET and POST parameters which are attacker controllable. Allowing the token to be submitted via request parameters is historically required with traditional forms, but for more modern AJAX based applications this can safely be set in the header. This means in certain situations where AJAX is used the attack can be mitigated by restricting the search location to req.headers. This mitigation does not work for traditional forms, but the library could introduce a configuration option which would restrict tokens to header validation only, allowing a mitigation to be applied to any application making use of XHR / AJAX.

3. The library provides a mechanism to sign cookies using the signed configuration value. While setting this value to true would stop arbitrary values being used, due to the stateless nature of the Double Submit pattern, an attacker could obtain their own signed value and toss this cookie. Instead, signed cookies could be scoped to the session (such as including a user identifier in the cookie, which is later validated against the currently logged in user).

4. The _Host- cookie prefix can be used to prevent any compromised subdomain from setting cookies on its parent. This approach is simple to implement and would only require a minor change to the csrf package cookie configuration. Any cookie tossing attempts would generate cookies which are invalid. The main caveats here are that the cookie must have the Secure attribute and the cookie path must be set to /.

[SorinGFS]

@jntnmoses
Thank you.

@dougwilson
Please put the library online.

[SorinGFS]

@dougwilson

The 7th row of documentation is like this:

If you have questions on how this module is implemented, please read Understanding CSRF.

I remembered that more than 2 years ago I wrote this issue on Understanding CSRF Rename the library: How-to-get-total-lost-regarding-CSRF! . Despite the fact that nobody understood me I couldn't be more right! I know that you are pillarjs contributor and probably you at least agreed to the conclusion of that document:

As the web moves towards JSON APIs and browsers become more secure with more security policies, CSRF is becoming less of a concern. Block older browsers from accessing your site and change as many of your APIs to be JSON APIs, and you basically no longer need CSRF tokens. But to be safe, you should still enable them whenever possible and especially when it's non-trivial to implement.

I will not comment on the obvious logical fractures in that document but this conclusion. First of all, http requests are not just json, they may be xml, file uploads, multipart forms, streams, and so on. CSRF protection covers all of them. SPA or MPA apps are not the miracle solution for everything, and certainly not for SEO, so the classical html websites won't go anywhere anytime soon! Likewise, the need for CSRF protection!

Therefore, I think you should replace the link to this missleading understanding CSRF document with the OWASP Cross-Site Request Forgery Prevention Cheat Sheet. In addition to that, I think we should write a nice short intro in the documentation pointing the following:

• this library has only one concern: generatates a secret-token pair which can be validated later by the server to ensure that subsequent requests are comming from the same client that generated the initial key pair. The validation proccess can vary depending on the needs and is not a part of this library.
• as OWASP Cross-Site Request Forgery Prevention Cheat Sheet states, any Cross-Site Scripting (XSS) can be used to defeat all CSRF mitigation techniques! and mitigations of xss, cookie tossing or any other kind of attacks are not concerns of this library!

I can try a PR if you don't have time.

[IdanAdar]

Thanks all! @dougwilson are you willing now to undo the deprecation, pretty please?

[morganney]

Wow. What an informative, frustrating and sad issue to read through. The spirit of OSS crushed by corporate America.