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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed, sec-want})

Trunk
x86
Mac OS X
dev-doc-needed, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Whiteboard: [sg:want P3][post-2.0][not-ready-for-cedar])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
See discussion in bug 604841.  Fixing this will reduce attack surface and fix bug 604262, bug 604556, bug 604807, bug 604841, and bug 605264.
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
(Reporter)

Updated

7 years ago
Whiteboard: [sg:want P3]
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.
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.
Not sure if this is blocking beta8, but the sooner the better.
blocking2.0: ? → beta8+
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).
I think I have this working.
Assignee: nobody → khuey
Blocks: 604262, 604807, 604841
Blocks: 605264
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.
Attachment #485584 - Flags: superreview?(jst)
Attachment #485584 - Flags: review?(jonas)
Keywords: dev-doc-needed
Hmm.  The extra word in all elements isn't all that great....  :(
Is the interface implementor thing just needed for nsIDOMNSEditableElement?  Or something else?
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 ...
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)?
(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.
Created attachment 485607 [details] [diff] [review]
Patch

Now with less words!
Attachment #485584 - Attachment is obsolete: true
Attachment #485607 - Flags: superreview?(jst)
Attachment #485607 - Flags: review?(jonas)
Attachment #485584 - Flags: superreview?(jst)
Attachment #485584 - Flags: review?(jonas)
Comment on attachment 485607 [details] [diff] [review]
Patch

I think peterv knows this stuff better.
Attachment #485607 - Flags: review?(jonas) → review?(peterv)
Does this need to block b7? Kyle was worried about API changes.
Jst said no over IRC earlier today.
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 on attachment 485607 [details] [diff] [review]
Patch

See comment 16.
Attachment #485607 - Flags: review?(peterv) → review-
Attachment #485607 - Flags: superreview?(jst)
(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.
(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.
Created attachment 492083 [details] [diff] [review]
Stop untrusted script from QIing to non-ClassInfo interfaces
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.
Attachment #492083 - Flags: feedback?(peterv)
Can an XPCNativeWrapper expose additional interfaces to those exposed by default on the underlying object, thereby allowing only chrome access to them?
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.
Attachment #492083 - Flags: feedback?(peterv)
Agreed, I don't think we can, or need, to deal with this for beta8.
blocking2.0: beta8+ → betaN+
This is too risky too late. We'll have to do it for next release instead.
blocking2.0: betaN+ → .x
Does that mean we need to fix the dependent bugs piecemeal for FF4?
Unfortunately yes
If those aren't assigned to me feel free to do so.
Thank you sir!

Updated

7 years ago
No longer blocks: 604807
Blocks: 604807
Blocks: 604556
(Reporter)

Updated

6 years ago
Whiteboard: [sg:want P3] → [sg:want P3][post-2.0]
Blocks: 610267
No longer blocks: 610267
Depends on: 610267
Whiteboard: [sg:want P3][post-2.0] → [sg:want P3][post-2.0][not-ready-for-cedar]
Think we're just going to wait for the new dom bindings here.
Assignee: khuey → nobody
Blocks: 691785
(Reporter)

Updated

6 years ago
Depends on: 580070
Keywords: sec-want
This isn't going to happen until we get to the brave new world of WebIDL bindings (and maybe not even then).
I believe it will happen then, ipso facto: the new bindings don't support random interfaces that aren't in the WebIDL.
Well, we still have to figure out how we're going to deal with XBL, but yes.
Depends on: 841442
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.
I declare this bug fixed.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
I know that we fixed this a while ago, but still, \o/
You need to log in before you can comment on or make changes to this bug.