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

Update: Cross_Site_Scripting_Prevention_Cheat_Sheet #376

Closed
calabacito opened this issue Mar 20, 2020 · 19 comments
Closed

Update: Cross_Site_Scripting_Prevention_Cheat_Sheet #376

calabacito opened this issue Mar 20, 2020 · 19 comments
Labels
ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet.

Comments

@calabacito
Copy link

What is missing or needs to be updated?

We should update Bonus Rule #4: Use the X-XSS-Protection Response Header

How should this be resolved?

Browsers don't give proper support anymore:
Chrome has XSS Auditor Removed: https://www.chromestatus.com/feature/5021976655560704
Firefox have not, and will not implement X-XSS-Protection: https://bugzilla.mozilla.org/show_bug.cgi?id=528661
Edge have retired their XSS filter: https://blogs.windows.com/windowsexperience/2018/07/25/announcing-windows-10-insider-preview-build-17723-and-build-18204/

Some of the links were provided via: metabase/metabase#11444

Thanks,
Ariel Coronel

@calabacito calabacito added ACK_WAITING Issue waiting acknowledgement from core team before to start the work to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet. labels Mar 20, 2020
@jmanico
Copy link
Member

jmanico commented Mar 20, 2020 via email

@ThunderSon
Copy link
Contributor

I don't believe this should be disabled. If the browser doesn't support, that header won't hinder performance nor affect the user in any way, unless an attack vector exists, which is discussed in one of the points below.
Important points to note:

  • By default, if the web-server doesn't set it, it's X-XSS-Protection: 1 set on filter or block, based on the browser version.
  • Some websites shouldn't set it because of the xs-leaks attack vector.
  • If a website is still served on older websites, the Header could help protect the user, so what is the concern that we're tackling?
  • If there is no CSP implemented in the response, removing it isn't that logical.

More on the decisions taken here in the daily swigs.

@rbsec
Copy link
Contributor

rbsec commented Mar 21, 2020

As with many things, I'm not sure there's a simple recommendation to make here. My understanding is:

X-XSS-Protection: 0

  • More vulnerable to XSS in some browsers (IE, older Chrome, maybe others?)

X-XSS-Protection: 1

  • More vulnerable to XSS in some browsers (as XSS auditor can break protection)
  • More vulnerable to clickjacking (can break framebusters)

X-XSS-Protection: 1; mode=block

  • Some protection against XSS in some browsers
  • Potentially vulnerable to XS-Leaks (although depends on lack of CSRF protection/SameSite)

Having it in the default state (filtering) seems the most dangerous, as it introduces a vulnerability that you can't really do much about (the attacker being able to disable/break scripts in your page).

As long as you have other good protections in place (CSP, CSRF-style protection or SameSite=Strict to protect against XS-Leaks) then I'm not really sure it makes much difference whether you have it disabled on blocking (and it won't affect a lot of users anyway.

I don't have a particularly strong feeling either way, but if we're going to recommend disabling the header (which is the opposite of what almost everyone else recommends), then I think we need to have a strong (and documented) justification for doing so, and to expect arguments from people down the line about it.

@calabacito
Copy link
Author

calabacito commented Mar 21, 2020

You guys gave great examples on why it needs an update.
Also there is no mention on XS-Leaks for example.

@jmanico
Copy link
Member

jmanico commented Mar 21, 2020 via email

@jmanico
Copy link
Member

jmanico commented Mar 21, 2020 via email

@mackowski
Copy link
Collaborator

Yeah we should either recommend to disable or describe a full threat model

@psiinon
Copy link
Member

psiinon commented Mar 23, 2020

FYI we no longer report this as an issue in ZAP: zaproxy/zaproxy#5849

@ThunderSon
Copy link
Contributor

Awesome to hear that, Simon.
Netsparker's take
Acunetix's take
I contacted Scott (Owner of Security Headers) to look this through. Once agreed, we can update the sheet.

I'll be in contact with the Security Headers Project in OWASP, and Detectify. Any other major player that should be contacted?

@jmanico
Copy link
Member

jmanico commented Mar 23, 2020 via email

@calabacito
Copy link
Author

Awesome to hear that, Simon.
Netsparker's take
Acunetix's take
I contacted Scott (Owner of Security Headers) to look this through. Once agreed, we can update the sheet.

I'll be in contact with the Security Headers Project in OWASP, and Detectify. Any other major player that should be contacted?

Thanks, since we are working here for industry standards would be nice to keep track of this "things" and since ZAP is already part of OWASP, have some pipeline towards following or create an issue for this situations, right?

If anything I can help with, just mention.
Best regards,

Ariel.

@jmanico
Copy link
Member

jmanico commented Mar 23, 2020 via email

@jmanico
Copy link
Member

jmanico commented Mar 23, 2020 via email

@jmanico
Copy link
Member

jmanico commented Mar 23, 2020 via email

@calabacito
Copy link
Author

Thank you Jim !
Can you update the ticket later with the final changes around the different OWASP projects please?

@ScottHelme
Copy link

I agree that the only universally applicable advice is to disable the Filter/Auditor with a value of 0 in the header. There are some edge cases where you could argue against this but they are too complicated and burdensome. It's worth noting that some very large sites, like Facebook, have been disabling the Filter/Auditor for quite some time and at the time of writing are still doing so.

It's also worth nothing that Security Headers (securityheaders.com) deprecated this header in July 2019 at which point is was no longer required for the A+ grade (source) and we do not recommend that sites set it if it is missing.

As for Jim's comments:

Scott Helme's site does not evaluate header values, it just checks if they are present. The grades it gives are highly inaccurate.

We do evaluate header values Jim, you can see us give grade restrictions for severe problems in a CSP header or warnings for problems in Cookie headers, as just two examples. As for inaccuracy in grading, could you let me know what we're inaccurate compared to or why they are inaccurate? I'd be happy to improve something if there is an issue 👍

@jmanico
Copy link
Member

jmanico commented Mar 24, 2020 via email

@ThunderSon
Copy link
Contributor

Thank you @ScottHelme for your feedback, and apologies for any misunderstanding that occurred 😄
The document will be updated to remove the header and set a note to inform the users that this header has been deprecated. A small study on the sheet will be conducted in order to update it.

Thank you all for your help and support!

@ThunderSon ThunderSon added ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. and removed ACK_WAITING Issue waiting acknowledgement from core team before to start the work to fix it. labels Mar 24, 2020
@ScottHelme
Copy link

No problem at all Jim, we're constantly working on improving. Let me know if you ever see something we can work on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet.
Projects
None yet
Development

No branches or pull requests

7 participants