Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should x-xss-protection default to “0” instead of “1; mode=block” #439

Closed
oreoshake opened this issue May 17, 2020 · 12 comments · Fixed by #479
Closed

Should x-xss-protection default to “0” instead of “1; mode=block” #439

oreoshake opened this issue May 17, 2020 · 12 comments · Fixed by #479
Milestone

Comments

@oreoshake
Copy link
Contributor

Related https://github.com/helmetjs/x-xss-protection/issues/14

There’s some good discussion there. The owasp consensus is that it does more harm than good.

We’ve always allowed people to override this setting, but maybe we should change the default.

@EvanHahn
Copy link

I'm the maintainer of the linked module and have been thinking about this for awhile. My thoughts:

I see two reasonable options for this header's default value: 0 and 1; mode=block. Which should be the default?

  1. Setting the default to 1; mode=block protects everyone from trivial XSS but puts everyone at risk of side-channel attacks.
  2. Setting the default to 0 protects everyone from side-channel attacks, but puts some people at risk for trivial XSS.

No matter what we choose, the default will put some people at risk.

I'm coming around to the idea that the default should be 0 but there should also be a strong default Content Security Policy. And in any case, users should be able to change the default to whatever suits them.

What do you think?


As a meta point, I think the maintainers of Helmet, Secure Headers, and many other modules need to answer questions like this. Should we find a way to work together? I don't think we need to do anything fancy, but it could be good to get all of us on an email list or something. I think this isn't the place to discuss that, but want to float the idea.

@ThunderSon
Copy link

I'll reply in here, instead of the issue at Helmet's repository.
Adding on what you mentioned, it's pretty clear what the compromise is.
Coming from a security PoV, the risk that can mitigated is the one that will be accepted. Side-channel attacks are much harder to prevent, while XSS attacks can be blocked by another header for example, the CSP, and in other various ways (encoding, sanitization, etc.), not to mention the weak protection that is being provided.

If I were to recommend, I'd follow point number 2 and disable the XSS auditor.

As for CSP, setting a default might be tricky. I mean I like it, but not so advanced developers won't like it. It could be an opportunity to make CSP implemented and more wide-spread if libraries set secure defaults. How I am looking at it:

  1. If a developer doesn't want it, they can remove it. In all cases, they won't implement CSP, so why even bother? Let them remove it and live their happy vulnerable lives. I am certain lots of panic will occur for developers that barely understand the technology, and recommendations to remove it will be everywhere on StackOverflow, such as SSL validity checks. What I am trying to say is people will always run to the easy road just to fix an blocker. Doesn't mean it's the correct thing, so I wouldn't worry much if a note was set in the documentation about it.
  2. If a developer doesn't know about it, but wants to make their website work with it, they'll start implementing it step by step. (it gave awareness and motivation)
  3. If a developer is invested in security, they'll be happy to see that a library already sets secure defaults.

cc @lweichselbaum for additional input on CSP, and what they think about implementing a CSP secure default.

@oreoshake
Copy link
Contributor Author

@ThunderSon thanks for the followup in another thread! This library will set csp by default that should work for apps out of the box and is a nice mix of common needs for apps without sacrificing all the security. I'm not sure how many people actually use it, but I'm a strong advocate of providing a default and I've seen it work wonders. Of course, I have no idea how many people actually use the default or benefit from it but I'm not worried about negative effects here.

Content-Security-Policy: default-src 'self' https:; font-src 'self' https: data:; img-src 'self' https: data:; object-src 'none'; script-src https:; style-src 'self' https: 'unsafe-inline'

@oreoshake
Copy link
Contributor Author

That script-src https: is definitely problematic with script gadgets. I could definitely soften my stance there.

@ThunderSon
Copy link

@oreoshake how are you thinking of softening it up? What is an example that you're thinking of?

@oreoshake
Copy link
Contributor Author

@oreoshake how are you thinking of softening it up?

My stance on the default policy as-is (or at all) being a good idea. @EvanHahn does helmet set a default policy?

@EvanHahn
Copy link

The current version does not, but I intend to add one in the next major version of Helmet.

@oreoshake
Copy link
Contributor Author

I think I'm going to try and tackle this in the next release. It's a breaking change and #422 really needs to get out (and is also a breaking change).

@oreoshake
Copy link
Contributor Author

@ho4ho at this point, I don't think there's any disagreement, there just hasn't been a pull request for it.

@oreoshake
Copy link
Contributor Author

Does anyone know if there's a similar issue in rails/rails? Might as well change it there too.

@oreoshake
Copy link
Contributor Author

Circling back to this, GitHub has defaulted to 0. It's past time to do the same here.

@Malvoz
Copy link

Malvoz commented Jan 18, 2022

It should be noted that many modern browsers don't support this header:

The MDN docs also warn against the use of this header:

Warning: Even though this feature can protect users of older web browsers that don't yet support CSP, in some cases, XSS protection can create XSS vulnerabilities in otherwise safe websites. See the section below for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants