605271 - Don't let scripts QI elements to non-classinfo interfaces

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

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

[Peter Van der Beken [:peterv]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure if this is blocking beta8, but the sooner the better.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think I have this working.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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

[Boris Zbarsky [:bzbarsky]]

Hmm.  The extra word in all elements isn't all that great....  :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

Is the interface implementor thing just needed for nsIDOMNSEditableElement?  Or something else?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Now with less words!

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 485607 [details] [diff] [review]
Patch

I think peterv knows this stuff better.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this need to block b7? Kyle was worried about API changes.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Jst said no over IRC earlier today.

Comment 16

[Peter Van der Beken [:peterv]]

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

[Peter Van der Beken [:peterv]]

Comment on attachment 485607 [details] [diff] [review]
Patch

See comment 16.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Stop untrusted script from QIing to non-ClassInfo interfaces

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[[e-mail]]

Can an XPCNativeWrapper expose additional interfaces to those exposed by default on the underlying object, thereby allowing only chrome access to them?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[Johnny Stenback (:jst)]

Agreed, I don't think we can, or need, to deal with this for beta8.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is too risky too late. We'll have to do it for next release instead.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does that mean we need to fix the dependent bugs piecemeal for FF4?

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unfortunately yes

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If those aren't assigned to me feel free to do so.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thank you sir!

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Think we're just going to wait for the new dom bindings here.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This isn't going to happen until we get to the brave new world of WebIDL bindings (and maybe not even then).

Comment 32

[Boris Zbarsky [:bzbarsky]]

I believe it will happen then, ipso facto: the new bindings don't support random interfaces that aren't in the WebIDL.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well, we still have to figure out how we're going to deal with XBL, but yes.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

I declare this bug fixed.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I know that we fixed this a while ago, but still, \o/