Closed Bug 1428473 Opened 5 years ago Closed 3 years ago

Support X-Content-Type-Options: nosniff when navigating

Categories

(Core :: DOM: Security, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: x, Assigned: sstreich)

References

(Regressed 1 open bug)

Details

(Keywords: sec-other, Whiteboard: [domsecurity-backlog1] [post-critsmash-triage][adv-main70-])

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36 OPR/49.0.2725.64

Steps to reproduce:

Request:
GET /nosniff.php/test.html

Server response headers:
X-Content-Type-Options: nosniff
Content-Disposition: inline
Content-Type: garbage



Actual results:

If not have sub part content-type (with `/`)
Or if not valid main content-type (before /)
e.g. has 1 or more [\s;] (Content-Type: text[space]/html)

Then content-type detecting from extension from location.pathname (if has).
e.g 
/nosniff.php/test.html
/nosniff.php/test.xml
/nosniff.php/test.swg


Expected results:

Header `X-Content-Type-Options: nosniff` for check Content-type header, but not for detecting content-type. 
If content-type not valid - ok, Firefox must show download dialog.
Christoph, looks like you worked on XCTO:nosniff, do you have any idea what's going on here?
Flags: needinfo?(ckerschb)
Summary: Sniffing content-type with nosniff → Document content-type determined from pathname when served with nosniff and garbage Content-Type header
(In reply to :Gijs from comment #1)
> Christoph, looks like you worked on XCTO:nosniff, do you have any idea
> what's going on here?

XCTO:nosniff only applies to script and style types (fwiw, even images turned out to be incompatible), but not top-level documents. Firefox will prompt the download manager if it can't render the requested document in the browser. Is this not what's happening? XCTO:nosniff should have no influence on making that decision.
Flags: needinfo?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> XCTO:nosniff only applies to script and style types (fwiw, even images
> turned out to be incompatible), but not top-level documents. Firefox will
> prompt the download manager if it can't render the requested document in the
> browser. Is this not what's happening? XCTO:nosniff should have no influence
> on making that decision.

Ok, but IMHO it's very strange that I can control content-type through pathname. I found vulnerability in SaaS with this trick, but think Firefox not right (so primarily report to Bugzilla)
(In reply to secator from comment #3)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> > XCTO:nosniff only applies to script and style types (fwiw, even images
> > turned out to be incompatible), but not top-level documents. Firefox will
> > prompt the download manager if it can't render the requested document in the
> > browser. Is this not what's happening? XCTO:nosniff should have no influence
> > on making that decision.
> 
> Ok, but IMHO it's very strange that I can control content-type through
> pathname.

Reporter: Only if the content-type itself is bogus, right?

:jduell, do you know what the spec says ought to happen here, and/or if we're actually doing something wrong (irrespective of XCTO:nosniff) ?


> I found vulnerability in SaaS with this trick

Did you report that to the SaaS vendor? Is that vulnerability still open in the affected server software? Was the same vulnerability open to abuse from other browsers, and if so which (and did you report it to any other browsers)?
Flags: needinfo?(x)
Flags: needinfo?(jduell.mcbugs)
> Reporter: Only if the content-type itself is bogus, right?

Yeah

> > I found vulnerability in SaaS with this trick
> Did you report that to the SaaS vendor?

No, I'm waiting for Mozilla decision.

> Is that vulnerability still open in the affected server software? 

Yes.

> Was the same vulnerability open to abuse from other browsers, and if so which (and did you report it to any other
> browsers)?

No. Chromium and IE prompt the download manager.
Flags: needinfo?(x)
For comparison

Content-type: garbage/garbage - donwload manager
Content-type: garbage - render
Anne: should nosniff apply to top-level documents? seems Chrome and IE are behaving differently from Firefox.
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(annevk)
Keywords: sec-other
Product: Firefox → Core
Group: core-security → dom-core-security
Is there any difference here with using X-Content-Type-Options: nosniff or not in other browsers? Currently the specification doesn't define anything about nosniff for top-level loads, but maybe it should.

Pathname-based sniffing is a thing that exist in the HTML Standard for plugins, but again, not for top-level loads, just <embed> and maybe <object>.

There is content-based sniffing as per https://mimesniff.spec.whatwg.org/, but that shouldn't detect XML.

It'd be good to have some tests.
Flags: needinfo?(annevk)
XML detect:
- if pathname ends with .xml 
- without extension but content start with <?xml ... ?>

HTML detect:
- if pathname ends with .html 
- without extension but content contains html tags
I tested 

  HTTP/1.1 200 OK
  Content-Type: garbage
  X-Content-Type-Options: nosniff

  <?xml version="1.0"?><test/>

both with and without the X-Content-Type-Options: nosniff header and both with and without a .xml path end. The .xml in the path did not matter, but the header did.

With the header present Chrome will render as text and Safari will download. Firefox always renders as XML.
I suspect we could fix this by having the same X-Content-Type-Options: nosniff check we have elsewhere here: https://searchfox.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#838. Jason can probably weigh in on that.

(Fixing the corresponding standard for this seems harder and probably requires more time, but we don't have to wait on that.)
(In reply to secator from comment #5)
> > > I found vulnerability in SaaS with this trick
> > Did you report that to the SaaS vendor?
> 
> No, I'm waiting for Mozilla decision.

You should report it to the vendor. Even if we do change the behavior here (and there's not much in the nosniff spec) there are lots of old Firefox around that this SaaS vendor should try to protect.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
> You should report it to the vendor.

I sent a report. Triaged.
FYI, I just got a twitter message from an external reporter about this very same bug.
Chrome disallows sniffing even if the content-type is empty (or garbage), when a nosniff-header is present.

This is in contradiction with the spec §7.1, where it says that if the "supplied MIME type" is null or undefined, it should sniff. Regardless of the nosniff header. (https://mimesniff.spec.whatwg.org/#determining-the-computed-mime-type-of-a-resource)


Maybe it's time to change the spec and modify the implementation as suggested by annevk in comment 11?
Flags: needinfo?(annevk)
I filed https://github.com/whatwg/mimesniff/issues/82 on changing the standard. Changing the standard is quite a bit of effort that I hope to make time for, but I wouldn't want to block this bug on that. If we can disable sniffing when X-Content-Type-Options: nosniff is present quite easily we should do so.
Flags: needinfo?(annevk)
Duplicate of this bug: 1498523
Summary: Document content-type determined from pathname when served with nosniff and garbage Content-Type header → Document content-type determined from pathname when served with nosniff and garbage or missing/empty Content-Type header
Dragana: looks like comment 15 is the fast fix here
Flags: needinfo?(jduell.mcbugs) → needinfo?(dd.mozilla)
Just FYI, I plan to make a larger disclosure on mime type issues in early February.

I consider the fact that XCTO nosniff does not disable HTML sniffing a security flaw. The spec may say so, but this heavily violates expectations and opens up practical XSS attacks (see e.g. bug 1509518).
Summary: Document content-type determined from pathname when served with nosniff and garbage or missing/empty Content-Type header → Support X-Content-Type-Options: nosniff when navigating
Attached patch xcto_nosniff.patch (obsolete) — Splinter Review

Dragana, as discussed on Slack I am trying to tackle that bug. Here is my approach:

  • In case XCTO nosniff is present within ProcessXCTO() then I set a newly added flag on the loadinfo - I call it skipContentSniffing.
  • Later in the NsNetutil where we actually do the sniffing, we just bail out early in case the flag is true within the loadinfo.

I am not sure if that is the best approach, you mentioned that we might be able to use the channel flag LOAD_CALL_CONTENT_SNIFFERS - do you think it might be possible to just remove the flag in ProcessXCTO if necessary?

Finally, how do we create an automated test for that?

Assignee: nobody → ckerschb
Attachment #9048429 - Flags: feedback?(dd.mozilla)

Regarding automated tests:

  • load file-bogus-content-type-nosniff.html in an iframe
  • content of the file should be along the lines of <script>ok(false, 'This shouldnt parse as HTML or execute JS')</script>
  • supply headers as in comment 0, via a ^headers file.

If I'm not mistaken we also seem to have tests for the download prompt in /uriloader/exthandler/tests/mochitest/download.sjs#6

An example of a sniffer test is:

https://searchfox.org/mozilla-central/source/netwerk/test/unit/test_plaintext_sniff.js

you can do something like that.

Flags: needinfo?(dd.mozilla)
Comment on attachment 9048429 [details] [diff] [review]
xcto_nosniff.patch

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

It looks like LOAD_CALL_CONTENT_SNIFFERS only prevents sniffing by the listeners, like imgloader, mediasniffer (the once that implement GetMIMETypeFromContent[1]) The sniffer is called after [3]

We also have other sniffing like nsUnknownDecoder. It calls NS_SniffContent but it also does other sniffing[2]. Neither using LOAD_CALL_CONTENT_SNIFFERS nor your change in NS_SniffContent will prevent all sniffings in nsUnknownDecoder. We will need to prevent nsUnknownDecoder as well. It would be good to have test to verify this...

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN17nsIContentSniffer22GetMIMETypeFromContentEP10nsIRequestPKhjR12nsTSubstringIcE&redirect=false
[2] https://searchfox.org/mozilla-central/source/netwerk/streamconv/converters/nsUnknownDecoder.cpp#472
[3] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1648
Attachment #9048429 - Flags: feedback?(dd.mozilla)

(In reply to Dragana Damjanovic [:dragana] from comment #22)

Comment on attachment 9048429 [details] [diff] [review]
xcto_nosniff.patch

Thanks Dragana for the detailed notes - I'll look into that and also write some automated tests!

Status: NEW → ASSIGNED

Basti will take over this bug - assigning to him.

Assignee: ckerschb → streich.mobile
Type: defect → task

Basti, here is a half baked patch (including test), both the patch as well as the test need to be modified and updated. I started working on this but then had too many other things on my plate.

In short: We have to identify all the cases where we do sniffing for navigations and bail out of that sniffing. I guess the easiest way is to set a flag as in the patch on the loadinfo and then just check the flag in those cases - we can talk more whenever you start working on it.

Attachment #9048429 - Attachment is obsolete: true

Please also compare with Chrome Canary and Safari Technology Preview to ensure we're reasonably compatible. (It's not going to be great as the fallbacks are different, from what I remember, but we should know what we're in for.)

Attachment #9070207 - Attachment is obsolete: true

Hey, i already applied the feedback of Henri (hsivonen) on the Patch but as he's currently away (until august) and it would be great to land the patch this cycle.
Is there anyone else from the html/parser team that could possibly review the Patch again?

Thanks So much,
Basti :)

Flags: needinfo?(htsai)
Flags: needinfo?(htsai)
Keywords: checkin-needed
Blocks: 1570658
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Blocks: 1571415
Flags: qe-verify-
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1] [post-critsmash-triage]
Depends on: 1579179
Duplicate of this bug: 1579772
Regressions: 1580607
Blocks: 1581512
Depends on: 1585055
Regressions: 1582671
Whiteboard: [domsecurity-backlog1] [post-critsmash-triage] → [domsecurity-backlog1] [post-critsmash-triage][adv-main70-]
Depends on: 1591932
Blocks: 1592651
Regressions: 1602213

Sorry, I commented on bug 1602213 before realizing that this one was still hidden. (It was easier to notice that bugs were restricted when the whole background color was different instead of just a banner at the top. Please hide bug 1602213 if considered necessary.)

Chromium still runs its encoding detector, so for non-Latin pages, we can end up in a situation where a page is completely unreadable in Firefox but readable in Chromium. See http://dolnocerovene.tk/site/novini.php . This is a Web compat problem.

Do we have a really strong security reason to turn off the encoding detector when Chromium doesn't? I.e. do we have a strong reason not to take the patch I attached to bug 1602213?

Flags: needinfo?(sstreich)

...and that bug got another duplicate. I think it's not practical to let nosniff affect the character encoding.

Explaining on this hidden side why I posted a patch that undoes a part of this for review:

I think it's not workable for Firefox to let headers that people add to legacy content "for security" break the content in ways that the content doesn't break in in Chrome. The breakage from security headers should be limited to what is easily discovered by testing in Chrome. Otherwise, we create a Web compat problem.

Furthermore, for avoidance of encoding sniffing to be an effective security measure, we'd need a story for why it would be OK to go to the previous state where lack of sniffing could still result in different interpretations on .com based on the UI localization.

Flags: needinfo?(sstreich)
Depends on: 1594766
No longer regressions: 1645796
Regressions: 1645796
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.