-
Notifications
You must be signed in to change notification settings - Fork 368
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
IE XSS filter can be used for XSS attacks instead of preventing it. #26
Comments
Thanks Arnout, I'll dig into it. appreciate the heads up. Arnout Kazemier wrote:
|
Setting the header With the header set to And with it set to My adventures in Firefox never ran the script. If you're interested, here's the PHP I used to test this: <?php
// Uncomment these to see the magic...
/* header('X-XSS-Protection: 0'); */
/* header('X-XSS-Protection: 1; mode=block'); */
?>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>test thing</title>
</head>
<body>
<p>Hello, <?php echo $_GET['name'] ?>!</p>
<p>The above code looks like this:</p>
<pre>Hello, <?php echo $_GET['name'] ?>!</pre>
</body>
</html> This might be another browser-sniffing adventure... |
Per my understanding [which may be incorrect, only skimmed Arnout's links] the trouble was with IE's page rewriting — the vulnerability was in "fixup" mode as it were. By sending |
FWIW, there's no mention of |
This isn't an easy thing to solve. If you include it, you make people on new browsers safer and people on old browsers vulnerable to complex attacks. If you don't include it, you make people on all browsers less safe. I think the algorithm is this: if ((browser != 'IE') || (version >= 9) || (options.setOnOldIE)) {
res.setHeader('X-XSS-Protection', '1; mode=block');
} else {
res.setHeader('X-XSS-Protection', '0');
} I think you should have to opt in to setting the header for IE 8 and below. I think the API should look like this: app.use(helmet.iexss());
app.use(helmet.iexss({ setOnOldIE: true })); And, of course, add a lot of information about this in the README. Does this sound good? |
I think this is perfectly acceptable, specifically the part about Evan Hahn wrote:
|
Working on it now! |
This addresses the problems discussed in issue #26. It also cleans up the readme and changes some of the language.
There used to a bug in the XSS filters in Internet Explorer which actually enabled XSS attacks instead of preventing it. Making sites which would normally be safe vulnerable for attacks.
So helmet could actually make sites more vulnerable instead of protecting them. The simplest solution would be disabling the filter for IE8 as this fix was most certainly landed in IE9 > as I doubt it can be detected by UA sniffing. If you feel it's not worth to fix this.. Please consider adding a note to the README file so developers know that they potentially expose them selfs to XSS attacks.
Related reading:
http://hackademix.net/2009/11/21/ies-xss-filter-creates-xss-vulnerabilities/
http://technet.microsoft.com/en-us/security/bulletin/MS10-002
The text was updated successfully, but these errors were encountered: