Closed Bug 339377 Opened 16 years ago Closed 16 years ago

"Bookmark this link" allows arbitrary code execution

Categories

(Firefox :: Bookmarks & History, defect)

2.0 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 2 beta1

People

(Reporter: Gavin, Assigned: myk)

References

Details

(Keywords: verified1.8.1, Whiteboard: [sg:moderate] FF2 feature, not 1.8.0.x)

Attachments

(2 files, 1 obsolete file)

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.
Attached file testcase
Flags: blocking-firefox2?
Indeed, addBookmarks2.js's Startup() calls MicrosummaryPicker.init() which calls getMicrosummaries() with the link's URI, without passing it through a checkLoadURI.
The easy fix is probably to make getMicrosummaries only load http(s) URIs.
Attachment #223511 - Flags: review?(mconnor)
Attachment #223511 - Flags: approval-branch-1.8.1?(mconnor)
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.
(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.
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.
Assignee: nobody → myk
Attachment #223511 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #223511 - Flags: review?(mconnor)
Attachment #223511 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #223519 - Flags: review?(mconnor)
Attachment #223519 - Flags: approval-branch-1.8.1?(mconnor)
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 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
Attachment #223519 - Flags: review?(mconnor)
Attachment #223519 - Flags: review+
Attachment #223519 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #223519 - Flags: approval-branch-1.8.1+
Flags: blocking1.9a1+
Flags: blocking-firefox2?
Flags: blocking-firefox2+
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
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.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:moderate] FF2 feature, not 1.8.0.x
Group: security
You need to log in before you can comment on or make changes to this bug.