1302539 - X-Content-Type-Options: nosniff breaks this page in Firefox but not in Chrome

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

http://people.mozilla.org/~jmuizelaar/implementation-tests/nosniff.html

The image doesn't load in Firefox but it does in Chrome.
The image is sent with X-Content-Type-Options: nosniff

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I noticed this on https://duck.co/blog/post/311/yahoo-partnership

Comment 2

[Masatoshi Kimura [:emk]]

IE11 also breaks, but Edge does not.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> http://people.mozilla.org/~jmuizelaar/implementation-tests/nosniff.html
> 
> The image doesn't load in Firefox but it does in Chrome.
> The image is sent with X-Content-Type-Options: nosniff

Different browsers implement XCTO nosniff slightly different. When running your test I get the following error message on the console:
> The resource from “https://duck.co/media/avatar/y/e/yegg/yegg_16” was blocked due to MIME type mismatch (X-Content-Type-Options: nosniff).

It's an image load that uses the content type: 'application/octet-stream'. When using XCTO nosniff images must use a content type that starts with "image/" (see blogpost of allowed content types [1]). It seems that Firefox's implementation aligns with the spec [2] in that regards.

[1] https://blog.mozilla.org/security/2016/08/26/mitigating-mime-confusion-attacks-in-firefox/
[2] https://fetch.spec.whatwg.org/#x-content-type-options-header

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Breaks some pages

Sounds like we need to either get Chrome/Edge to align with the spec or the spec is not web-compatible and needs to change.  Either way, we need to sort this out before we ship 50...

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (TPAC) from comment #5)
> [Tracking Requested - why for this release]: Breaks some pages
> 
> Sounds like we need to either get Chrome/Edge to align with the spec or the
> spec is not web-compatible and needs to change.  Either way, we need to sort
> this out before we ship 50...

Anne, what do you think? Do we need to change the spec? Or other browsers to align with our implementation?

Comment 7

[Anne (:annevk)]

So they basically don't support "nosniff" for images? Sigh. So now we need X-Content-Type-I-Really-Mean-It: nosniff...

Comment 8

[Anne (:annevk)]

I guess for now we could disable this for images and maybe add telemetry at the same time to measure how often this happens?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Do they not support it, or do they just not support it for the specific case of application/octet-stream response?

Comment 10

[Anne (:annevk)]

Mike West says Chrome only supports it for stylesheets and scripts (they might also not support it for all script sources I think).

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

So, what should we do? Should I add a pref surrounding the image check [1] and allow the load for now? And also add telemetry how often that is the case?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1032

Comment 12

[Anne (:annevk)]

Yeah, I think that's the way to go for now.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1302539_nosniff_allow_image.patch

Anne, I am not sure if you want to review those bits, otherwise feel free to delegate. Anyway, I added a pref and telemetry. While doing so I also figured we had a typo within file_nosniff_testserver.sjs, which I also updated within this patch. Thanks!

Comment 14

[Anne (:annevk)]

Comment on attachment 8792598 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

C++ is not mine to review.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 8792598 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

Review of attachment 8792598 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1031,5 @@
>      if (aLoadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_IMAGE) {
>          if (StringBeginsWith(contentType, NS_LITERAL_CSTRING("image/"))) {
>              return NS_OK;
>          }
> +        Accumulate(Telemetry::XCTO_NOSNIFF_BLOCK_IMAGE, 1);

Will this be useful without a comparison to how many images we _don't_ block? Do we have a count of "images in general" elsewhere? Knowing we might block 3 Million images sounds like a lot, but if we encounter 30Billion in the same time period then maybe it isn't.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Will this be useful without a comparison to how many images we _don't_
> block? Do we have a count of "images in general" elsewhere? Knowing we might
> block 3 Million images sounds like a lot, but if we encounter 30Billion in
> the same time period then maybe it isn't.

You are absolutely right, thanks for pointing that out. I updated the patch so telemetry reports include that ratio.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acffb71ed793
X-Content-Type-Options: nosniff should not apply to images (temporarily). r=dveditz

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing wpt /fetch/nosniff/image.html at least on Linux:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5262594e0f3f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=acffb71ed793d03b5e2019a78ed6585f4c18ee8b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=36813171&repo=mozilla-inbound

[task 2016-09-30T08:34:45.167295Z] 08:34:45     INFO - TEST-UNEXPECTED-FAIL | /fetch/nosniff/image.html | URL query:  - assert_unreached: Unexpected load event Reached unreachable code

Comment 19

[Boris Zbarsky [:bzbarsky]]

Right, that test tests that nosniff works for images.  And since the whole point of this patch is to make it not work...

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1302539_nosniff_allow_image.patch

Uploading an updated version of the patch which consists the requested changes regarding telemetry count. Carrying over r+ from dveditz.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1302539_nosniff_allow_images_wpt_updates.patch

Anne, since we are allowing all images to load we have to mark those wpt tests [1] as failing - thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/nosniff/image.html#5

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b24a357512b
X-Content-Type-Options: nosniff should not apply to images (temporarily). r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb7e1d20e40
Update wpt tests because XCTO: nosniff should not apply to images (temporarily). r=annevk

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3b24a357512b
https://hg.mozilla.org/mozilla-central/rev/9eb7e1d20e40

Comment 24

[Georg Fritzsche [:gfritzsche]]

Note that any new data collection needs data review:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Note that any new data collection needs data review:
> https://wiki.mozilla.org/Firefox/Data_Collection

Oh, sorry, I haven't realized that we need data review. Francois, are you fine with that telemetry gathered within this patch?

Comment 26

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> Francois, are you fine with that telemetry gathered within this patch?

Looks fine. You could have also used a "boolean" type instead of "enumerated" since you only have allow/block, but I don't think that's a big deal.

datareview+

Comment 27

[Nazar Mokrynskyi]

https://bugzilla.mozilla.org/show_bug.cgi?id=1303527 was marked as duplicate of this bug, but in that case `X-Frame-Options` is used, not `X-Content-Type-Options`.
After the patch was landed it does work fine, but I'm curious how these 2 options are related (there is no explicit mention on MDN about this).

A bit of background: on my site there is a possibility to upload arbitrary files, so I've added `Content-Type: application/octet-stream` in order to forbid direct file opening (for instance, HTML file, it will now cause download dialog instead) and `X-Frame-Options: DENY` to forbid using uploaded HTML files in iframes on the same website (while iframes from other sources might appear on other pages).

Am I doing it wrong? If this patch will be reverted, what should I do instead to achieve the same result?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> but in that case `X-Frame-Options` is used, not `X-Content-Type-Options`.

Both are used:

bash-3.2$ wget -S -O /dev/null 'https://cleverstyle.org/storage/public/Uploader/2016-05-30/11/2_3530574bfb52e097a4.80611140.jpg' 2>&1 | grep Options
  X-Frame-Options: DENY
  X-Content-Type-Options: nosniff

> I'm curious how these 2 options are related

They're not.

> If this patch will be reverted, what should I do instead to achieve the same result?

Don't send both "Content-Type: application/octet-stream" and "X-Content-Type-Options: nosniff" for images that are meant to actually be linked to from <img> tags.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8797067 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

Approval Request Comment
Different behavior in Chrome and Firefox causes web compatibility issues. We should uplift that change to aurora and beta.

[Feature/regressing bug #]:
Bug 471020 - Add X-Content-Type-Options: nosniff support to Firefox

[User impact if declined]:
Sites using the header XCTO: nosniff might block images within Firefox. Even though Firefox follows the spec, other browsers implement that part differently. We added telemetry around that feature so we can make a better decision based on that data.

[Describe test coverage new/current, TreeHerder]:
We have mochitests (test_nosniff.html) as well as web platform tests.

[Risks and why]:
Low, since it only affects sites using the header XCTO: nosniff.

[String/UUID change made/needed]:
no

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8797068 [details] [diff] [review]
bug_1302539_nosniff_allow_images_wpt_updates.patch

Approval Request Comment
See comment 29.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8797067 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

Fixes a web compat issue, Aurora51+, Beta50+

Comment 32

[Nazar Mokrynskyi]

Well, turns out, `X-Content-Type-Options: nosniff` header is added by CloudFlare under certain circumstances. Waiting for response from them about when and why. I imaging quite a lot of websites might be affected.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e8f3a9e4631
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd5dfc0913d9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf4f0aeb51e7
https://hg.mozilla.org/releases/mozilla-beta/rev/6bf136c04b47

Comment 35

[Florian Scholz (Open Web Docs)]

Updated https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options