1018583 - <style>background: url('javascript:while(true){}');</style> hangs Firefox

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mathias Bynens]

Test case:

    data:text/html,<style>*%7Bbackground%3Aurl('javascript%3Awhile(true)%7B%7D')%7D</style>

Credit to Mario Heiderich: https://twitter.com/0x6D6172696F/status/465589939015811072

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tend to think this probably isn't CSS-specific.  Thoughts?

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase with <img> (WARNING: hangs firefox)

Yeah, this works with <img> too.

Maybe belongs in imagelib?  Seems like no good can come from accepting javascript: URIs as images.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: non-hanging testcase, with "while(false)" instead

Here's a non-hanging alternative testcase, with "while(false)" instead of "while(true)", for easier debugging/inspection w/out hanging your browser.

Comment 4

[Daniel Holbert [:dholbert]]

We end up hanging in nsJSChannel::EvaluateScript():
http://mxr.mozilla.org/mozilla-central/source/dom/src/jsurl/nsJSProtocolHandler.cpp?rev=51e11d4c451c&mark=719-721#705

...which is put on the event queue via imgLoader::LoadImage's call to AsyncOpen for this channel:
http://mxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp?rev=9c9833f16a62#1934

(nsJSChannel::AsyncOpen queues up a runnable for EvaluateScript()).

Comment 5

[Daniel Holbert [:dholbert]]

I suspect that we should try to catch js URIs in nsContentUtils::CanLoadImage(), or one of its helpers like NS_CheckContentLoadPolicy...

Perhaps by checking "URI_OPENING_EXECUTES_SCRIPT"?

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v1

(like so?  This fixes the data URI from comment 0 as well as the attached testcase.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(fixed typo in comment)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, technically, this breaks a feature that could work, I think -- a javascript: URL could return image data.  I'm not sure if people use this or if it works in other browsers, but I think I've at least seen demos.

The other question, then, is why we don't have the usual slow-script protections when evaluating javascript: URLs.  It seems like having that would fix this sort of problem for more than just images.  Aren't there other contexts in which javascript URLs can be used that would show the same problem?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> We end up hanging in nsJSChannel::EvaluateScript()

Why are we not triggering the slow script dialog?  That's the real question.

> this breaks a feature that could work, I think

We've very explicitly and purposefully made it work.  That's why javascript: channels default to "execute in sandbox" instead of "do not execute".

If we decide to make changes to javascript: loading policy, we should change that default (which we've been considering) instead of adding hacks elsewhere.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er, I should have read the second paragraph of dbaron's comment... ;)

Oh, and I think at this point we're the only browser that runs javascript: anywhere except for navigation contexts.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432121 [details] [diff] [review]
fix v2

Pretty sure this is not what we want.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  So the reason the interrupt callback is not triggering is that this sandbox doesn't have a window as prototype, and XPCJSRuntime::InterruptCallback tries to convert the global to window, and in the case of a sandbox looks for a window as the proto, and if all that fails just goes ahead and returns true.

So we should either add an out-of-band way to associate a window with this sandbox or just remove the execute-in-sandbox thing altogether.

I'm honestly leaning toward the latter.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> OK.  So the reason the interrupt callback is not triggering is that this
> sandbox doesn't have a window as prototype, and
> XPCJSRuntime::InterruptCallback tries to convert the global to window, and
> in the case of a sandbox looks for a window as the proto, and if all that
> fails just goes ahead and returns true.
> 
> So we should either add an out-of-band way to associate a window with this
> sandbox or just remove the execute-in-sandbox thing altogether.
> 
> I'm honestly leaning toward the latter.

Latter SGTM, as long as it matches the spec. I can figure out a way to make the former work if need be.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, let's remove the execute-in-sandbox thing. My understanding is that that matches what other browsers do? I'm more interested in looking at that than looking at the spec.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the execute-in-sandbox mode for javascript: URIs, and use the don't-execute mode wherever we used the sandbox one

Gavin, any ideas on how I can keep the test coverage in browser tests for this?  The new behavior is that the javascript: doesn't load at all, and I'm not sure there's a good way to test that short of using setTimeout hacks.  :(

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8435810 [details] [diff] [review]
Remove the execute-in-sandbox mode for javascript: URIs, and use the don't-execute mode wherever we used the sandbox one

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

r=me with comments addressed.

::: dom/src/jsurl/crashtests/1018583.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<img src="javascript:while(true){}">

Clever. Add a comment explaining what this is testing?

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ -271,5 @@
>  
> -        // First check to make sure it's OK to evaluate this script to
> -        // start with.  For example, script could be disabled.
> -        if (!securityManager->ScriptAllowed(globalJSObject)) {
> -            // Treat this as returning undefined from the script.  That's what

Can you preserve the first sentence of this comment in the analogous check in the new code?

@@ -298,5 @@
> -        nsCxPusher pusher;
> -        pusher.Push(cx);
> -        rv = xpc->EvalInSandboxObject(NS_ConvertUTF8toUTF16(script),
> -                                      /* filename = */ nullptr, cx,
> -                                      sandboxObj, true, &v);

Can you please write an additional patch that removes the "returnStringOnly" param to EvalInSandbox and all of the associated junk?

Comment 17

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Created attachment 8435810 [details] [diff] [review]
> Remove the execute-in-sandbox mode for javascript: URIs, and use the
> don't-execute mode wherever we used the sandbox one
> 
> Gavin, any ideas on how I can keep the test coverage in browser tests for
> this?  The new behavior is that the javascript: doesn't load at all, and I'm
> not sure there's a good way to test that short of using setTimeout hacks.  :(

Are you saying javascript: would not work in the location bar anymore? Seems like we need to stop using DISALLOW_INHERIT_OWNER then (i.e. bug 1018154).

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Are you saying javascript: would not work in the location bar anymore?

It already doesn't really.  Or rather, it works, as long as you don't try to touch any DOM APIs or anything else that's not a JS builtin.  So it's not clear to me that anyone would consider that actually "working".

But yes, after this patch it simply won't execute at all.

Comment 19

[Frederik Braun [:freddy]]

Will this also affect bookmarklets?

Comment 20

[Boris Zbarsky [:bzbarsky]]

If they currently don't take the sandbox codepath, then no.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Clever. Add a comment explaining what this is testing?

Done.

> Can you preserve the first sentence of this comment in the analogous check in the new
> code?

There is no such thing.  Or more precisely, it's hidden inside nsJSUtils::EvaluateString.

> Can you please write an additional patch that removes the "returnStringOnly" param to
> EvalInSandbox and all of the associated junk?

Coming up.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the returnStringOnly gunk from sandboxes

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8441583 [details] [diff] [review]
part 2. Remove the returnStringOnly gunk from sandboxes

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

\o/

Comment 24

[Boris Zbarsky [:bzbarsky]]

Gavin, review ping?

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8435810 [details] [diff] [review]
Remove the execute-in-sandbox mode for javascript: URIs, and use the don't-execute mode wherever we used the sandbox one

>--- a/browser/base/content/test/general/browser_locationBarExternalLoad.js
>+++ b/browser/base/content/test/general/browser_locationBarExternalLoad.js
>@@ -3,17 +3,19 @@
> 
> function test() {
>   waitForExplicitFinish();
> 
>   nextTest();
> }
> 
> let urls = [
>-  "javascript:'foopy';",
>+  // We used to test javascript: here as well, but now that we no longer run
>+  // javascript: in a sandbox, we end up not running it at all in the
>+  // DISALLOW_INHERIT_OWNER case, so never actually do a load for it at all.
>   "data:text/html,<body>hi"
> ];
> 
> function urlEnter(url) {
>   gURLBar.value = url;
>   gURLBar.focus();
>   EventUtils.synthesizeKey("VK_RETURN", {});
> }
>diff --git a/browser/base/content/test/general/browser_middleMouse_inherit.js b/browser/base/content/test/general/browser_middleMouse_inherit.js
>--- a/browser/base/content/test/general/browser_middleMouse_inherit.js
>+++ b/browser/base/content/test/general/browser_middleMouse_inherit.js
>@@ -14,18 +14,20 @@ function test() {
>     Services.prefs.clearUserPref(middleMousePastePref);
>     Services.prefs.clearUserPref(autoScrollPref);
>     gBrowser.removeTab(tab);
>   });
> 
>   addPageShowListener(function () {
>     let pagePrincipal = gBrowser.contentPrincipal;
> 
>-    // copy javascript URI to the clipboard
>-    let url = "javascript:1+1";
>+    // This used to test javascript: URIs, but those no longer run in a
>+    // sandbox, so in the click case they don't run at all....
>+    // copy data URI to the clipboard
>+    let url = "data:text/html,<body>hi";
>     waitForClipboard(url,
>       function() {
>         Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>                   .getService(Components.interfaces.nsIClipboardHelper)
>                   .copyString(url, document);
>       },
>       function () {
>         // Middle click on the content area
>diff --git a/docshell/test/browser/browser_loadDisallowInherit.js b/docshell/test/browser/browser_loadDisallowInherit.js
>--- a/docshell/test/browser/browser_loadDisallowInherit.js
>+++ b/docshell/test/browser/browser_loadDisallowInherit.js
>@@ -42,17 +42,19 @@ function test() {
>           func();
>         });
>       });
>     });
>   }
> 
>   let urls = [
>     "data:text/html,<body>hi",
>-    "javascript:1;"
>+    // We used to test javascript: here as well, but now that we no longer run
>+    // javascript: in a sandbox, we end up not running it at all in the
>+    // DISALLOW_INHERIT_OWNER case, so never actually do a load for it at all.
>   ];
> 
>   function nextTest() {
>     let url = urls.shift();
>     if (url)
>       testURL(url, nextTest);
>     else
>       finish();

These changes shouldn't be needed anymore.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, I guess I have to wait for stuff to merge over from fx-team...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the execute-in-sandbox mode for javascript: URIs, and use the don't-execute mode wherever we used the sandbox one

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8449955 [details] [diff] [review]
part 1.  Remove the execute-in-sandbox mode for javascript: URIs, and use the don't-execute mode wherever we used the sandbox one

I don't think you need gavin's review at this point, bholley's should be sufficient.

Comment 29

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f9398b90dc
   https://hg.mozilla.org/integration/mozilla-inbound/rev/df2b43d4581e

Comment 30

[Carsten Book [:Tomcat]]

sorry had to this out in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7231daefbb28 since one this changes caused bustages/test failures like

https://tbpl.mozilla.org/php/getParsedLog.php?id=43106706&tree=Mozilla-Inbound
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=43107088&tree=Mozilla-Inbound

Comment 31

[Boris Zbarsky [:bzbarsky]]

I expect the failures were from the other two bugs, but just in case: https://tbpl.mozilla.org/?tree=Try&rev=e3b3f8792f5c

Comment 32

[Boris Zbarsky [:bzbarsky]]

And relanded:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3f139baca5
   https://hg.mozilla.org/integration/mozilla-inbound/rev/22524cb2d69f

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ad3f139baca5
https://hg.mozilla.org/mozilla-central/rev/22524cb2d69f