Content-Security-Policy should use default-src 'none'.

[ronperris]

What I expected

The Content Security Policy would deny loading resources by default.

What I experienced

The Content-Security-Policy uses 'self', which is more permissive than 'none'.

Why does this matter?

The Content-Security-Policy currently sets the following directives to permit loading from self.

child-src
connect-src
font-src
frame-src
img-src
manifest-src
media-src
object-src
prefetch-src
script-src
style-src
worker-src

There is no reason to allow all this functionality since the error page doesn't use any of these browser features. The current input validation and escaping code prevent tampering with the page contents, but if a bypass was found the Content Security Policy is needlessly permissive, IMHO. Set the default-src directive to 'none' and the page will continue to function and another layer of defense will be enabled.

[dougwilson]

Thanks! I set the value to this just because it is what a security researcher suggested. If there is a more restrictive and it works with all the possible outputs, feel free to make a pull request :)

[dougwilson]

Clicked on the wrong button to leave a comment, I apologize.

[ronperris]

@dougwilson Will this PR work for you?

[dougwilson]

Hi @ronperris sorry I've just been away traveling to a hackathon to help students learn open source :) ! So haven't had the opportunity to take a look just yet.

[ronperris]

@dougwilson Would you be open to releasing this soon? I ran into this issue because I was using another library and this functionality was downgrading my defined CSP policy.

[dougwilson]

Yes, I can. Typically these are coordinated with express releases but it's not required. The other lib that is using this is not express, is it?

[ronperris]

@dougwilson I was using https://github.com/helmetjs/helmet to set policy and they were being downgraded in finalhandler.

[dougwilson]

I see, though helmet doesn't use the finalhandler module. Just curious on where in your setup this module is getting invoked.

[ronperris]

@dougwilson Sorry, it is express. I was using helmet with express.

[dougwilson]

Ok. So even if a new version of this module is released, you still won't see the behavior change until a new version of express is release. Since this is a new feature, it would be bundled with the in progress express 4.17 release, which is still in progress, though it's being closed in on.

That being said, ideally your express app would never invoke this module by having a custom-defined 404 and error handler to generate the output for those as you would like instead of relying on the default catch-all (this module).

So it sounds like the release of this module will stay on its current course: it will get a new release bundled with express, as that is what you'd need anyway.

[bespokebob]

This has just been published as an npm advisory: https://www.npmjs.com/advisories/836
So, any upstream users of this module (which includes any users of express, react-scripts) are going to start seeing vulnerability reports when running npm/yarn audit. Please publish a new version of this module with the fix, so users can upgrade to a fixed version (even if that involves forcing the resolution in yarn until express also updates).

[dougwilson]

This is not a vulnerability? How can the module author not be involved in any of the publishing of an advisory? I wasn't even aware this would be an advisory at all...

[bespokebob]

Sorry, I wasn't involved in publishing this, I just ran into this when running one of my own builds and thought I should mention something here.

[dougwilson]

Like I still need a few more days to wrap up the Express 4.17 release. Perhaps if there was some sort of coordination? @ronperris ?

[dougwilson]

It's no problem @youngbob . I would have absolutely had a new version published before an advisory was published had I known an advisory was even going to be published (and ideally on what date). But I had no idea and now it's published.

I'm also unclear on what, specifically, the vulnerability is? Like there is no method to get arbitrary HTML onto the responses in this module, so it's not like there is a method to do anything?

Like is the issue that there isn't the most strict Content-Security-Policy header on responses? If so, then it would seem like many, many packages across npm would have this issue, as most never even set this header, and this module doesn't set the header on responses it doesn't produce.

Or is the issue that this module is producing a Content-Security-Policy and it's not the most strict it could be? If so, then I would assume that the old versions of this module that never set this header would not be marked as vulunerable, but yet the linked advisory has them marked as vulunerable.

Can you help us @ronperris ? I se you're also listed in the HackerOne report, perhaps you can provide clarity here.

[dougwilson]

Maybe @lirantal or @vdeturckheim would be able to help here as well, as they are usually active in the Node.js security stuff.

A sudden drop on a Friday evening (my time, but still a Friday across the world) makes it that much harder to get addressed in a timely manor, even ignoring the lack of prior notification to the module author (myself) that it was going to be appearing.

[bespokebob]

For what it's worth, I'm 100% on your side here. I suggest emailing security@npmjs.com about how this was handled. Considering the number of upstream users that are going to be seeing this, maybe it will get someone's attention.

[dougwilson]

Thanks @youngbob I just wrote up an email and sent it off.

[73rhodes]

Suggest re-opening this issue pending resolution to https://nodesecurity.io/advisories/836
(more specifically, until npm audit passes on this package).

[dougwilson]

As an update I have not gotten a response from npm security so far, besides an automated email confirmed my email was received. It's possible that the email is only active during business hours, so it may not be until Monday at the earliest for a resolution to begin.

[dougwilson]

Thanks @evilpacket

[dougwilson]

Thanks @isaacs that seems like the statement being made here as well. This seems like the most correct solution here is to stop setting the Content-Security-Policy header, like the versions prior to 1.0.0 did. Is there agreement on this being the correct solution?

[ronperris]

@evilpacket

we published a vulnerability in this module because of this HackerOne issue that was made public. The issue shouldn't be disclosed on HackerOne if it's not intended to be imo.

We have found that there is significant lag in some cases between when issues are made public on HackerOne and then are imported into the security-wg db so we monitor both.

It makes perfect sense to monitor alternative channels for information, but taking that information and upgrading the risk level and affected versions seems to be the issue here.

[evilpacket]

@ronperris yes I agree. I recognized this after I wrote that (see the DM I just sent to you). I think that it was likely that an analyst missed that it was set from Low to None for severity.

[ronperris]

@dougwilson

Notice that sites like Google and GitHub have CSP but no known vulnerabilities.

[ronperris]

@evilpacket Thanks for your help on this one.

[dougwilson]

So what is the path for https://www.npmjs.com/advisories/836 ? Does everyone agree or disagree on the proposed solution in #26 (comment) ? I would like to get a statement out for all the affected Express users who have been asking about this issue, and there are similar issues like this in some of my other modules, so I'm trying to get them solved, but refusing to talk about the issue outside of this module @ronperris , rather than the root issue, is not helpful, in fact, it feels outwardly hostile, to be honest.

[ronperris]

@dougwilson

You can call me hostile, ad hominem, but it seems that you've agreed with the refined points I've made about this particular case. I was just trying to discuss this particular case and be helpful and accurate.

[evilpacket]

My thought is it looks like if was disclosed on HackerOne with a none severity.

I would argue you can't have a vulnerability with a none severity so if it is an issue it's Low and then we would pin the advisory to specific version. We do have the ability to issue Informational advisories but we have no yet to date, those are more for footguns, "if you hold it this way it can make this vulnerable" but again haven't used it yet.

imo if you can unset the header as opposed to having a weak header then we can pin an advisory and give people an upgrade path.

[ronperris]

@dougwilson I'm a huge fan of yours and Express.

If that helps to better understand my perspective, I am trying to be helpful.

[dougwilson]

It feels hostile as in the following is how I have experienced events:

1. I get a report from you (this issue) in the public, just as a general question.
2. You offer a PR to make the value stricter.
3. You ask when it'll be released and I gave a general answer and no further clarification was asked for.
4. No indication that this was a vulnerability of any kind.
5. I have been working towards the agreed upon release in good faith
6. Suddenly there is a vulnerability disclosed on this, showing up in npm audit, which is a default feature with no noticed what so ever.
7. I try to get in contact as soon as I can, but nothing.
8. In floods issues, emails, DMs from understandable frustrated end users.
9. No responses...

I don't think you understand that when this happens, I literally get hate emails to my email address. I removed my email address from GitHub, but they still get it. Here is a literal quoted email:

Fucking release an express that does not have a security vulnerability out of the box. I can no longer check in code to my company CI because audit blocks it. Get off your ass.

[ronperris]

Suddenly there is a vulnerability disclosed on this, showing up in npm audit, which is a default feature with no noticed what so ever.

I didn't make this happen. I didn't publish a vulnerability in finalhandler the NPM team did.

[dougwilson]

I didn't make this happen. I didn't publish a vulnerability in finalhandler the NPM team did.

Right, I get that now, but how am I supposed to know that at the time? The npm site lists that you were the reporter on May 3rd, so it made it sound like you reported it to npm on May 3rd. What else was I supposed to interpret that as?

[evilpacket]

@dougwilson we monitor the hackerone feed from the security-wg, and issued an advisory but failed to see it as a none severity. Note this was human error was we do not automatically import from there into our db. I have now removed the advisory until we can decide what versions are impacted or if we even want the advisory to exist.

My plan here is to discuss on Monday our process for disclosure in npm audit to ensure that maintainers are aware regardless of what communications happens via the security-wg.

[evilpacket]

@dougwilson as an aside I'm very sorry for the hate that was directed towards you because of this. You do a lot of good work and maintain a lot of OSS that I've relied on and I'm grateful for this.

[dougwilson]

Yes, @ronperris I give my apology to you. Since the bombardment has started, I admit that it has affected me, and I may not be of the right mind...

Really, I just want to get this over with and get a fix out that is acceptable to resolve the situation.

To summarize, it sounds like the right solution here is just to remove the Content-Security-Policy header from this module, and the other modules in Express currently also only setting it to 'self' and not to 'none' such that it removes the false sense of security the header existing is providing. This way I can get a 4.17 express release cut such that nowhere in it is it providing the not-the-strictest-possible CSP headers. Is this correct?

[ronperris]

@dougwilson Thank you for all the work you do on these critical projects. I admire you and hope to make contributions that are more valued going forward.

For this case, I understand if you want to remove CSP from Express and its related modules. Let other modules dedicated to setting security headers handle it, like helmet.

[dougwilson]

Thanks @ronperris . The header (and even their specific value) were all added from another security researcher saying they should be there. I just hope that I am doing the right thing, as it just feels like I am at the mercy of the whims of whatever the different security researchers want, because if I do nothing, then the module ends up blacklisted, but if I do something, then maybe it now results in a new issue.

The landscape in this regard has changed so significantly from when I first started, that it has actually becomes the biggest friction point to maintaining modules in Node.js...

[ronperris]

@dougwilson Express is so massively successful that you will continue to get targeted by researchers and users reporting issue with security settings, I know you already know that.

I have a deep interest in Express and its security, if you ever need to off load any work I'm free to help.

[dougwilson]

Thank you. What I need help with is fi we can determine the final solution in regards to this issue and get some form of double jeopardy protection in regards to it. For example, if I remove the Content-Security-Policy header, is it possible such that there can no be another hearing on this issue in the future? As in the solution here is marked as the final ruling i.r.t. the CSP header on this module? I fear that removing the header could case the same or others to come claiming security vulnerability due to the lack of the header. The processes around the judgements i.r.t. ecosystem security issues feels like a black box.

For instance, I didn't know this issue had a backing HackerOne report and that it would be disclosed (which in turn caused confusion with npm security). Ideally Synk and others are not confused and I have to go through this entire dance all over again... but npm security points out having the open vulunerability listed on HackerOne is a point of confusion.

So like I guess what I'm asking here is together we should determine the following

1. What is the final judgement w.r.t. this module and the CSP header
2. Can this judgement be enforced by the node security-wg to prevent a double jeopardy like situation in the future?
3. What is going to happen with the npm advisory listing?
4. What is going to happen with the HackerOne report?

[dougwilson]

So now SourceClear has picked up this issue: https://www.sourceclear.com/vulnerability-database/security/insecure-content-security-policy-csp/javascript/sid-13708/ and everyone using SourceClear to check their modules are now showing as a Medium security issue.

[dougwilson]

I should just disappear from GitHub / Node.js at this point... I can't catch a break and even enjoy this weekend :(

[dougwilson]

Ok, so since there hasn't been any response to my questions above, and it does sound like it won't be until Monday for a decision to be made, I am going to turn off my email and GitHub until Monday at minimum so I do not have to be harassed with users who cannot get express installed due to this. Please do not email me, as I have a filter to trash everything for now and I'm not going to dig through the trash even after I remove it because I just don't want to read the harassing emails.

For those who are able to follow though the linked issues and come upon this, I have locked this issue to make sure that while I'm going to be offline this notice remains at the bottom and can be found and read:

There is an ongoing discussion on the issue here for finalhandler. I am sorry that you cannot get express, connect, karama, brave, or whatever else includes express/connect installed due to this issue. I had no idea this was classified as a security vulnerability before it was published and I was blind-sided. There is still not a specific agreement on what the fix should be, which is why there is no new version yet.

[dougwilson]

if you ever need to off load any work I'm free to help.

@ronperris if you really stand by this offer, please help get the https://www.sourceclear.com/vulnerability-database/security/insecure-content-security-policy-csp/javascript/sid-13708/ removed, as they told me they will not as long as the HackerOne report is there as reports are vulnerabilities. They make it sound like it's your report that they made the issue and will not take it down unless maybe you contact them as you are the security researcher they are basing it off.

[dougwilson]

I have requested the Security-WG to take down the HackerOne report and reto on why the HackerOne process was not followed: nodejs/security-wg#527

[dougwilson]

Due to the conversancy around this issue, the merged PR will be backed out and a release made without it, keeping the behavior intact. An internal issue within the Express TC will be opened to determine the method in which this should be resolved: lock the header down or remove the header. There was no clear answer provided by the security researcher in this regard and I don't think there will be, so the Express TC as a whole will make the call. In the thread above, the question presented was the following:

To summarize, it sounds like the right solution here is just to remove the Content-Security-Policy header from this module, and the other modules in Express currently also only setting it to 'self' and not to 'none' such that it removes the false sense of security the header existing is providing. This way I can get a 4.17 express release cut such that nowhere in it is it providing the not-the-strictest-possible CSP headers. Is this correct?

The response was not a yes/no, instead it was the following:

For this case, I understand if you want to remove CSP from Express and its related modules. Let other modules dedicated to setting security headers handle it, like helmet.

This makes it sound like a "yes" to remove the header completely, in order to "[...] Let other modules dedicated to setting security headers handle it, like helmet."

[jasnell]

Fwiw, @dougwilson, I think removing the header is the best course of action at this point

[dougwilson]

Though the npm security advisory has been removed (perminently or temporarily, I'm not sure of yet), the SourceClear listing will not be removed. I got an email response that they disagree with the HackerOne report severity level and their report will stand. I'm sorry about this situation.

There is active discussion for the resolution here in the Express security repository. There are multiple people from multiple time zones, so it will take at least a bit more to reach a consensus.