339377 - "Bookmark this link" allows arbitrary code execution

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Testcase to follow. I think the microsummaries code is loading the URL being bookmarked, which may be a javascript: URL. This works on the 1.8 branch, but not on trunk, due to the "Bookmark this link" function being broken there.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Indeed, addBookmarks2.js's Startup() calls MicrosummaryPicker.init() which calls getMicrosummaries() with the link's URI, without passing it through a checkLoadURI.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

The easy fix is probably to make getMicrosummaries only load http(s) URIs.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: only load http|https pages from getMicrosummaries

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Wouldn't it be simpler to move this check into downloadPage? I don't know the microsummary code very well, but it seems to be the only place where arbitrary URLs could be problematic.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> Wouldn't it be simpler to move this check into downloadPage?

Yeah, or better yet into downloadHTMLPage and downloadXMLPage, since code sometimes calls those functions directly.  Looking into it.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: pushes check down into download*Page functions

This patch pushes the security check down into the download*Page functions, for more robust protection against these kinds of attacks.  I *think* we're ok not trapping this error for other callers of those functions.

Comment 8

[Martijn Wargers (dead)]

Isn't something like this possible?
const secMan = Components.classes["@mozilla.org/scriptsecuritymanager;1"]                        .getService(Components.interfaces.nsIScriptSecurityManager);
 const nsIScriptSecMan = Components.interfaces.nsIScriptSecurityManager;
secMan.checkLoadURI(pageURI, pageURI, nsIScriptSecMan.DISALLOW_SCRIPT_OR_DATA);
You only want to disable script and data url's, right?

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 223519 [details] [diff] [review]
patch v2: pushes check down into download*Page functions

please add a comment about why we're filtering here

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Fix checked in to trunk and branch.

(In reply to comment #8)
> Isn't something like this possible?
> const secMan = Components.classes["@mozilla.org/scriptsecuritymanager;1"]      
>                  .getService(Components.interfaces.nsIScriptSecurityManager);
>  const nsIScriptSecMan = Components.interfaces.nsIScriptSecurityManager;
> secMan.checkLoadURI(pageURI, pageURI, nsIScriptSecMan.DISALLOW_SCRIPT_OR_DATA);
> You only want to disable script and data url's, right?

Yes, we only want to disable script and data URLs, but it's not clear from the nsIScriptSecurityManager documentation how to use that interface to determine whether or not chrome should load a certain URL.  Should the "from" parameter to checkLoadURI be the URI itself or the URI of the chrome document?

And in this case, we don't even have a chrome document, since the microsummary service doing the load is a JavaScript component.  I'd be happy to use this interface if it was clear how to use it properly for this case.


(In reply to comment #9)
> (From update of attachment 223519 [details] [diff] [review] [edit])
> please add a comment about why we're filtering here

Ok, I added the following comment to both checks:

  // Make sure we're not loading javascript: or data: URLs, which could
  // take advantage of the load to run code with chrome: privileges.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060822 BonEcho/2.0b2. This is broken on the trunk due to bug 321973, but the patch landed there too.