1288361 - Return a network error for requests whose type is "script" and response has a MIME type that starts with image/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Anne (:annevk)]

Standard: https://fetch.spec.whatwg.org/#should-response-to-request-be-blocked-due-to-mime-type?

This is already implemented by Chrome. A request whose type is "script" includes <script>, new Worker(), new SharedWorker, and importScripts().

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1288361_block_scripts_with_mime_image.patch

Dan, since you already reviewed all of XCTO nosniff I was wondering if you wanna review those bits as well. Fairly similar approach. Thanks!

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1288361_block_scripts_with_mime_image_tests.patch

Comment 3

[Tom S]

Seems like this doesn't actually follow the fetch spec. You are blocking all image mime types that we actually support, but the spec wants us to block all mime types starting with `image/`.

We talked about adding telemetry for the proposed text/csv blocking, should this have telemetry as well?

Comment 4

[Daniel Veditz [:dveditz]]

As a standard 
  If type is "script", and MIMEType starts with `image/`, then return blocked. 

is so arbitrary that it needs some supportive language. Surely there are reasons that could be given to some of the obvious questions:
 * why is "nosniff" an opt-in header if this chunk of its functionality is standard?
   why not just make "nosniff" the standard? (too much breakage, I assume)
 * Why block only image/* ? There are lots of nonsense types we could block, but
   if we're blocking image/ then audio/ and video/ seem obviously equivalent.
 * is there a common set of "misconfigured server" mime types we could accept as a whitelist
   for <script> instead? maybe even as broad as text/ and application/, but of course
   narrower is better.

Really need some telemetry on this. Would prefer having some before deciding what to do, but at the very least this patch should measure what effect it's having.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 8773731 [details] [diff] [review]
bug_1288361_block_scripts_with_mime_image.patch

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

need another round on this one. In addition to the comments before I also think we need to add some telemetry so we have an idea of whether we're breaking lots of stuff, or maybe catching some attacks.

::: dom/locales/en-US/chrome/security/security.properties
@@ +78,5 @@
>  MimeTypeMismatch=The resource from “%1$S” was blocked due to MIME type mismatch (X-Content-Type-Options: nosniff).
>  # LOCALIZATION NOTE: Do not translate "X-Content-Type-Options" and also do not trasnlate "nosniff".
>  XCTOHeaderValueMissing=X-Content-Type-Options header warning: value was “%1$S”; did you mean to send “nosniff”?
> +
> +BlockScriptWithMimeImage=Script from “%1$S” was blocked because its MIME type “image/“ is not executable.

This string is better if you drop "is not executable" (and add "is" before image/). Or better '... MIME type matches "image/*"'  ("is" implies equality, and "image/" isn't a MIME type).

Since I think blocking only image/ is a half-measure and we're likely to eventually block other media types, could this be recast in a more generic way? "Script from %1$S was blocked: disallowed MIME type". Developers who see that can always find out what the MIME type was from dev tools.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1031,5 @@
> +
> +    nsAutoCString contentType;
> +    aResponseHead->ContentType(contentType);
> +
> +    if (imgLoader::SupportImageWithMimeType(contentType.get(),

I agree with evilpie here -- doesn't matter whether we support it or not. We're not rendering the image (it's a script context), the danger is if the _web_site_ supports that type. The spec says to block if the MIME type starts with "image/" and that's what we should do -- simple string compare.

@@ +1052,5 @@
>                         "call OnStartRequest");
>  
> +    nsresult rv = EnsureScriptMIMENotImage(mResponseHead, mLoadInfo);
> +    if (NS_FAILED(rv)) {
> +        LOG(("Blocking script with MIME type image/.\n"));

Please move the logging cruft into the "ensure" function -- CallOnStartRequest doesn't need to be cluttered with it.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8773732 [details] [diff] [review]
bug_1288361_block_scripts_with_mime_image_tests.patch

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

I think we can trust Shared Workers and Service Workers to behave the same, but I think you should add a test for importScripts() inside the worker. IIRC you'll need to catch exceptions for errors rather than use event handlers.

::: dom/security/test/general/test_block_script_mime_image.html
@@ +14,5 @@
> +<script class="testbody" type="text/javascript">
> +/* Description of the test:
> + * We load 2 script files and 2 worker scripts where the first of each
> + * has the correct MIME type and the second of each has a MIME type
> + * of 'image/'. We make sure that the first of each test pass and the

ObNit: MIME type that _starts_with_'image/', not "of" 'image/'.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1288361_block_scripts_with_mime_image.patch

Dan and Tom, thanks for your comments. We should definitely include Anne in the discussion around everything Dan mentioned in comment 4. Further, I agree with both of you: we need telemetry around that feature. The main reason I implemented it without any telemtry was because Chrome already ships it.

Anyway, here is what I propose to push this Bug and also Bug 1048535 forward:
a) I added telemetry for how often 'script' is loaded with MIME: script, image, audio, video and text/csv (relevant for fixing Bug 1048535).
b) I added a pref which disables the feature by default. Potentially telemetry provides enough feedback so we (mabye) can uplift the patch for switching the blocking mechanism on by default. Further, the pref allows people to switch it on themselves if they want. Sounds like another reason to land the feature (prefed off).
c) I updated the tests and also include sub-tests for 'new worker(importScripts())' with correct/incorrect MIME type.

Dan, what do you think?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1288361_block_scripts_with_wrong_mime_tests.patch

Comment 9

[Anne (:annevk)]

See https://bugs.chromium.org/p/chromium/issues/detail?id=433049 for why we want to block image/ (and my reasoning was that we could do so because Chrome already does it), but we should definitely investigate if we can lock things down more.

Comment 10

[Boris Zbarsky [:bzbarsky]]

>+    if (contentType.Find("image/") == 0) {

Please use StartsWith.

Comment 11

[Tom S]

I am interested in this telemetry data, can we get this landed?

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8774115 [details] [diff] [review]
bug_1288361_block_scripts_with_mime_image.patch

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

I think we need another pass on this.

::: modules/libpref/init/all.js
@@ +2103,5 @@
>  pref("security.sri.enable", true);
>  
> +// Block scripts with wrong MIME type
> +// (e.g. block scripts with MIME type 'image/*')
> +pref("security.block_script_with_wrong_mime", false);

Why wouldn't we land with this pref set to true? If you're concerned with it riding the trains prematurely then do an #ifdef holding it to dev edition, but the Nightly version should have the feature on. If it's not ready for that we shouldn't land it. (Sure, giant complex features--e10s for example--should land off for testing, but this is a minor tweak)

[Nits:] Small concern here about the name. We may in the future want to start blocking even more, but if the "more" ends up breaking stuff and users/testers want to back off they'll unset this pref and then be vulnerable to "image/*" which presumably is OK to block at that point. I suppose at that point you could just "build-in" the image checks and make the pref only control the new stuff.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1037,5 @@
> +        Telemetry::Accumulate(Telemetry::SCRIPT_BLOCK_WRONG_MIME, 1);
> +        return NS_OK;
> +    }
> +
> +    if (contentType.Find("image/") == 0) {

As Boris said in comment 10, please use StartsWith() (and in the following audio/video code)

@@ +1042,5 @@
> +        // script load has type image
> +        Telemetry::Accumulate(Telemetry::SCRIPT_BLOCK_WRONG_MIME, 2);
> +
> +        // do not block the load if the feature is not enabled
> +        if (!Preferences::GetBool("security.block_script_with_wrong_mime", false)) {

How hot is this function likely to be? I don't know if getting prefs are still a perf concern but this isn't likely to change so you could use something like Preferences::AddBoolVarCache(). Of course where would you call it? Moving it to the nsHttpChannel init doesn't save us anything since there's a channel per load. I guess we just keep an eye on it--this is the not-normal case anyway and we want to eventually remove the pref.

@@ +1067,5 @@
> +        return NS_OK;
> +    }
> +
> +    // script load has unknown type
> +    Telemetry::Accumulate(Telemetry::SCRIPT_BLOCK_WRONG_MIME, 0); 

Should we check for a couple other really common "wrong" (but default) types to separate them out from the oddballs?

A lot of scripts are sent as text/plain (I think).
Many HTTP servers send text/html for unknown/unconfigured types.
others send application/octet-stream.

I think I'd at least like to know about text/plain ones.

@@ +1084,5 @@
>  
> +    nsresult rv = EnsureMIMEOfScript(mResponseHead, mLoadInfo);
> +    if (NS_FAILED(rv)) {
> +        // log a warning to the console that loading script was
> +        // blocked due to having a wrong MIME type

Really don't like this logging cluttering up ::CallOnStartRequest -- thought I made that comment part of the XCTO review? Please move this and the ProcessXCTO one inside their respective functions.

The logging of the two is identical except for the strings -- make a logging function and share it. In fact, if you have a shared function (that does all the spec conversion and params building) I wouldn't mind leaving a one-line/self-documenting
   ReportTypeBlocking("BlockScriptWithWrongMimeType");
in ::CallOnStartRequest() and equiv in ProcessXCTO().

::: toolkit/components/telemetry/Histograms.json
@@ +5161,5 @@
>      "kind": "enumerated",
>      "n_values": 3,
>      "description": "Whether the user is in safe mode (No, Yes, Forced)"
>    },
> +  "SCRIPT_BLOCK_WRONG_MIME": {

We never got any telemetry into the XCTO blocking. Would it be appropriate to shoehorn into this patch?

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8774115 [details] [diff] [review]
bug_1288361_block_scripts_with_mime_image.patch

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

I have code doing a semi-similar thing in bug 1048535.  The approach I use there is not restricted solely to HTTP requests; for a security concern, I think this is a feature, not a bug, over effectively a blacklist with two protocols on it.  bz blessed the approach I used in bug 1048535 comment 48, but he's also apparently *aware* of the approach taken here, so I don't know exactly what you/we want to do.  And now he's on vacation, bah.

Your approach does have the nice benefit of cutting off requests after headers are available, not continuing to download something that'll ultimately be thrown away.  I didn't see how to do that in nsScriptLoader (that is, I couldn't find exactly where OnStartRequest would happen amidst the code, nor the exact point where one might inject code to hook into it), but I didn't look closely.  Maybe it's possible.

Anyway, not sure how we want to do this, or how exactly to coordinate it.  I should say I've been punting bug 1048535 followuping for awhile due to too many actual-JS things to do, so if I had the possibility to drop it (with assurance someone else would be picking it up), that would definitely be nice solely from my point of view.  :-)

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1033,5 @@
> +    aResponseHead->ContentType(contentType);
> +
> +    if (nsContentUtils::IsScriptType(contentType)) {
> +        // script load has type script
> +        Telemetry::Accumulate(Telemetry::SCRIPT_BLOCK_WRONG_MIME, 1);

For script loads triggered by the exact set of requests in comment 0, nsContentUtils::IsJavascriptMIMEType is the right condition to use.  The query here would also wrongly allow {application,text}/json for those requests, and it would omit a laundry list of garbage types that apparently we've decided should be JS.  I don't know whether "script load", as determined by the GetExternalContentPolicyType() comparison, is restricted to exactly the operations in comment 0, or instead extends beyond that.

@@ +1045,5 @@
> +        // do not block the load if the feature is not enabled
> +        if (!Preferences::GetBool("security.block_script_with_wrong_mime", false)) {
> +            return NS_OK;
> +        }
> +        return NS_ERROR_CORRUPTED_CONTENT;

I'd think there's a better error to use than this, but if not, maybe we should add one?

@@ +1068,5 @@
> +    }
> +
> +    // script load has unknown type
> +    Telemetry::Accumulate(Telemetry::SCRIPT_BLOCK_WRONG_MIME, 0); 
> +    return NS_OK;

What dveditz said.

Also, the HTML spec has semi-aspirational language defining "JavaScript MIME type" that says text/{plain,xml} and application/{octet-stream,xml} must not be interpreted as scripting languages, so I would be absolutely sure to do something special for those.

Comment 14

[Anne (:annevk)]

Per the Fetch Standard a blob or data URL with a MIME type that starts with `image/` would also be blocked, so we should probably fix that up.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This is where the fetch standard's assumption that it knows the full set of supported protocols runs into reality...  What should happen for file:// URLs with image/ types?  What about an extension-implemented protocol?  Maybe we should add a protocol flag for whether this thing should be enforced or something....  or just enforce across the board and deal with the file:// fallout if OSes have bogus extension to type mappings. 

(Yes, Anne I know you think extensions should not implement protocols.  In practice, they do _very_ often; I seem to recall that at least 3 of the top 10 most popular extensions implement protocol handlers.)

Comment 16

[Anne (:annevk)]

Extensions are not part of the cross-browser web, so that would be up to each browser. I don't see the problem for file URLs though it requires the OS to have some kind of MIME type system and the browser to tunnel that somehow into a Content-Type header. Same for extensions. In practice the requirement applies mostly to blob and data URLs for us, and new future cross-browser schemes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> so that would be up to each browser

By the "don't read between the lines" standard usually applied to whatwg specs, any time you have a whitelist of protocols for which something is supposed to be done, doing it for any protocol _not_ in that list is not spec compliant.  You can't have it both ways, with "don't read between the lines, except when the spec editor implicitly expects you to read between the lines".

But in this case there is no need to even read between the lines.  https://fetch.spec.whatwg.org/#concept-basic-fetch clearly says browser extensions are not allowed to implement protocols and browsers are not allowed to implement any new protocols as far as I can tell: a browser is required to return a network error for something not in the protocol whitelist.

So for example, an extension that added tel: support to a browser (not uncommon, fwiw) would cause that browser to not be spec-compliant if it hooked into the fetch pipeline.  But it would be spec compliant if it just prevented the default action on link click and did something with the link URL.... 

Anyway, for the image/ thing what what the fetch spec _actually_ says to do is... that it defines things in terms of whether the response has a Content-Type header in its header list, and for file:// the fetch spec doesn't say what the header list looks like, or whether it's even nonempty.

> I don't see the problem for file URLs

Say the OS maps the ".js" extension to "image/png".  Should <script> references from HTML files on disk to ".js" files on disk fail?  This is a question about desired behavior, not the spec mechanics of achieving it.

Comment 18

[Anne (:annevk)]

tel URLs would never reach Fetch. They would be handled during navigation, which allows such extensions. (I agree that Fetch doesn't allow for extensions at the moment. If you violate that, you're on your own. I think that even if we allowed for it though it'd still be largely undefined what actually happens, as with file URLs.)

> Say the OS maps the ".js" extension to "image/png".  Should <script> references from HTML files on disk to ".js" files on disk fail?

I think so. Note that Fetch says "For now, unfortunate as it is, file and ftp URLs are left as an exercise for the reader." in the part where those resources are supposed to be located and turned into responses.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1288361_block_scripts_with_mime_image.patch

Dan, Jeff, here is what I updated based on your feedback from comment 12 and comment 13:

(a) The pref for blocking script loads using a content-type of 'image/' is now *on* by default. Also updated the name to clearly indicate that 'image/' types are blocked, which makes it easier to switch on/off other types which we are going to block potentially in the future.

(b) Replacing Preferences::GetBool() with a singleton mechanism so we can use Preferences::AddBoolVarCache() to speed things up.

(c) Added a helper function 'ReportTypeBlocking' and also calling it from within ProcessXCTO() as well as EnsureMIMEOfScript(); not polluting CallOnStartRequest(); it actually makes sense not to call it within CallOnStartRequest() also with respect to Bug 1048535 where we most likely want to log a different warning/error to the console.

(d) Extended telemetry to also cover additonal content-types. If we want telemetry for XCTO: nosniff then I suppose we should file another bug; I haven't coved that telemetry within this changeset.

(e) I suppose NS_ERROR_CORRUPTED_CONTENT [1] is the right return value, we updated the description to mention 'potentially MIME type mismatch ...'. If you still think we should use a different error, please let me know!

(f) Don't know how to proceed with blob and data (see comment 14 and discussion thereafter). If we want to incorporate that, can we leave that for a follow up?

[1] https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpc.msg#142


@Jeff: I propose to fix (land) this bug first and then incorporate changes from Bug 1048535 into this setup. It should be fairly straight forward once we have the infrastracture from this bug in place. Agreed?

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 8781541 [details] [diff] [review]
bug_1288361_block_scripts_with_mime_image.patch

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

r=dveditz

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5be08fc846
Return a network error for requests whose type is script and response has a MIME type that starts with image. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dab8cd5e6f9
Test block script with wrong MIME type. r=dveditz

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da5be08fc846
https://hg.mozilla.org/mozilla-central/rev/8dab8cd5e6f9

Comment 23

[Jeff Walden [:Waldo]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> (e) I suppose NS_ERROR_CORRUPTED_CONTENT [1] is the right return value, we
> updated the description to mention 'potentially MIME type mismatch ...'. If
> you still think we should use a different error, please let me know!

Most corrupted content will have nothing to do with nosniff and will have a different proximate cause.  And lots of nosniffed content probably is probably some other valid thing, we're just being strict about it.  A different error still seems preferable to me to differentiate two typically-different failure modes.

> @Jeff: I propose to fix (land) this bug first and then incorporate changes
> from Bug 1048535 into this setup. It should be fairly straight forward once
> we have the infrastracture from this bug in place. Agreed?

Yes, very much so.

Comment 24

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/javascript-served-with-wrong-mime-type-will-be-blocked/

Comment 25

[Jean-Yves Perrier [:teoli]]

Added a comment near the top of:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/scrip
and
https://developer.mozilla.org/en-US/docs/Web/API/HTMLScriptElement

as well as a line in:
https://developer.mozilla.org/en-US/Firefox/Releases/51#Networking

Comment 26

[Jean-Yves Perrier [:teoli]]

Mmmh, I was too quick.

1. the first link is: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

2. Also I added some info in:
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/importScripts
https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker#Exceptions
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorker/ServiceWorker#Exceptions

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This bug fixes the PoC currently trending on the front page of Hacker News. I'm wondering if we should consider this for 50.1 since we're waiting until late January otherwise.

http://blog.portswigger.net/2016/12/bypassing-csp-using-polyglot-jpegs.html

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Any thoughts on this, Dan?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Guess not.