Closed Bug 1018583 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: mathias, Assigned: bzbarsky)

References

Details

(Keywords: site-compat)

Attachments

(5 files, 2 obsolete files)

testcase with <img> (WARNING: hangs firefox)
53 bytes, text/html
Details
non-hanging testcase, with "while(false)" instead
54 bytes, text/html
Details
fix v2
1.27 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
part 2. Remove the returnStringOnly gunk from sandboxes
9.62 KB, patch
bholley
: review+
Details | Diff | Splinter 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
9.71 KB, patch
dao
: review+
Details | Diff | Splinter Review 
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)
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
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"?
Attached patch fix v1 (obsolete) — — Splinter Review 
(like so?  This fixes the data URI from comment 0 as well as the attached testcase.)
Attached patch fix v2 — — Splinter Review 
(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)
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]
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.
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...
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
I expect the failures were from the other two bugs, but just in case: https://tbpl.mozilla.org/?tree=Try&rev=e3b3f8792f5c
Depends on: 1061656
Blocks: 988672
Depends on: 1297918
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: