Last Comment Bug 605271 - Don't let scripts QI elements to non-classinfo interfaces
: Don't let scripts QI elements to non-classinfo interfaces
Status: RESOLVED FIXED
[sg:want P3][post-2.0][not-ready-for-...
: dev-doc-needed, sec-want
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
-- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: ParisBindings post2.0 841442
Blocks: 604556 604262 604807 604841 605264 691785
  Show dependency treegraph
 
Reported: 2010-10-18 14:29 PDT by Jesse Ruderman
Modified: 2013-12-03 19:04 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
Patch (22.81 KB, patch)
2010-10-24 06:52 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (20.01 KB, patch)
2010-10-24 12:19 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
peterv: review-
Details | Diff | Splinter Review
Stop untrusted script from QIing to non-ClassInfo interfaces (25.72 KB, patch)
2010-11-20 12:42 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review

Description User image Jesse Ruderman 2010-10-18 14:29:23 PDT
See discussion in bug 604841.  Fixing this will reduce attack surface and fix bug 604262, bug 604556, bug 604807, bug 604841, and bug 605264.
Comment 1 User image Peter Van der Beken [:peterv] 2010-10-18 14:33:20 PDT
The only problem here are interfaces exposed through XBL, we'll have to
whitelist them. I did have a patch for that at some point, trying to find it
again.
Comment 2 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-18 14:37:12 PDT
One possible way to fix this, which would probably be easy to code, but somewhat risky to deploy, is to simply deny access to Components.interfaces for untrusted callers.
Comment 3 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-18 14:38:46 PDT
Not sure if this is blocking beta8, but the sooner the better.
Comment 4 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-23 18:51:15 PDT
I have a patch for this that flips the ClassInfo flag on elements and special cases XBL.  The code changes are pretty small, but this breaks a lot of stuff that expects to QI to nsIDOMNSEditableElement (both tests and content XBL bindings like textbox.xml).
Comment 5 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-24 02:49:26 PDT
I think I have this working.
Comment 6 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-24 06:52:58 PDT
Created attachment 485584 [details] [diff] [review]
Patch

This patch disallows QIing to interfaces that are not in ClassInfo (or the ancestor of an interface in ClassInfo) for scripts that don't have UniversalXPConnect.  I'm not crazy about a couple things in this patch (particularly the new interface and the nested class in nsGenericElement) but I couldn't think of a good way to do this without an iface and I didn't want to chase ambiguous cast errors through ClassInfo and quickstubs.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-24 10:58:48 PDT
Hmm.  The extra word in all elements isn't all that great....  :(
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-24 10:59:17 PDT
Is the interface implementor thing just needed for nsIDOMNSEditableElement?  Or something else?
Comment 9 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-24 11:02:55 PDT
The extra word we can fix by fixing up all of the ambiguous casts that that creates.  The extra interface (nsIExtraInterfaceImplementor) I don't see a way to get rid of.  For XBL XPConnect needs to determine which interfaces that aren't in ClassInfo it should make visible, and it can't do that by QIing, so ...
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-24 11:25:16 PDT
Oh, right.  This patch actually adds two words to elements as things stand...

What are the ambiguous casts you speak of?  To nsISupports?

Is there a reason to not put nsIExtraInterfaceImplementor between nsIContent and Element?  Would that avoid the ambiguous casts (and incidentally adding that second word)?
Comment 11 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-24 11:40:47 PDT
(In reply to comment #10)
> Is there a reason to not put nsIExtraInterfaceImplementor between nsIContent
> and Element?  Would that avoid the ambiguous casts (and incidentally adding
> that second word)?

No; I believe so.  Will try.
Comment 12 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-24 12:19:19 PDT
Created attachment 485607 [details] [diff] [review]
Patch

Now with less words!
Comment 13 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-25 15:11:52 PDT
Comment on attachment 485607 [details] [diff] [review]
Patch

I think peterv knows this stuff better.
Comment 14 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-10-29 11:40:52 PDT
Does this need to block b7? Kyle was worried about API changes.
Comment 15 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-29 12:23:16 PDT
Jst said no over IRC earlier today.
Comment 16 User image Peter Van der Beken [:peterv] 2010-11-16 11:51:45 PST
I think we should add a flag to nsIXPCScriptable for this (we should be able to reuse '1 << 30') and add a function to nsXPCClassInfo, to avoid the additional interface and QI.

I also don't understand how this works given that we cache tearoffs. Won't this just mean that all that needs to happen is for chrome code to QI to one of these interfaces once, and then content can get its hands on the interface anyway? We should make this not cache (btw, that seemed like a much more difficult change when I'd looked at fixing this). That would also make dynamic XBL changes work properly by revoking an interface when a binding is removed.

There's also a change to move NS_FORWARD_NSISUPPORTS, but that seems totally unrelated to this bug.
Comment 17 User image Peter Van der Beken [:peterv] 2010-11-16 11:53:22 PST
Comment on attachment 485607 [details] [diff] [review]
Patch

See comment 16.
Comment 18 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-16 13:05:23 PST
(In reply to comment #16)
> I think we should add a flag to nsIXPCScriptable for this (we should be able to
> reuse '1 << 30') and add a function to nsXPCClassInfo, to avoid the additional
> interface and QI.

OK.

> I also don't understand how this works given that we cache tearoffs. Won't this
> just mean that all that needs to happen is for chrome code to QI to one of
> these interfaces once, and then content can get its hands on the interface
> anyway? We should make this not cache (btw, that seemed like a much more
> difficult change when I'd looked at fixing this). That would also make dynamic

I haven't looked at the code again yet, but we don't handle elements substantively different than other DOM objects do we?  It seems like that'd be a problem across the board then.  Of course, perhaps non-element DOM objects don't implement any interesting scriptable interfaces.

> XBL changes work properly by revoking an interface when a binding is removed.

Doesn't that violate XPCOM rules?  If I have an owning pointer to an XBL implemented interfaces from C++ (say in a11y) that pointer isn't allowed to be taken away from me, regardless of what happens at the XBL level.

> There's also a change to move NS_FORWARD_NSISUPPORTS, but that seems totally
> unrelated to this bug.

Yeah, that's bogus.
Comment 19 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-16 13:16:50 PST
(In reply to comment #18)
> (In reply to comment #16)
> > I also don't understand how this works given that we cache tearoffs. Won't this
> > just mean that all that needs to happen is for chrome code to QI to one of
> > these interfaces once, and then content can get its hands on the interface
> > anyway? We should make this not cache (btw, that seemed like a much more
> > difficult change when I'd looked at fixing this). That would also make dynamic
> I haven't looked at the code again yet, but we don't handle elements
> substantively different than other DOM objects do we?  It seems like that'd be
> a problem across the board then.  Of course, perhaps non-element DOM objects
> don't implement any interesting scriptable interfaces.

The answer to that is either the DOM object in question has CLASSINFO_INTERFACES_ONLY, in which case even chrome doesn't get to QI to the interface in question, or that DOM object has the same problem that we're trying to fix for elements here.
Comment 20 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-20 12:42:17 PST
Created attachment 492083 [details] [diff] [review]
Stop untrusted script from QIing to non-ClassInfo interfaces
Comment 21 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-20 12:52:48 PST
Comment on attachment 492083 [details] [diff] [review]
Stop untrusted script from QIing to non-ClassInfo interfaces

This mostly works, but it's starting to feel like a rabbithole.  This patch keeps track of whether or not a given XPCNativeInterface (and the corresponding tearoff) should be exposed to untrusted script.  If the interface isn't safe to be exposed to untrusted script we refuse to hand back a tearoff.

Fixing up instanceof is easy enough, but this gets messy when we have to deal with properties on the JSObject.  Right now nsElementSH hooks GetProperty and decides whether or not to allow the access through.  That's inefficent, but it works.  Setting the property is more complicated.  I'm not sure what should happen in the scenario:

enablePrivilege('UniversalXPConnect');
foo.QI(non-ClassInfo-iface);
revertPrivilege('UniversalXPConnect');
foo.<method-defined-on-said-iface> = bar; // What does this do?

I don't see any good way to solve that; either we forbid the property set and so random properties become inaccessible, or we let untrusted script clobber the IDL properties.

I wonder if we're not better off pursuing the Components.interfaces method that Jonas proposed.
Comment 22 User image neil@parkwaycc.co.uk 2010-11-22 02:06:28 PST
Can an XPCNativeWrapper expose additional interfaces to those exposed by default on the underlying object, thereby allowing only chrome access to them?
Comment 23 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-28 18:37:34 PST
Comment on attachment 492083 [details] [diff] [review]
Stop untrusted script from QIing to non-ClassInfo interfaces

I think this is going to be nasty no matter what we do.  Even if we do Jonas's suggestion we still have to deal with the same wrapper issues.  I think we should do one of two things:

1) Identify which interfaces are relevant for extensions and other js to be able to QI to and add them to classinfo.  Audit these to ensure that they're safe for exposure to content.  There shouldn't be too many of these. Then flip the classinfo flags.  It might be too late to do this for 2.0, in which case we should ...
2) Unblock on this and fix the dependent bugs in an ad-hoc fashion.

Either way we are probably going to have to bump this from b8.
Comment 24 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-11-29 15:10:55 PST
Agreed, I don't think we can, or need, to deal with this for beta8.
Comment 25 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-12-14 10:35:18 PST
This is too risky too late. We'll have to do it for next release instead.
Comment 26 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-12-14 10:53:29 PST
Does that mean we need to fix the dependent bugs piecemeal for FF4?
Comment 27 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-12-14 13:03:19 PST
Unfortunately yes
Comment 28 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-12-14 16:45:20 PST
If those aren't assigned to me feel free to do so.
Comment 29 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-12-14 16:54:23 PST
Thank you sir!
Comment 30 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-29 09:43:42 PDT
Think we're just going to wait for the new dom bindings here.
Comment 31 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-06 09:47:04 PDT
This isn't going to happen until we get to the brave new world of WebIDL bindings (and maybe not even then).
Comment 32 User image Boris Zbarsky [:bz] (still a bit busy) 2012-08-06 10:10:15 PDT
I believe it will happen then, ipso facto: the new bindings don't support random interfaces that aren't in the WebIDL.
Comment 33 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-06 10:16:46 PDT
Well, we still have to figure out how we're going to deal with XBL, but yes.
Comment 34 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-19 12:13:07 PDT
This is all done now, since we're on WebIDL bindings, no?  QI just does a QI on the c++ side and then returns the same JSObject you already had.
Comment 35 User image Boris Zbarsky [:bz] (still a bit busy) 2013-12-03 19:01:24 PST
I declare this bug fixed.
Comment 36 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-12-03 19:04:14 PST
I know that we fixed this a while ago, but still, \o/

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