User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) X-Content-Type-Options: nosniff is introduced by the IE Team to disable MIME-sniffing (http://blogs.msdn.com/ie/archive/2008/09/02/ie8-security-part-vi-beta-2-update.aspx). Firefox should support this to disable MIME-sniffing. Reproducible: Always Steps to Reproduce: 1. Open http://franklin.pacificstorms.org/IE8-Test/authoritativeMIME.php Actual Results: Feed reading view is displayed Expected Results: A XML DOM tree is displayed

If the feed has a XSLT stylesheet attached, the expected result will be the feed being transformed by the stylesheet.

This is not needed because Gecko doesn't violate the Http RFC unlike IE and doesn't do content-sniffing at all if a content-type is given. Example URL: http://www.mversen.de/mozilla/text/mozilla.rar Any UA should render this gif as text causing a binary data displayed as text. Displaying as gif violates the http RFC.

How about feed sniffing? X-Content-Type-Options: nosniff allows publishers to disable browsers' feed reading view and use their own XSLT stylesheets.

Gecko does sniff in some cases, such as "text/plain" content that is full of control characters, or "text/html" content that looks like an RSS feed. We try to do so in a way that doesn't introduce security holes, but we should probably support this header for security reasons anyway. But I don't think it's right to use the same header to mean "Don't second-guess my MIME type" and "Display this feed using my stylesheet instead of your own UI". The first is a tradeoff between security and compatibility, while the second is a tradeoff between consistency within a site and consistency between feeds. If we want to give web developers a way to disable the built-in feed display, it should be a different header. (I would discourage web authors from self-styling their feeds because that would cause Firefox's convenient "Add to [my preferred feed reader]" button to disappear, but I won't go so far as to argue that Firefox shouldn't support such a header for that reason alone.)

See also the flamewar that is bug 338621, "Feed View overrides XSLT stylesheet defined in XML document".

Another case that I want the nosniff header is to show the DOM tree instead of rendering the XML document. Currently, when an XML document has used XHTML namespace, even if it is not rooted "html", Firefox attempts to render XML document. I hope that the nosniff header can force Firefox render the contents only when the document is served as "application/xhtml+xml". Content-Type: application/xml => XHTML Content-Type: application/xml X-Content-Type-Options: nosniff => DOM Tree

Comment 6 has nothing to do with this bug (because it has nothing to do with MIME type sniffing). I'm reasonably opposed to implementing this header for core Gecko (so text/plain sniffing, basically) unless there's more of a standardization effort involved and the behavior is clearly defined. I refuse to play catch-up on a treadmill IE sets up here. What Firefox does with its feed sniffing is up to the maintainer of that component, of course. If the argument is that type sniffers shouldn't be called by the core at all if this header is sent, I might be able to be convinced of that, though it does restrict what extensions can do in a way that seems unwarranted to me.

This is a mass search for bugs which are in the Firefox General component, are UNCO, have not been changed for 500 days and have an unspecified version. Reporter, can you please update to Firefox 3.6.10 or later, create a fresh profile, http://support.mozilla.com/en-US/kb/managing+profiles, and test again. If you still see the issue, please update this bug. If the issue is gone, please set the status to RESOLVED > WORKSFORME.

In IE, this header also controls CSS sniffing: http://blogs.msdn.com/b/ieinternals/archive/2011/02/11/ie9-release-candidate-minor-changes-list.aspx

this has been supported in chrome since at least chrome 11 according to http://www.browserscope.org/?category=security

So well I started looking into this. A few things are very easy to fix, although I am not sure if it's always the right way. These sniffer instances were easy to find and fix: - imgRequest/imgLoader - nsFeedSniffer - AsyncFaviconHelpers At the moment I am having a hard time figuring out when we decide to parse something as html. Eg. consider this page http://www.browserscope2.org/security/static/x-content-type-options.html. It has no Content-Type and we still parse it as HTML. I tried messing with nsHttpChannel::CallOnStartRequest but no success there :( Oh and tell me when I am missing even more sniffers.

So after even more poking around today and finding https://developer.mozilla.org/en/Document_Loading_-_From_Load_Start_to_Finding_a_Handler, which was hugely helpful to understand what is going on (Thanks to bz). After following the control flow, I found the mentioned nsDSURIContentListener, which looked like a good place to interfere, because it's called for the most thing we handle. And to my surprise this is actually where we handle X-Frame-Options, too. So I put some small check into there and yeah, at least something seems to work. I am not sure if the way I override the Content-Type is acceptable, but this passes http://www.browserscope.org/security/test. I am not sure if I actually still need to patch the other files. I am asking bz for feedback, mostly to know if this might a sensible way to do this. I don't really feel strongly about this feature, it's just a thing I was looking at and found interesting to learn a bit more about all the moving parts from Necko until we parse HTML.

Comment on attachment 627719 [details] [diff] [review] something works Talked this over with Tom. The plan is to just check for the header in nsHttpChannel::CallOnStartRequest and in the HTTP channel code that consults the sniffer category. That should cover all the cases we care about.

This still passes the test, and is more like what Chromium does. Tomorrow I am going to spent some time investigating what IE is actually doing.

Yeah added a test for this. This just test if an file looking like html, with the .html extension, but explicitly not with an Content-Type is loaded as text/plain. This is essentially the same test as on browserscope. Btw: NS_ERROR_NOT_AVAILABLE is totally stupid!

Comment on attachment 628897 [details] [diff] [review] WIP with one test I actually don't intent to change anything right now. I would also like to request some feedback from the sec team.

Comment on attachment 628897 [details] [diff] [review] WIP with one test >+++ b/content/base/test/test_x-content-type-options.html >+document.addEventListener('DOMContentLoaded', function () { >+ var iframe = document.querySelector('iframe'); >+ iframe.addEventListener('load', function () { How about: addLoadEvent(function() { and then your code? There's no reason to specifically listen for load on the iframe, since the iframe load is guaranteed to block the main-page load. (Note also that nothing actually guarantees that DOMContentLoaded will fire _before_ the iframe load. It can fire after. So the test as written would randomly time out, in cases when the iframe load fires before the parent DOMContentLoaded.) >+++ b/netwerk/protocol/http/nsHttpChannel.cpp >@@ -341,17 +341,16 @@ nsHttpChannel::SpeculativeConnect() >- mConnectionInfo->SetPrivate(UsingPrivateBrowsing()); Why? Please put that back! > nsHttpChannel::CallOnStartRequest() >+ if (mResponseHead) { This looks like X-Content-Type-Options will force treatment as text/plain on no type sent even when we have an mContentTypeHint. Is that desired? Or put another way, if a file is sent with X-Content-Type-Options and no type and some page loads it as a stylesheet, should it actually get loaded as a stylesheet or now? With your code as written it would not, which seems reasonable... If this is actually on purpose, I suggest documenting that clearly so that no one is tempted to merge this block with the following one. >+ const nsHttpAtom atom = nsHttp::ResolveAtom("X-Content-Type-Options"); Just add it to nsHttpAtomList.h, please. >+ nsCAutoString typeOptions; >+ nsresult rv = mResponseHead->GetHeader(atom, typeOptions); You can probably get simpler code here if you PeekHeader(): null will mean no header, while non-null will give you a const char* to strcmp against. >+ rv = mResponseHead->GetHeader(nsHttp::Content_Type, contentType); >+ if (NS_SUCCEEDED(rv) || rv == NS_ERROR_NOT_AVAILABLE) { >+ if (rv = NS_ERROR_NOT_AVAILABLE || contentType.IsEmpty()) { Why does this not do what you want? if (mResponseHead->ContentType().IsEmpty()) { // No MIME type. Default to text/plain, since we're not allowed to sniff mResponseHead->SetContentType(NS_LITERAL_CSTRING(TEXT_PLAIN)); } Though actually, to be safe you probably also want to make sure that mResponseHead->ContentType() is not one of the following "magic" types that trigger sniffing behavior in downstream consumers: APPLICATION_GUESS_FROM_EXT UNKNOWN_CONTENT_TYPE Ideally we just wouldn't use types for that, but.... :( In any case, if it's one of those two, we want to set to TEXT_PLAIN as well.

(In reply to Tom Schuster [:evilpie] from comment #16) > Comment on attachment 628897 [details] [diff] [review] > WIP with one test > > I actually don't intent to change anything right now. I would also like to > request some feedback from the sec team. What sort of feedback are you looking for ? Overall, I'm definitely pleased to see movement on this feature from a security perspective. It's also possible this feature needs at least a brief security review - i'll ping dveditz.

I think we need to define some kind of rules (a specification if you will) for when we actually want to block sniffing. In the next patch I am going to apply only sniffing for documents is disabled and at least Chromium seems to follow the same rules. Let me enumerate some examples: - something that looks like html - no Content-Type => displayed as text/plain - an image - no Content-Type => displayed as text/plain - an image - non matching Content-Type (eg. image/png for an jpeg image) => displayed as image We also disable sniffing based on the extension (.html etc.) But on the other hand this also has no effect on for example - images included via the image tag - style sheets (we already block style sheets with the wrong content-type in standard mode) From what I can tell my patch exhibits the same behavior as Chromium. (See: http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/buffered_resource_handler.cc?annotate=139874#l160)

Just attaching the new version, because we still got some discussion to do.

I am wondering if we could ever wrap this up?

(In reply to Tom Schuster [:evilpie] from comment #21) > I am wondering if we could ever wrap this up? I sent a link to comment 19 to dev-security to try to get some more feedback/input on your implementation. I'm also trying to get a security review/discussion set up to drive to some consensus on what we're doing here. Tom, feel free to ping me if this stalls out and I will attempt to get it restarted :)

Marked as sec-review? for triage and to schedule a meeting to discuss what Tom has done and what we think we should do here and also, what we should do about driving a spec and how, if we think we should try to drive a real spec here (there is none so far). In particular, i'm interested in dveditz's thoughts on this (and maybe sicking if he has some...)

I'm looking to spec the appropriate behavior with regard to this header in the mimesniff spec[1], so I've started a discussion on the WHATWG mailing list[2] about what the correct behavior should be. Feedback welcome! [1] http://mimesniff.spec.whatwg.org/ [2] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-November/037974.html

Adam and Anne is right. Dropping "X-" prefix will only add another bloat to HTTP headers forever.

A spec is in progress per comment 24 - Tom is still looking for feedback from security folks etc.

We don't need to drop the X-. The feature is to patch holes in other specs (and in IE's case, a broken implementation when the spec was clear) so it can stay X- until the entire header is no longer necessary. Websites already use this header so the main goal should be compatibility with Chrome (we are more like Chrome than IE which does too much sniffing). Unfortunately the link in comment 19 no longer works so I can't quickly check myself. Patch looks good to me (this is not an r+ though, you'll need to get that from a module peer).

I agree with Dan that less sniffing is better. The spec says that if X-Content-Type-Options: nosniff is sent without a Content-Type, sniffing should be done. This patch does not sniff but follows the Chromium behavior of using 'text/plain' in this case - I think this approach better honors the intention of the header. From comment 19, it sounds like sniffing will only be done for documents/iframes and not <img>, since we already don't sniff for <img> - what about <script> which was also mentioned on the WHATWG thread ? Do we already never sniff that as well ?

(In reply to Ian Melven :imelven from comment #28) > The spec says that if X-Content-Type-Options: nosniff is sent without a > Content-Type, sniffing should be done. This patch does not sniff but follows > the Chromium behavior of using 'text/plain' in this case - I think this > approach better honors the intention of the header. The spec currently does not mention how to handle a X-Content-Type-Options header. The "no-sniff flag" can be set multiple ways, and its use has not yet been extended to encompass the semantics of the X-Content-Type-Options header.

(In reply to Gordon P. Hemsley [:gphemsley] from comment #29) > (In reply to Ian Melven :imelven from comment #28) > > The spec says that if X-Content-Type-Options: nosniff is sent without a > > Content-Type, sniffing should be done. This patch does not sniff but follows > > the Chromium behavior of using 'text/plain' in this case - I think this > > approach better honors the intention of the header. > > The spec currently does not mention how to handle a X-Content-Type-Options > header. The "no-sniff flag" can be set multiple ways, and its use has not > yet been extended to encompass the semantics of the X-Content-Type-Options > header. My apologies for the misstatement ! What I should have said is that the spec says that if no Content-Type is sent, the no-sniff flag is not checked : http://mimesniff.spec.whatwg.org/#determining-the-sniffed-media-type-of-a-resource 2. If the supplied media type is undefined or if the supplied media type is not undefined and the media type portion of the supplied media type is equal to "unknown/unknown", "application/unknown", or "*/*", execute the rules for identifying an unknown media type with the sniff-scriptable flag set and abort these steps. 3. If the no-sniff flag is set, the sniffed media type is the supplied media type. Abort these steps. -- As Gordon says, the spec does not actually mention this header currently.

Please tell me how we are going forward with this. Thanks.

Comment on attachment 629878 [details] [diff] [review] v2 Review of attachment 629878 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good to me, although I am not a peer. Maybe consider adding a test for this case mentioned by Adam in this posting: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-November/037983.html "6) One of the more interesting cases to study is what happens when the server both supplies an X-Content-Type-Options header and also fails to supply a Content-Type header. My recollection is that both IE and Chromium treat the response as text/plain regardless of its contents." Looking at https://wiki.mozilla.org/Modules/All#Content_HTTP_Headers is Gerv someone who should review this as well as a DOM peer ? (https://wiki.mozilla.org/Modules/All#Document_Object_Model) You already have sec-review+ from dveditz, so should be fine to get a review and land IMO. ::: build/pylib/blessings/blessings.egg-info/PKG-INFO @@ +1,1 @@ > +Metadata-Version: 1.1 does this belong in this patch ? ::: content/base/test/file_x-content-type-options.html^headers^ @@ +1,2 @@ > +X-Content-Type-Options: nosniff > +Content-Type: i'm guessing you need the space here ::: content/base/test/test_x-content-type-options.html @@ +15,5 @@ > + var iframe = document.createElement('iframe'); > + iframe.src = "file_x-content-type-options.html"; > + > + iframe.addEventListener('load', function () { > + is(javascript_ran, false, trailing whitespace

(In reply to Ian Melven :imelven from comment #32) > Looking at https://wiki.mozilla.org/Modules/All#Content_HTTP_Headers is Gerv > someone who should review this as well as a DOM peer ? > (https://wiki.mozilla.org/Modules/All#Document_Object_Model) Gerv, is this the type of header that is in the scope of your module?

Comment on attachment 629878 [details] [diff] [review] v2 Review of attachment 629878 [details] [diff] [review]: ----------------------------------------------------------------- Tom, thanks for working on this. I think it is a great idea to implement this. What is the best draft of the specification for this? More questions below. It would be great if you could answer those questions by expanding the tests to handle the cross-product of those cases. ::: content/base/test/test_x-content-type-options.html @@ +25,5 @@ > + }) > + > + document.body.appendChild(iframe); > +}) > +</script> We need to test many more cases. <img> and <script> are ones with which I know for a fact that implementations disagree on. Which implementation are we agreeing with? Also, in each of the <iframe>, <img>, <script> conditions, what happens when the X-Content-Type-Options header doesn't have a corresponding Content-Type header, does have a Content-Type header of a "correct" type (e.g. application/javascript for <script>), and does have a Content-Type header of the "wrong" type (e.g. image/png for application/javascript)? What happens when the header has a value other than "nosniff"? And, what happens when the server sends multiple X-Content-Type-Options headers? I should be able to answer these questions by reading this test. If an appcache manifest is served with X-Content-Type-Options then should we require the text/cache-manifest Content-Type? If an OCSP response, CRL, or X.509 certificate is served with X-Content-Type-Options then what mime types are acceptable? Or, should we ignore the header in those cases?

(In reply to Brian Smith (:bsmith) from comment #34) > Comment on attachment 629878 [details] [diff] [review] > v2 > > Review of attachment 629878 [details] [diff] [review]: > ----------------------------------------------------------------- > > Tom, thanks for working on this. I think it is a great idea to implement > this. What is the best draft of the specification for this? There isn't a specification for this header - see comment 29 > <img> and <script> are ones with which I know for a fact that > implementations disagree on. Which implementation are we agreeing with? AIUI the patch agrees with Chromium - see Tom's comment 19 for reference, Adam's post at http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-November/037983.html has a summary of Chrome's and most of IE's behavior

(In reply to Brian Smith (:bsmith) from comment #34) > Comment on attachment 629878 [details] [diff] [review] > v2 > > Review of attachment 629878 [details] [diff] [review]: > ----------------------------------------------------------------- > > Tom, thanks for working on this. I think it is a great idea to implement > this. What is the best draft of the specification for this? > > More questions below. It would be great if you could answer those questions > by expanding the tests to handle the cross-product of those cases. > > ::: content/base/test/test_x-content-type-options.html > @@ +25,5 @@ > > + }) > > + > > + document.body.appendChild(iframe); > > +}) > > +</script> > > We need to test many more cases. > Sure, more tests is always good. > <img> and <script> are ones with which I know for a fact that > implementations disagree on. Which implementation are we agreeing with? > Also, in each of the <iframe>, <img>, <script> conditions, what happens when > the X-Content-Type-Options header doesn't have a corresponding Content-Type > header, does have a Content-Type header of a "correct" type (e.g. > application/javascript for <script>), and does have a Content-Type header of > the "wrong" type (e.g. image/png for application/javascript)? What happens > when the header has a value other than "nosniff"? And, what happens when the > server sends multiple X-Content-Type-Options headers? I should be able to > answer these questions by reading this test. > We don't do anything new for <img> and <script>. I think we change behavior with <iframe>, but I need to check/test that. > If an appcache manifest is served with X-Content-Type-Options then should we > require the text/cache-manifest Content-Type? > If an OCSP response, CRL, or X.509 certificate is served with > X-Content-Type-Options then what mime types are acceptable? Or, should we > ignore the header in those cases? This seems out of scope for this feature.

Brian: yes, I'd say it is. My understanding is that X-Content-Type-Options: nosniff exists in IE because otherwise IE had some behaviours where it would sniff non-active-content types into active-content-types and that was a security issue. However, the HTML5 sniffing spec was very, very carefully written to avoid this happening, and so there should be no need for a browser which implements that spec correctly to have a switch to disable the sniffing. http://mimesniff.spec.whatwg.org/ Is that understanding correct or deficient? On general principles, I'm in favour of specs, and writing them (particularly if they aren't large) before shipping features rather than after (particularly given that we've done without this for so long), and reverse-engineering existing implementations for compatibility. I would certainly not drop the X. Gerv

Okay I will start to look into new tests for this during the rest of the week.

This is now specced. Feedback welcome: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-May/039561.html

I am not working on this.

(In reply to Gervase Markham [:gerv] from comment #37) > Brian: yes, I'd say it is. > > My understanding is that X-Content-Type-Options: nosniff exists in IE > because otherwise IE had some behaviours where it would sniff > non-active-content types into active-content-types and that was a security > issue. However, the HTML5 sniffing spec was very, very carefully written to > avoid this happening, and so there should be no need for a browser which > implements that spec correctly to have a switch to disable the sniffing. > http://mimesniff.spec.whatwg.org/ > > Is that understanding correct or deficient? I would say that it is not correct. Fictional example: I put a configuration file with a .txt extension in a protected area of my webserver (you need to be logged in to see it), let's say it is at "http://example.org/protected-area/config.txt". Let's say the path is public for whatever reason and an attacker wants to gain access to the file. Let's say that the file looks like this: user="foobar" password="supersecret" The file is served with a proper text/plain content-type header, but as you can see, it would also be valid JS... So now the attacker makes a website that contains this piece of code: <script src="http://example.org/protected-area/config.txt"></script><script>alert(user+'\n'+password)</script> or maybe he just grabs all the new variables from the global scope and sends them back to his server. Any user with permission to access the protected file visiting this website would then unwillingly grab the config file for the attacker and send it to him. Apart from not being supported in enough browsers (like this one), "X-Content-Type-Options: nosniff" is a really nice way to defend against that.

(In reply to Jann Horn from comment #41) > (In reply to Gervase Markham [:gerv] from comment #37) > > Brian: yes, I'd say it is. > > > > My understanding is that X-Content-Type-Options: nosniff exists in IE > > because otherwise IE had some behaviours where it would sniff > > non-active-content types into active-content-types and that was a security > > issue. However, the HTML5 sniffing spec was very, very carefully written to > > avoid this happening, and so there should be no need for a browser which > > implements that spec correctly to have a switch to disable the sniffing. > > http://mimesniff.spec.whatwg.org/ > > > > Is that understanding correct or deficient? > > I would say that it is not correct. Whoops, I should read the pages people link to before replying to them. The attack I mentioned would not work if Firefox implemented that spec correctly.

http://www.whatwg.org/specs/web-apps/current-work/multipage/#the-script-element [[ The src attribute, if specified, gives the address of the external script resource to use. The value of the attribute must be a valid non-empty URL potentially surrounded by spaces identifying a script resource of the type given by the type attribute, if the attribute is present, or of the type "text/javascript", if the attribute is absent. A resource is a script resource of a given type if that type identifies a scripting language and the resource conforms with the requirements of that language's specification. ]] If the script resource specified by the 'src' attribute (without specifying a 'type' attribute) is not served as "text/javascript", it is my understanding that the browser should already ignore it.

> If the script resource specified by the 'src' attribute (without specifying a 'type' attribute) is not served as "text/javascript", it is my understanding that the browser should already ignore it. Unfortunately, that's not the case. For example: http://btoe.ws/security_testing/3e4b3ae04b160f68ac5475e9e98c900d.html It seems like a big problem if there is no way for an application to say "seriously, this is text/plain". It isn't clear what is blocking this issue (is it still the Atom/RSS discussion, or the no content-type scenario?), but it is pretty important for a web app to be able to reiterate that it wasn't kidding about the Content-Type. If more discussion is needed around other issues, could the simple case be addressed first and the other cases be addressed after further discussion?

(In reply to Ben Toews from comment #44) > > If the script resource specified by the 'src' attribute (without specifying a 'type' attribute) is not served as "text/javascript", it is my understanding that the browser should already ignore it. > > Unfortunately, that's not the case. For example: > http://btoe.ws/security_testing/3e4b3ae04b160f68ac5475e9e98c900d.html > > It seems like a big problem if there is no way for an application to say > "seriously, this is text/plain". It isn't clear what is blocking this issue > (is it still the Atom/RSS discussion, or the no content-type scenario?), but > it is pretty important for a web app to be able to reiterate that it wasn't > kidding about the Content-Type. If more discussion is needed around other > issues, could the simple case be addressed first and the other cases be > addressed after further discussion? Ben, good question. The biggest blocker is that nobody is actively working on this at this time. I see the mimesniff spec now specifies that X-Content-Type-Options: nosniff should set the no-sniff flag (see http://mimesniff.spec.whatwg.org/#interpreting-the-resource-metadata) so that removes one of the previous difficulties - so there is now a spec that defines how this header should behave explicitly, thanks to Gordon. What would you consider 'the simple case' ? It seems like different browsers have implemented this differently and indeed the spec was written after IE and Blink/Webkit had already shipped implementations of this. Another problem is that if the WHATWG spec Gordon linked and the mimesniff spec itself were implemented as they stand by Gecko, as Gerv said there would be no need for nosniff. Pragmatically though, as you've pointed out there are real world cases where nosniff is useful and indeed it's now a default header set by Rails 4. I checked http://blog.veracode.com/2013/11/security-headers-on-the-top-1000000-websites-november-2013-report/ but it didn't have any info on X-Content-Type-Options: nosniff so I've asked Isaac if he would add that to the next revision to help gauge usage. Pragmatically, I think one way forward would be for someone to write the tests I asked for in comment 32 and the ones Brian asked for in Comment 37, make sure the behavior the tests check for agrees with the mimesniff spec and that they all pass (might require some tweaks to the patch) and then ask for review from Gerv and a networking peer.

Functionality provided by nosniff is one of the ways of stopping this style of exploit. http://miki.it/blog/2014/7/8/abusing-jsonp-with-rosetta-flash/ If not nosniff, then something to protect FF from applications allow valid ASCII characters to appear in a malicious GET response but are sniffed into an unsuspecting application in the wrong sandbox. (obviously fixing the the root cause is another solution).

GitHub put up a bounty for getting this feature merged. https://www.bountysource.com/issues/3949300-add-x-content-type-options-nosniff-support-to-firefox

Okay I guess I will write some tests for 1000$ ;)

From what I can tell the mimetype spec isn't really going to help us. The sniffing we do is kind of spread out. We have NS_SniffContent and well use the nsIStreamConverterService, which is also doing some kind of sniffing. The easiest way is probably to still just explicitly set text/plain.

(In reply to Tom Schuster [:evilpie] from comment #49) > From what I can tell the mimetype spec isn't really going to help us. Are there ways that mimesniff can be improved to make it more helpful, or is this just an inherent issue in the Gecko codebase?

(In reply to Ben Toews from comment #44) > > If the script resource specified by the 'src' attribute (without specifying a 'type' attribute) is not served as "text/javascript", it is my understanding that the browser should already ignore it. > > Unfortunately, that's not the case. For example: > http://btoe.ws/security_testing/3e4b3ae04b160f68ac5475e9e98c900d.html Ben, this 404s. Could you attach this test case to the bug or at least describe it here so we can make sure the patch (and the mimesniff spec) address the case you're worried about ?

Sorry about that Ian. I took down that site and forgot about that PoC. I put it back up and verified that this is still an issue in FF. That link should work now.

Thanks, Ben. Your test case is pretty simple - JS served with content type text/plain via the src attribute of a <script> element is being executed.. Comment 46 is a good test case as well if this is possible to implement in the browser (as opposed to the Flash plugin) My suggestion would be : * make sure Ben's test case of <script> src doesn't execute (this is a big part of the point of the header from a security perspective IMO) * add a test case for <img> (maybe one making sure it loads as the supplied Content-Type which is not necessarily an image) * add a test case for multiple X-Content-Type-Options headers * add a test case to make sure an invalid x-Content-Type Options header has no effect * add a test case for what happens when the server both supplies an X-Content-Type-Options header and also fails to supply a Content-Type header (Tom does have a blank Content-Type in the existing test in the patch) for the test cases for how <img> <script> <iframe> behave, i'd suggest including a test with the right content type and the wrong content type for each i'd then suggest asking for review (i would be happy to try to help to find someone) and propose to do the OCSP etc stuff in a followup bug after more research on how other browsers or the related specs say to handle it.

For <img>, do we want to decode PNG labeled as image/jpeg? (I suspect we still want to do that, but simply reject non-image MIME types.) There's a whole bunch of contexts this can affect: https://fetch.spec.whatwg.org/#concept-request-context Including workers, XSLT, fonts, audio, video, text tracks. And we don't have a list of valid MIME types for each of those. We very much need to have this covered by either https://mimesniff.spec.whatwg.org/ or HTML as otherwise this will be nightmare to get interoperability on down the road.

(In reply to Anne (:annevk) from comment #54) > > We very much need to have this covered by either > https://mimesniff.spec.whatwg.org/ or HTML as otherwise this will be > nightmare to get interoperability on down the road. My understanding is that there's already interoperability problems with this header between IE and Chrome. There are definitely many contexts this header should probably cover - i imagine not all of them are covered by this header in the existing implementations, whereas the cases that security folk particularly care about (like <script> being executed with type text/plain) are currently covered in other browsers but not Firefox - that could be considered an existing interoperability problem also. I agree that this work absolutely should help drive a spec for this header as much as possible and also should attempt to drive the existing implementations to be more interoperable via the spec.

(In reply to Ian Melven :imelven from comment #55) > (In reply to Anne (:annevk) from comment #54) > > We very much need to have this covered by either > > https://mimesniff.spec.whatwg.org/ or HTML as otherwise this will be > > nightmare to get interoperability on down the road. > > My understanding is that there's already interoperability problems with this > header between IE and Chrome. Could elaborate on what those are or where you heard about them?

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #56) > (In reply to Ian Melven :imelven from comment #55) > > > My understanding is that there's already interoperability problems with this > > header between IE and Chrome. > > Could elaborate on what those are or where you heard about them? According to Adam Barth in http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-November/037983.html : "3) With regards to <img>, Chromium does not consider the X-Content-Type-Options header when determining what to do, but IE does. It's not clear to me what the spec should say given that there is lack of interop here and I don't think you have buy-in from either implementor to change the current behavior." Note that that post is almost 2 years old, it would be great to get some up to date testing of IE and Chrome and Safari to determine differences in behavior on the test cases discussed in the bug so far (like the list i put in comment 53). I don't have access to Windows 8/latest IE currently to get that data myself :(

Some info on how IE behaves from our IE friends : http://msdn.microsoft.com/en-us/library/ie/gg622941(v=vs.85).aspx

Are this still needed to be done?

Yes.

Anne has stated that Gecko and Chrome are pretty close overall when it comes to implementing the WHATWG mimesniff spec as it stands - he and Ted also clued me in to the fact that Mozilla test automation also now runs https://github.com/w3c/web-platform-tests as part of the overall test suite. That said, the path to victory here seems to me like : 1) adding X-Content-Type-Options: nosniff to https://github.com/w3c/web-platform-tests 2) implementing X-Content-Type-Options: nosniff as outlined in https://mimesniff.spec.whatwg.org/ 3) submit patches/edits to the mimesniff spec as/if needed, particularly around the security-related case that's been brought up in this bug by Ben (see comment 53) and to document the behaviour of the contexts Anne brings up in comment 54

Just realized that mimesniff is a whatwg spec and the web platform tests are part of W3C - so we'll see how it goes with getting them accepted. I guess at the very least we can take those tests and port them to Mozilla infrastructure if needed.

Web Platform Tests is used by both organizations. If you provide a Pull Request, I'll make sure it gets reviewed and integrated.

(In reply to Anne (:annevk) from comment #63) > Web Platform Tests is used by both organizations. If you provide a Pull > Request, I'll make sure it gets reviewed and integrated. really appreciate that, Anne !

Great data from Isaac Dawson in https://www.veracode.com/blog/2014/03/guidelines-for-setting-security-headers : For script resources in Internet Explorer, the following content types are valid: application/ecmascript application/javascript application/x-javascript text/ecmascript text/javascript text/jscript text/x-javascript text/vbs text/vbscript For Chrome, the following are supported MIME types: text/javascript text/ecmascript application/javascript application/ecmascript application/x-javascript text/javascript1.1 text/javascript1.2 text/javascript1.3 text/jscript text/livescript

Shouldn't X-Content-Type-Options work the same in every context?

My reading of mimesniff is such that if a Content-Type header is not specified for a resource and X-Content-Type-Options: nosniff is sent with that resource, the algorithm in section 7.1 is followed with the sniff-scriptable flag set to false. At that point, "text/html" can never end up being used as the content-type of the resource but other algorithms to sniff text/plain, images, audio, and archive types are followed. If none of those match, if the resource header (defined in section 5.2) doesn't contain any binary data bytes, 'text/plain' is used - otherwise, 'application/octet-stream' is used. I put this here because I'm trying to figure out how Ben's test case of a script loaded via src attribute should be treated if no Content-Type is specified for it. Per the above, as 'text/plain'. Open to dissention as always.

(In reply to Andre Carneiro from comment #66) > Shouldn't X-Content-Type-Options work the same in every context? Per mimesniff, I'd say yes - there are two major effects of using X-Content-Type-Options: nosniff 1) if the MIME type is supplied (via Content-Type for HTTP) and nosniff is set, the supplied MIME type is used and that's the end 2) if the MIME type is not supplied/undefined, the first step of 7.1 is skipped, since the sniff-scriptable flag will be false after those take place, the same steps are followed regardless of whether nosniff is set (which can include sniffing of text/plain, images, audio, and archive types)

The issue is that <script> elements simply never check the type of the thing that came on the wire. It's not a matter of "sniffing"; they just never consider the type at all. In fact, the only place mimesniff in its present state is really useful as far as I can tell is for document loads. Other contexts have quite different behavior.

(In reply to Please do not ask for reviews for a bit [:bz] from comment #69) > The issue is that <script> elements simply never check the type of the thing > that came on the wire. It's not a matter of "sniffing"; they just never > consider the type at all. > > In fact, the only place mimesniff in its present state is really useful as > far as I can tell is for document loads. Other contexts have quite > different behavior. Context-specific sniffing has yet to be decided. Sniffing in a script context would be described here: https://mimesniff.spec.whatwg.org/#sniffing-in-a-script-context

I should note that my ultimate goal is to have mimesniff describe all sniffing in terms of context, and not have any generic sniffing.

Ben, the biggest thing blocking this is that there is no clear definition of what the behavior should be, with existing implementations disagreeing. And the summary of this bug is broad enough that it covers support in all sorts of situations... So let me ask: what exactly is Github looking for here? Is it support for "X-Content-Type-Options: nosniff" for <script> tags? Or for document loads? Or for something else? You talk about addressing the "simple" case, but not what that "simple" case _is_.

Our desire is for FF to not use documents for unexpected purposes. If the `Content-Type` header is `text/plain`, we would like that response not to be interpreted as JavaScript, HTML, a Flash applet, or a PDF file. I agree that it is problematic having Chrome and IE behaving differently and not following the spec. Following the spec would be to introduce a third behavior, and mimicking Chrome or IE would be to disregard the spec. We're pretty used to Chrome's behavior and the spec seemed reasonable last time I read it. Either option sounds fine to me.

Ben, there is no spec for what <script> should do with this header. More precisely, per spec <script> should run no matter what this header is set to... Alright, so what this bug needs to make progress is a clear indication of what the behavior should actually be. :(

(In reply to Not doing reviews right now from comment #74) > Ben, there is no spec for what <script> should do with this header. More > precisely, per spec <script> should run no matter what this header is set > to... > > Alright, so what this bug needs to make progress is a clear indication of > what the behavior should actually be. :( Here's what the HTML spec says: >>> The src attribute, if specified, gives the address of the external script resource to use. The value of the attribute must be a valid non-empty URL potentially surrounded by spaces identifying a script resource of the type given by the type attribute, if the attribute is present, or of the type "text/javascript", if the attribute is absent. A resource is a script resource of a given type if that type identifies a scripting language and the resource conforms with the requirements of that language's specification. <<< That suggests to me that it is the 'type' attribute that dictates whether a resource is to be run in a script context. The real question on the table is how to determine whether the value of the 'src' attribute identifies "a script resource of the type given by the type attribute". I think maybe this bug should become a meta bug and we should discuss the browsing context implementation separately from the script context implementation (and implementations in other contexts).

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #75) > The real question on the table is how to determine whether the value of the > 'src' attribute identifies "a script resource of the type given by the type > attribute". Actually, I guess that question is already answered by the HTML spec: [[[ A resource is a script resource of a given type if: * that type identifies a scripting language and * the resource conforms with the requirements of that language's specification. ]]] So, if you have the following: <script src="https://raw.githubusercontent.com/w3c/respec/develop/js/text.js"></script> then the browser should run that file as a script, since "text/javascript" is the default value of the 'type' attribute, "text/javascript" refers to JavaScript, JavaScript is a scripting language, and that file is valid JavaScript. Theoretically, the only way to get around this is for the scripting language itself to require a specific Content-Type and/or X-Content-Type-Options header in order to be valid.

That is a requirement for authors, not for user-agents.

(In reply to Masatoshi Kimura [:emk] from comment #77) > That is a requirement for authors, not for user-agents. Oh, fair enough. There's also this section: [[[ The following MIME types (with or without parameters) must not be interpreted as scripting languages: * text/plain * text/xml * application/octet-stream * application/xml ]]] Which I guess indicates the opposite of what I was saying. The bottom line is, if the HTML spec doesn't point to a particular section of the mimesniff spec, then it doesn't really matter what the mimesniff spec says about something, because the HTML spec is more likely to be accurate and cross-browser compatible. Which goes back to my point about sniffing in a browsing context versus other contexts: Implementing sniffing in the browsing context is independent of sniffing in any other context, and it is the only one (AFAIK) that is explicitly referenced by the HTML spec.

Chrome only supports X-Content-Type-Options: nosniff for <script>, but is open to support it for other request contexts. IE11 supports it for a bunch (don't know exhaustively). I have created some tests: https://github.com/w3c/web-platform-tests/pull/1715 I will standardize it as well: https://github.com/whatwg/fetch/issues/35 The set of MIME types Chrome supports when this header is present is equivalent to the MIME types they support in <script type> and are listed here: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util.cc&rcl=1427945811&l=438 As explained in the following email there's quite some differences between browser, even for the single case that is supported in both Chrome and IE11: https://lists.w3.org/Archives/Public/public-webappsec/2015Apr/0004.html I think what we should do for <script> is to require a strict MIME type match if X-Content-Type-Options: nosniff is present and if there's none, treat it as a network error (creates error event and such). The first X-Content-Type-Options header needs to contain the nosniff value (case-insensitive, unquoted). What we do for <script> we can also implement for Worker, SharedWorker, and importScripts(). We can have follow up bugs for contexts that are not yet in Chrome, such as <link rel=stylesheet and <img>. Given the precedent set by Chrome I don't think we need to support it everywhere from the start. We can roll it out slowly. It's a defense-in-depth mechanism for sites.

Do we have any telemetry around how often we mime sniff? Are there any console messages that indicate that the mime type was sniffed?

Related thread on dev-platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/eqNH9xiIVlU

Here is an example of mime sniffing in Firefox: http://research.insecurelabs.org/content-sniffing/nosniff/gifjs.html It's a file that is a valid gif, js and html. The HTML loads the itself twice, once as a gif and once as a javascript. In this case it results in javascript execution. See also: https://stackoverflow.com/questions/34288688/preventing-content-sniffing-type-vulnerabilities-when-handling-user-uplaoded-i/

Hey Dan, having the Loadinfo attached to every channel allows us to query the contentPolicyType (image,script,style) so we can compare the type of the channel with the required type and block the load if necessary. As far as I can tell the first time the needed response headers are available are within nsHttpChannel::ProcessResponse(), so I suppose this is where we shold do the blocking so we don't perform any unecessary Necko work before blocking the load. I will ask someone from Necko also to review the patch obviously, but wanted to make sure I am not missing something from a security point of view. Further, we can block the load and log a message to the console if the types don't match. I have commented out some additional types where I am not sure whether we need to support them or not. I suppose not, but want to make sure. I will deleted the commented out types for the final patch. Finally, I added mochitests and also verified that web platform tests work.

Comment on attachment 8765219 [details] [diff] [review] bug_471020_nosniff_wpt_tests.patch Review of attachment 8765219 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/fetch/nosniff/stylesheet.html @@ -23,5 @@ > link.rel = "stylesheet" > link.onerror = t.unreached_func("Unexpected error event") > - link.onload = t.step_func_done(function(){ > - if(passes[passes.length-1] == urlpart) { > - assert_equals(document.styleSheets.length, passes.length) Please explain this change.

(In reply to :Ms2ger from comment #86) > Comment on attachment 8765219 [details] [diff] [review] > bug_471020_nosniff_wpt_tests.patch > > Review of attachment 8765219 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/web-platform/tests/fetch/nosniff/stylesheet.html > @@ -23,5 @@ > > link.rel = "stylesheet" > > link.onerror = t.unreached_func("Unexpected error event") > > - link.onload = t.step_func_done(function(){ > > - if(passes[passes.length-1] == urlpart) { > > - assert_equals(document.styleSheets.length, passes.length) > > Please explain this change. Oh sorry, as far as I can tell Firefox adds the stylesheet to the document even if it's blocked, so that document.stylesHeets.length also contains the blocked stylesheets instead of only the loaded ones. So the result is 6 instead of 2.

What does/should the spec say? What do other browsers do? In any case we should test the length, whichever answer is correct, so we can converge on a single behaviour.

(In reply to :Ms2ger from comment #88) > What does/should the spec say? What do other browsers do? In any case we > should test the length, whichever answer is correct, so we can converge on a > single behaviour. Sounds like a different bug to me. Happy to keep the .ini file for that test and file a follow up bug to investigate. This bug solely focuses on adding XCTO header nosniff.

Comment on attachment 8765217 [details] [diff] [review] bug_471020_nosniff.patch Review of attachment 8765217 [details] [diff] [review]: ----------------------------------------------------------------- It's close but I think we need another round on this one ::: dom/locales/en-US/chrome/security/security.properties @@ +73,5 @@ > # LOCALIZATION NOTE: Do not translate "RC4". > WeakCipherSuiteWarning=This site uses the cipher RC4 for encryption, which is deprecated and insecure. > + > +# LOCALIZATION NOTE: Do not translate "X-Content-Type-Options: nosniff". > +MimeTypeMismatch=The Script/Style/Image from “%1$S” was blocked due to mime type mismatch (X-Content-Type-Options: nosniff). I'm not keen on "Script/Style/Image". I agree we don't want one message per type, but every resource we add in the future only makes this more awkward. Perhaps worse, forces a retranslation--and that forces us to iterate the MimeTypeMismatch key because if you change the English without changing the key the localization tools won't realize they need to re-translate that string. Could this be simply 'The resource from "%1$S" was blocked due to [...]' -- the URL should give a good enough clue for the developer to hunt down which element was loading it. Give or take redirects, I guess, but redirects mess you up in any case: even if you knew it came from _some_ <script> somewhere on your page, a redirect means it could be any of them. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1603,5 @@ > + aResponseHead->GetHeader(nsHttp::X_Content_Type_Options, contentTypeOptionsHeader); > + ToLowerCase(contentTypeOptionsHeader); > + int32_t idx = contentTypeOptionsHeader.Find("nosniff"); > + // if the header does not start with 'nosniff', then there is nothing to do here > + if (idx != 0) { Does our header implementation trim leading whitespace? If not this isn't going to work reliably. What if in the future someone specifies other supported options? In theory that other option flag might be specified first. This code will match for "nosniffsucks"; we're unlikely to find that, but it'd be wrong if we did :-) If we assume there will never be additional options specified (and I think everyone agrees we don't want to extend this) then I'd be happier with using Equals() instead of Find() -- after making sure whitespace is trimmed, of course. Otherwise you'll need to use Find() and then make sure the beginning is either idx 0 -OR- idx-minus-one is whitespace (and http headers support different kinds of whitespace -- use the necko macro) -AND- that the string ends after 'nosniff' or the following character is whitespace. The first option is easier. @@ +1613,5 @@ > + idx = contentType.FindChar(';'); > + // skip everything after the first appearing ; > + // e.g. "text/javascript;version=1.8" should be translated to "text/javascript" > + if (idx > 0) { > + contentType = Substring(contentType, 0, idx); This again assumes the response headers are whitespace-trimmed. That makes the previous comment easier, but let's add a test for it. I'd be happier if you used net_ParseContentType() from nsURLHelper.h because there are tricky edge cases we should keep in one place. It seems really uncommon to use net_xxx() helper functions outside Necko though (only found two cases) so maybe it's discouraged -- ask the network guys. If it's not OK to use the urlhelper function then this approach is OK (again, assuming leading whitespace is already trimmed for you). @@ +1628,5 @@ > + if (contentType.EqualsIgnoreCase("image/gif") || > + contentType.EqualsIgnoreCase("image/jpg") || > + contentType.EqualsIgnoreCase("image/jpeg") || > + contentType.EqualsIgnoreCase("image/png")) { > + // "image/svg+xml" Why are you exempting svg images? I would expect an <IMG> tag to be able to load any image/* the browser supports if there's a nosniff option. @@ +1639,5 @@ > + if (contentType.EqualsIgnoreCase("application/ecmascript") || > + contentType.EqualsIgnoreCase("application/javascript") || > + contentType.EqualsIgnoreCase("application/x-javascript") || > + contentType.EqualsIgnoreCase("text/ecmascript") || > + contentType.EqualsIgnoreCase("text/javascript")) { I wonder if we should be using the #defines from nsMimeTypes.h here (and for text/css above)? We also need to accept application/json for <script> or we're going to break things.

Comment on attachment 8765219 [details] [diff] [review] bug_471020_nosniff_wpt_tests.patch Review of attachment 8765219 [details] [diff] [review]: ----------------------------------------------------------------- I don't feel confident enough about web-platform tests to review this.

Comment on attachment 8765218 [details] [diff] [review] bug_471020_nosniff_tests.patch Review of attachment 8765218 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz

Thanks Dan, let's have a quick look what I've updated: 1) Console Message: I totally agree with your comment and hence updated the localization to: The resource from “%1$S” was blocked due to mime type mismatch (X-Content-Type-Options: nosniff). 2) Header comparison: Our header implementation trims leading whitespace. In my initial implementation I used Equals() but that caused web platform tests to fail. Those tests are: * "nosniff, no" - which is expected to PASS * "no, nosniff" - which is expected to FAIL Hence my initial implementation checked if the header starts with 'nosniff'. Anyway, you are right, we should be smarter about that. Hence, I updated our implementation and added detailed comments about the steps we are performing before matching for 'nosniff'. While doing so I also figured, if a site is sending XCTO header then most likely a mistake happenend if the actual value is not 'nosniff'. E.g. potentially a typo happenend and sending 'nosnif' will be harmful, hence I think it's a good idea to log a warning to the console in such a case. Further, I updated our mochitests to also check for that scenario by sending: response.setHeader("X-Content-Type-Options", " NoSniFF , foo ", false); 3) Parsing Content Types: When parsing content types, Necko internally already uses net_ParseContentType when parsing the content type [1]. We should trimWhiteSpace() anyway since we are cutting off the version of the type (e.g. text/javascript;version=1.8) before applying the matching algorithm. 4) Pre defined content types: I am using the mime types defined within nsMimeTypes, thanks for the hint. That's way cleaner of course. Hopefully we are not missing any mime types in our comparison algorithm. I browsed through the list and I don't think we are, but can you please also double check to make sure? Here is the list [2]. Thanks! [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpResponseHead.cpp#596 [2] https://dxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#93

Slightly modified the test to send * " NoSniFF , foo " instead of * "NoSniFF" as the XCTO header value. Carrying over r+ from dveditz.

Nit: "mime type" -> "MIME type".

James, implementing XCTO lets these web platform tests pass. Can you accept those changes or do I have to find someone else to review? Two things need explanation: * testing/web-platform/meta/fetch/nosniff/worker.html.ini While the individual subtests within that test are passing, the over state is still error. This is due to this exception [1]. We are using 'expected: ERROR' for a lot of other worker related web platform tests where subtests are passing but the overall state of the harness encounters an error. * testing/web-platform/tests/fetch/nosniff/stylesheet.html As far as I can tell Firefox adds the stylesheet to the document even if it's blocked, so that document.stylesHeets.length also contains the blocked stylesheets instead of only the loaded ones. So the result is 6 instead of 2. It's kind of surprising to me that only stylesheet tests here are using an additional test within |link.onload = t.step_func_done(function(){| because all the other nosniff tests don't. Hence I would assume it's fine to update that part. Thanks! [1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#2169

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #93) > 2) Header comparison: Our header implementation trims leading whitespace. In > my initial implementation I used Equals() but that caused web platform tests > to fail. Those tests are: > * "nosniff, no" - which is expected to PASS > * "no, nosniff" - which is expected to FAIL According to https://mimesniff.spec.whatwg.org/ those should both pass ("any such header") because ',' represents a coalescing of two headers with the same name. According to https://fetch.spec.whatwg.org/ those tests are correct. Both specs appear to be actively maintained (dated in the last few months), yet both claim to govern X-Content-Type-Options behavior. It should be ripped out of one of those. They could be made to match but that seems like a future-bomb waiting to happen when one of them changes. If we believe the fetch spec then the first 7 chars should be "nosniff" and the next character should be either ',' or '\0'. The mimesniff spec behavior is more work. You'd have to slice on ',' before trimming and comparing equal to "nosniff". And you'd have to update the tests to match which then probably breaks other browsers. Anne: which spec wins?

Comment on attachment 8768736 [details] [diff] [review] bug_471020_nosniff.patch Review of attachment 8768736 [details] [diff] [review]: ----------------------------------------------------------------- I think we will regret the fragility of enumerating image MIME types but it yields correct behavior currently; r=dveditz ::: dom/locales/en-US/chrome/security/security.properties @@ +73,5 @@ > # LOCALIZATION NOTE: Do not translate "RC4". > WeakCipherSuiteWarning=This site uses the cipher RC4 for encryption, which is deprecated and insecure. > + > +# LOCALIZATION NOTE: Do not translate "X-Content-Type-Options: nosniff". > +MimeTypeMismatch=The resource from “%1$S” was blocked due to mime type mismatch (X-Content-Type-Options: nosniff). As Anne said, "MIME type". ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1606,5 @@ > + // if there is no XCTO header, then there is nothing to do. > + return NS_OK; > + } > + // XCTO header might contain multiple values which are comma separated, so: > + // a) let's skip all subsequent values This implements the fetch spec, which is what the web-platform tests match so that's consistent. Now we just need to get the MIMEsniff spec to match also. @@ +1625,5 @@ > + // a warning to the console. > + nsAutoString msg; > + msg.AppendLiteral("X-Content-Type-Options header warning: value was '"); > + msg.Append(NS_ConvertUTF8toUTF16(contentTypeOptionsHeader)); > + msg.AppendLiteral("'; did you mean to sent 'nosniff'?"); s/sent/send/ @@ +1656,5 @@ > + if (contentType.EqualsIgnoreCase(IMAGE_GIF) || > + contentType.EqualsIgnoreCase(IMAGE_JPG) || > + contentType.EqualsIgnoreCase(IMAGE_JPEG) || > + contentType.EqualsIgnoreCase(IMAGE_PNG) || > + contentType.EqualsIgnoreCase(IMAGE_SVG_XML)) { This is still fragile: if we add support for more image formats (for example, some people want .webp) someone will have to come in and update this to match every time--or more likely forget to and cause nosniff to break pages. What is the argument for not accepting image/<any> ?

Or at least not accepting anything that imgLoader::SupportImageWithMimeType(contentType.get(), AcceptedMimeTypes::IMAGES_AND_DOCUMENTS) returns true for?

Also, why are we reinventing Content-Type header parsing, when we already have utilities for it?

And yes, the "is a script type" check should be factored out into a helper, used from both here and nsContentUtils::IsPlainTextType. The list of image types being used here definitely doesn't match the list of image types we actually support. Does it match other UA behavior? For example, do image/pjpeg nosniff responses not work in other UAs? image/x-icon responses? image/bmp responses?

Comment on attachment 8768739 [details] [diff] [review] bug_471020_nosniff_wpt_tests.patch Review of attachment 8768739 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/fetch/nosniff/stylesheet.html @@ +21,5 @@ > async_test(function(t) { > var link = document.createElement("link") > link.rel = "stylesheet" > link.onerror = t.unreached_func("Unexpected error event") > + link.onload = t.step_func_done(function(){}) I feel like the behaviour here should be specified one way or another, and we should make sure there is a test for the correct behaviour. So I think that this change is OK if you also add a new test for whatever the specified behaviour is here, even if we fail it.

That part of the test is fundamentally buggy as written right now: it asserts that when the sheet with href given by passes[passes.length-1] is done loading there is some particular number of sheets in the document, but it's doing all the sheet loads in parallel and therefore they can happen in any order. So at that point you don't even know whether the other loads have succeeded or not, no matter what the spec says about document.styleSheets.length.

Incorporated suggestions, carrying over r+ from dveditz.

Comment on attachment 8770524 [details] [diff] [review] bug_471020_nosniff.patch @Boris: Thanks for your suggestions. I think I incorporated all of it: * Using imgLoader::SupportImageWithMimeType() for image loads * Using aResponseHead->ContentType() to query the content type * Added nsContentUtils::IsScriptType() @Pat: Can you have a look at the patch from the Necko point of view? I assume ProcessResponse() is the first method where the response headers are available, right? We still have to wait for Anne's response to Comment 98 before landing, but that part should be easy once we got everything else covert.

Comment on attachment 8770524 [details] [diff] [review] bug_471020_nosniff.patch Much better, thank you!

Comment on attachment 8770524 [details] [diff] [review] bug_471020_nosniff.patch Review of attachment 8770524 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to suggest a minor change.. ::: netwerk/protocol/http/nsHttpAtomList.h @@ +91,5 @@ > HTTP_ATOM(WWW_Authenticate, "WWW-Authenticate") > HTTP_ATOM(Warning, "Warning") > HTTP_ATOM(X_Firefox_Spdy, "X-Firefox-Spdy") > HTTP_ATOM(X_Firefox_Spdy_Proxy, "X-Firefox-Spdy-Proxy") > +HTTP_ATOM(X_Content_Type_Options, "X-Content-Type-Options") nit - the rest of the list is alphabetized.. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1596,5 @@ > + nsILoadInfo* aLoadInfo) > +{ > + if (!aResponseHead || !aLoadInfo) { > + // if there is no response head or no loadInfo, then there is nothing to do > + return NS_OK; whitespace @@ +1657,5 @@ > + if (imgLoader::SupportImageWithMimeType(contentType.get(), > + AcceptedMimeTypes::IMAGES_AND_DOCUMENTS)) { > + return NS_OK; > + } > + return NS_ERROR_CORRUPTED_CONTENT; as this seems like it might be kinda fragile you probably want to also update the error text that goes with CORRUPTED_CONTENT to mention xcto in passing. @@ +1674,3 @@ > nsHttpChannel::ProcessResponse() > { > + nsresult rv = EnforceXContentTypeNoSniff(mResponseHead, mLoadInfo); I'd like you to move this to a new function - ProcessXCTO() and then call it from CallOnStartRequest() as the first thing (before that function invokes the sniffer code) much like ProcessContentSignatureHeader() is called.. that way you can just return a status and you don't need the Cancel() and CallOnStartRequest() at the end.

Thanks Pat, renamed the function and calling it from within ::CallOnStartRequest. Carrying over r+ from dveditz and bz.

I removed the header from MIME Sniffing in https://github.com/whatwg/mimesniff/commit/64bfe025012be3ded16ac4978844acc0e8dfec3c.

(In reply to Anne (:annevk) from comment #110) > I removed the header from MIME Sniffing in > https://github.com/whatwg/mimesniff/commit/ > 64bfe025012be3ded16ac4978844acc0e8dfec3c. Thanks Anne!

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe60708c457 Implement X-Content-Type-Options: nosniff. r=dveditz,bz,mcmanus https://hg.mozilla.org/integration/mozilla-inbound/rev/000576e264bd Test X-Content-Type-Options: nosniff. r=dveditz https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee328c56de0 Update wpt tests for X-Content-Type-Options: nosniff. r=jgraham

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9675ab3d203 Follow up, update quotes within security.properties. r=me

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/610971ea2c3d for w(2) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=31998527&repo=mozilla-inbound

(In reply to Wes Kocher (:KWierso) from comment #114) > All backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/610971ea2c3d for w(2) > failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=31998527&repo=mozilla- > inbound James, it seems the web platform test only fails on > Windows 7 VM opt W3C Web Platform Tests W3C Web Platform Tests W(2) I suppose we should just disable the three 3 tests: * [URL query: ?type=image/gif] * [URL query: ?type=image/png] * [URL query: ?type=image/png;blah] when running tests on Windows 7 VM, does that sound ok to you? If so, what is the exact syntax for that? A quick DXR search did not provide information about windows 7 VM. I would imagine it's something like: > [URL query: ?type=image/png] > expected: > if (os == "win") and (version == "7"): FAIL Do you happen to know? Or do you suggest a different strategy? It seems there is no obvious reason why the tests are passing on all platforms but not on Windows 7 VM. Thanks!

You want something like this attachment, but please check that it works on try or so.

(In reply to James Graham [:jgraham] from comment #116) > You want something like this attachment, but please check that it works on > try or so. Thanks James! Let's make sure we got it all right before re-landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7abb4cad28b5

Ah, I see win7 is not included when running tests on all platforms, let's try this again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ac5933aed7

Hey Carsten, patches got backed out because of Windows 7 VM failures (see comment 114). I think James and I got the right syntax to disable the failing tests for Windows 7 VM. To make sure we got it all right, I pushed to TRY running tests on all platforms (see comment 117): > try: -b do -p all -u all -t none I know that's not that great and efficient to run tests on all platforms, but I want to avoid another surprise when re-landing. Apparently '-p all' does not cover windows 7 VM :-( so tried a different syntax (see comment 118): > try: -b do -p win32,win64 -u web-platform-tests[Windows 7] -t none[Windows 7] Unfortunately that syntax does not cover Windows 7 VM either. Before wasting any more resources, do you happen to know what syntax I have to use to run web-platform tests on Windows 7 VM to make sure everything works as expected? Thanks for your help!

> It seems there is no obvious reason why the tests are passing on all platforms but not on Windows 7 VM. Can we please look into WHY the tests are failing instead of blindly disabling them? Because at some point it's likely that the job where we disabled them ends up the only Windows job running them or something and then we have no test coverage. In general, disabling tests should be a last resort and come with documentation that explains exactly why the test had to be disabled instead of some other action being taken.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #119) > Hey Carsten, patches got backed out because of Windows 7 VM failures (see > comment 114). I think James and I got the right syntax to disable the > failing tests for Windows 7 VM. To make sure we got it all right, I pushed > to TRY running tests on all platforms (see comment 117): > > try: -b do -p all -u all -t none > I know that's not that great and efficient to run tests on all platforms, > but I want to avoid another surprise when re-landing. > Apparently '-p all' does not cover windows 7 VM :-( so tried a different > syntax (see comment 118): > > try: -b do -p win32,win64 -u web-platform-tests[Windows 7] -t none[Windows 7] > > Unfortunately that syntax does not cover Windows 7 VM either. Before wasting > any more resources, do you happen to know what syntax I have to use to run > web-platform tests on Windows 7 VM to make sure everything works as > expected? Thanks for your help! james, do have any hints for best web-platform runs :)

(In reply to Boris Zbarsky [:bz] from comment #120) > Can we please look into WHY the tests are failing instead of blindly > disabling them? Because at some point it's likely that the job where we > disabled them ends up the only Windows job running them or something and > then we have no test coverage. I agree and generally I don't just disable tests. It's slightly unfortunate that the test is only failing on Windows VM 7 which I don't have set up locally and it seems I can't even trigger to run on TRY. > In general, disabling tests should be a last resort and come with > documentation that explains exactly why the test had to be disabled instead > of some other action being taken. We can certainly wait before we land those patches and see what we can do. Maybe I can find someone who can help debug those tests. Please note that we have written our own mochitests which are enabled on all platforms, including windows. Please also note that we have only disabled a subset of image tests; all the other web platform tests run on all the platforms.

For what it's worth, I looked at the test and I found what I was expecting: It's buggy in the typical web platform image test way in testing/web-platform/tests/fetch/nosniff/resources/image.py: body = open(os.path.join(os.path.dirname(__file__), "../../../images/blue96x96.png")).read() That's doing a non-binary read of the file, so if there are any bytes in there that look like Windows newlines the resulting data will get corrupted on Windows. The open() call should get a second argument like so: body = open(os.path.join(os.path.dirname(__file__), "../../../images/blue96x96.png"), "rb").read() and I expect that will fix the problem.

> Ah, I see win7 is not included when running tests on all platforms Oh, and the claim is that it should be. The try push in comment 18 doesn't have completed builds yet, so the tests aren't running yet, as of right now.

(In reply to Boris Zbarsky [:bz] from comment #124) > > Ah, I see win7 is not included when running tests on all platforms > > Oh, and the claim is that it should be. The try push in comment 18 doesn't > have completed builds yet, so the tests aren't running yet, as of right now. In fact they are, but it takes some time before they show up on treeherder. I guess I was posting too hasty. I should have waited a little longer before posting my comment. Anyway, thanks for comment 123, would have taken me ages before I would have even looked within the python script.

Boris, thanks for your help, I really wouldn't have checked the python script at all. Should we run wpt tests on TRY again? Only on windows?

Comment on attachment 8772433 [details] [diff] [review] bug_471020_nosniff_wpt_python_script_update.patch r=me Running a Windows push on try is probably a good idea, just to make sure this really does fix things, yes.

I think the ni? for James has become superfluous - clearing. Here is another TRY run to make sure everything is fine now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0e82eb09b9e

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf0a45011d4 Implement X-Content-Type-Options: nosniff. r=dveditz,bz,mcmanus https://hg.mozilla.org/integration/mozilla-inbound/rev/004aa0e93265 Test X-Content-Type-Options: nosniff. r=dveditz https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fefead2fed Update wpt tests for X-Content-Type-Options: nosniff. r=jgraham https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac674ec465d Update python script of wpt tests to perform binary read of image file. r=bz

https://hg.mozilla.org/mozilla-central/rev/6cf0a45011d4 https://hg.mozilla.org/mozilla-central/rev/004aa0e93265 https://hg.mozilla.org/mozilla-central/rev/a2fefead2fed https://hg.mozilla.org/mozilla-central/rev/0ac674ec465d

I just saw that this issue was marked resolved, but also noticed that the bounty on https://www.bountysource.com/issues/3949300-add-x-content-type-options-nosniff-support-to-firefox is not marked as "paid out". We assumed this might be because the issue was resolved via an internal effort and not from an external contributor. That said, even if you don't want to keep the bounty, feel free to claim it to donate to charity, etc.

Added: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options Updated https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers#Other https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types#MIME_sniffing https://developer.mozilla.org/en-US/Firefox/Releases/50#HTTP