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

RESOLVED FIXED in mozilla33

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Mathias Bynens, Assigned: bz)

Tracking

({site-compat})

Trunk
mozilla33
site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
I tend to think this probably isn't CSS-specific.  Thoughts?
Flags: needinfo?(bzbarsky)
Created attachment 8432111 [details]
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.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Created attachment 8432112 [details]
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.
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()).
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"?
Created attachment 8432120 [details] [diff] [review]
fix v1

(like so?  This fixes the data URI from comment 0 as well as the attached testcase.)
Created attachment 8432121 [details] [diff] [review]
fix v2

(fixed typo in comment)
Attachment #8432120 - Attachment is obsolete: true
Attachment #8432121 - Flags: review?(bzbarsky)
Flags: in-testsuite?
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?
> 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.
Flags: needinfo?(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 on attachment 8432121 [details] [diff] [review]
fix v2

Pretty sure this is not what we want.
Attachment #8432121 - Flags: review?(bzbarsky) → review-
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.
Flags: needinfo?(jonas)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(jonas)
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.  :(
Attachment #8435810 - Flags: review?(gavin.sharp)
Attachment #8435810 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]

Updated

3 years ago
Keywords: dev-doc-needed, site-compat
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?
Attachment #8435810 - Flags: review?(bobbyholley) → review+
(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).
> 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.
Will this also affect bookmarklets?
If they currently don't take the sandbox codepath, then no.
> 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.
Created attachment 8441583 [details] [diff] [review]
part 2. Remove the returnStringOnly gunk from sandboxes
Attachment #8441583 - Flags: review?(bobbyholley)
Comment on attachment 8441583 [details] [diff] [review]
part 2. Remove the returnStringOnly gunk from sandboxes

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

\o/
Attachment #8441583 - Flags: review?(bobbyholley) → review+
Depends on: 1018154
Gavin, review ping?
Flags: needinfo?(gavin.sharp)
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.
Oh, I guess I have to wait for stuff to merge over from fx-team...
Created 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
Attachment #8449955 - Flags: review?(gavin.sharp)
Attachment #8435810 - Attachment is obsolete: true
Attachment #8435810 - Flags: review?(gavin.sharp)
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.
Attachment #8449955 - Flags: review?(gavin.sharp) → review+
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f9398b90dc
   https://hg.mozilla.org/integration/mozilla-inbound/rev/df2b43d4581e
Flags: needinfo?(gavin.sharp)
Flags: in-testsuite?
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla33
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
I expect the failures were from the other two bugs, but just in case: https://tbpl.mozilla.org/?tree=Try&rev=e3b3f8792f5c
And relanded:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3f139baca5
   https://hg.mozilla.org/integration/mozilla-inbound/rev/22524cb2d69f
https://hg.mozilla.org/mozilla-central/rev/ad3f139baca5
https://hg.mozilla.org/mozilla-central/rev/22524cb2d69f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1061656
Keywords: dev-doc-needed
Blocks: 988672
You need to log in before you can comment on or make changes to this bug.