291651 - Address bar accepts dragged javascript urls

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Paul Nickerson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

By dragging a javascript url to the address bar of another firefox window, we
can execute script in the context of any site

Reproducible: Always

Steps to Reproduce:
(see http://greyhatsecurity.org/vulntests/more/jscriptgo.htm)
Actual Results:  
cross site scripting

Expected Results:  
Javascript urls should not be draggable.

Comment 1

[timeless]

i rely on being able to do this

Comment 2

[Jesse Ruderman]

I don't rely on being able to do this.  Timeless, can you elaborate?

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to comment #2)
> Timeless, can you elaborate?

In particular, why isn't cut-n-paste (or "Copy Link Location" and paste) good
enough? Yes, it will be slightly more effort for an action that no normal user
would ever do (bookmarklets are leading-edge normal, but are bookmarked and not
dragged).

For some reason the linked test case isn't working for me in 1.0.3 -- maybe one
of my extensions or non-standard pref settings is interfering. The "this link"
bit is appended to the javascript: url giving a syntax error. Doesn't take much
to fix it though.

This doesn't seem to work in the Suite -- when you drag to the urlbar it's
populated but not automatically executed. The user would have to see the odd
javascript: url and think it's a good idea to execute it. Some would, probably,
but that much less a problem.

Dragging to the 'go' button is equivalent, and avoids any problems with dragging
into the middle of the url bar ending up inserting text (sometimes--why the
inconsistency?) rather than replacing it.

Comment 4

[Daniel Veditz [:dveditz]]

Ben, mconnor: how do you want to handle this? One approach would be to make
Firefox behave like the suite and just paste, not paste and submit. Or only
submit if it's an http(s) url.

The other approach is to block drops of javascript: and data: urls entirely (and
do so using CheckLoadURI() so we don't have reopens like bug 290949)

Comment 5

[Mike Connor [:mconnor]]

If the paste behaviour is considered sufficient, lets do that, and disable
entirely on the Go button, to match the behaviour implemented for tabs in Bug
280056.

I can write the patch, or you can write and I'll review, your call. :)

Comment 6

[Bob Clary [:bc] (inactive)]

The URL is 404. Does anyone have a copy of the testcase they can send me?

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

mconnor: can you write the patch you mention in comment 5?

Comment 8

[Mike Connor [:mconnor]]

Attachment: fix

This makes drag and drop to the URL bar or the Go button behave as follows:

non-URIs and data/javascript URIs will paste on drop in the URL bar, but will
not be executed.  Dropping on the Go button will have no effect.
valid non-data/javascript URIs will continue to function as before.

This is the minimum amount of real change, but it seems adequate to the task at
hand.

Comment 9

[Jesse Ruderman]

Dragged chrome:// URLs should also be blocked or treated like dragged
javascript: URLs.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 192801 [details] [diff] [review]
fix

> 
>         // XXXBlake Workaround caret crash when you try to set the textbox's value on dropping
>-        setTimeout(function(u) { gURLBar.value = u; handleURLBarCommand(); }, 0, url);
>+        function urlbarDrop(u) {
>+          try {
>+            gURLBar.value = u;
>+            var uri = makeURI(gURLBar.value);
>+            const secMan = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                                     .getService(Components.interfaces.nsIScriptSecurityManager);
>+            const nsIScriptSecMan = Components.interfaces.nsIScriptSecurityManager;
>+            secMan.checkLoadURI(gBrowser.currentURI, uri, nsIScriptSecMan.DISALLOW_SCRIPT_OR_DATA);
>+            handleURLBarCommand();
>+          } catch (ex) {}
>+        }
>+        setTimeout(urlbarDrop, 0, url);

Leave the XXXBlake comment right above the setTimeout call, I think.  And
please
make sure that there's a bug on file for the crash case, and mention it there
as
well.

>+      var url = xferData[0] ? xferData[0] : xferData[1];
>+      if (url) {
>         getBrowser().dragDropSecurityCheck(aEvent, aDragSession, uri);
>-
>-        loadURI(uri, null, null);
>+        try {
>+          var uri = makeURI(url);
>+          const secMan = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                                   .getService(Components.interfaces.nsIScriptSecurityManager);
>+          const nsIScriptSecMan = Components.interfaces.nsIScriptSecurityManager;
>+          secMan.checkLoadURI(gBrowser.currentURI, uri, nsIScriptSecMan.DISALLOW_SCRIPT_OR_DATA);
>+          loadURI(uri, null, null);
>+        } catch (ex) {}
>       }

Lose the if (url) test and just put that whole chunk into the try.

Comment 11

[Mike Connor [:mconnor]]

(In reply to comment #9)
> Dragged chrome:// URLs should also be blocked or treated like dragged
> javascript: URLs.

As far as I can tell, they already are, even without this patch.

Comment 12

[Jesse Ruderman]

> + secMan.checkLoadURI(gBrowser.currentURI, uri,
nsIScriptSecMan.DISALLOW_SCRIPT_OR_DATA);

gBrowser.currentURI will be wrong if you drag from one window to another, or if
you switch tabs while dragging.

Comment 13

[Mike Connor [:mconnor]]

Attachment: address comments, remove timeout

turns out, whatever crash Blake was hitting way back in 2002 is gone now.

Comment 14

[Mike Connor [:mconnor]]

Fixed, branch and trunk.

Comment 15

[Bob Clary [:bc] (inactive)]

fyi, fails in firefox 1.0.8/winxp/20060221 (and yes I know it is marked as only fixed on 1.8/trunk).