Closed Bug 1302539 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 + fixed
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: jrmuizel, Assigned: ckerschb)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [domsecurity-active])

Attachments

(2 files, 1 obsolete file)

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
Blocks: 471020
IE11 also breaks, but Edge does not.
(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
Duplicate of this bug: 1303527
[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...
(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?
Flags: needinfo?(annevk)
So they basically don't support "nosniff" for images? Sigh. So now we need X-Content-Type-I-Really-Mean-It: nosniff...
Flags: needinfo?(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?
Do they not support it, or do they just not support it for the specific case of application/octet-stream response?
Mike West says Chrome only supports it for stylesheets and scripts (they might also not support it for all script sources I think).
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
Yeah, I think that's the way to go for now.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
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!
Attachment #8792598 - Flags: review?(annevk)
Comment on attachment 8792598 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

C++ is not mine to review.
Attachment #8792598 - Flags: review?(annevk) → review?(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.
Attachment #8792598 - Flags: review?(dveditz) → review+
(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.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acffb71ed793
X-Content-Type-Options: nosniff should not apply to images (temporarily). r=dveditz
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
Flags: needinfo?(ckerschb)
Right, that test tests that nosniff works for images.  And since the whole point of this patch is to make it not work...
Uploading an updated version of the patch which consists the requested changes regarding telemetry count. Carrying over r+ from dveditz.
Attachment #8792598 - Attachment is obsolete: true
Flags: needinfo?(ckerschb)
Attachment #8797067 - Flags: review+
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
Attachment #8797068 - Flags: review?(annevk)
Attachment #8797068 - Flags: review?(annevk) → review+
Pushed by mozilla@christophkerschbaumer.com:
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
https://hg.mozilla.org/mozilla-central/rev/3b24a357512b
https://hg.mozilla.org/mozilla-central/rev/9eb7e1d20e40
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Note that any new data collection needs data review:
https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(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?
Flags: needinfo?(ckerschb) → needinfo?(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+
Flags: needinfo?(francois)
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?
> 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 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
Attachment #8797067 - Flags: approval-mozilla-beta?
Attachment #8797067 - Flags: approval-mozilla-aurora?
Comment on attachment 8797068 [details] [diff] [review]
bug_1302539_nosniff_allow_images_wpt_updates.patch

Approval Request Comment
See comment 29.
Attachment #8797068 - Flags: approval-mozilla-beta?
Attachment #8797068 - Flags: approval-mozilla-aurora?
Comment on attachment 8797067 [details] [diff] [review]
bug_1302539_nosniff_allow_image.patch

Fixes a web compat issue, Aurora51+, Beta50+
Attachment #8797067 - Flags: approval-mozilla-beta?
Attachment #8797067 - Flags: approval-mozilla-beta+
Attachment #8797067 - Flags: approval-mozilla-aurora?
Attachment #8797067 - Flags: approval-mozilla-aurora+
Attachment #8797068 - Flags: approval-mozilla-beta?
Attachment #8797068 - Flags: approval-mozilla-beta+
Attachment #8797068 - Flags: approval-mozilla-aurora?
Attachment #8797068 - Flags: approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.