Ignore QueryInterface exposed by non-chrome-privilege content

RESOLVED FIXED

Status

()

Core
XPConnect
--
enhancement
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
blocking1.9.0.19 -
wanted1.9.0.x +

Firefox Tracking Flags

(blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Can we make XPConnect ignore QueryInterface in content JS objects, as suggested by roc in bug 503699 comment 4?

This would be a general fix for bugs where QI is weird (bug 503644, bug 503697) or has side effects (bug 503699).    Programmers expect QI to have no side-effects apart from perhaps creating a tearoff.  It's hard to reason about code where QI can have arbitrary side effects.
Flags: blocking1.9.2?
instanceof will still work, I assume, which flattens interfaces. Does it rely on QI?
Nochum, that's unrelated, this is QI *implemented by* content script.
(Assignee)

Comment 3

8 years ago
So, in that case content objects would trivially support all requested interfaces? Or trivially *not* support any interfaces? Either way, it's easy to do this. I do note that if we allow them to implement all interfaces, then any code touching a maybe-content-JS-C++-thingi will still have to be careful for misbehavior...
(Reporter)

Comment 4

8 years ago
Ideally, XPConnect would behave as if content hadn't mucked with QI.  For example, document.body should be able to QI to nsIDOMNode but not nsIDOMSVGAnimationElement.  Is that significantly harder than claiming to support no interfaces?

Claiming to support all interfaces sounds scary, I don't know why we'd want to do that.
For XPCWrappedNatives, I'd be perfectly happy if QI did absolutely nothing magic.

The issue is vanilla JSObjects (XPCWrappedJS); those need to be able to QI to the interface they were created for (esp. nsIDOMEventListener).

I think content objects should claim support for as few interfaces as we can get away with...
(Assignee)

Comment 6

8 years ago
Created attachment 389035 [details] [diff] [review]
Proposed fix

Before I check this in, I need to go through and make sure that we actually make all components' global objects STOBJ_IS_SYSTEM. Otherwise, this should be good to go.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #389035 - Flags: superreview?(jst)
Attachment #389035 - Flags: review?(bzbarsky)
Hmm.  Could this cause issues for cases when people load chrome documents in content docshells?  In those cases the global is not system, right? Not that they should be doing it, but.... I'm looking at about:config here, say.  :(
Note that I'm all in favor of causing problems for such (ab)users.  Maybe it'll finally make the tabbrowser folks get off their butts and fix things...
(Assignee)

Updated

8 years ago
Attachment #389035 - Attachment is obsolete: true
Attachment #389035 - Flags: superreview?(jst)
Attachment #389035 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

8 years ago
Comment on attachment 389035 [details] [diff] [review]
Proposed fix

In that case, I'll call GetObjectPrincipal before bailing. Hopefully, that'll let chrome code run without any sort of bad perf hit and still let the case bz points out in comment 7 still work.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(Assignee)

Comment 10

8 years ago
Created attachment 390127 [details] [diff] [review]
Updated to comments

The chrome mochitests test both the STOBJ_IS_SYSTEM codepath and the codepath bz mentioned in comment 7.
Attachment #390127 - Flags: superreview?(jst)
Attachment #390127 - Flags: review?(bzbarsky)
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

r=me, but I'd rather the test explicitly opened both its windows, so we don't depend on this "chrome tests run in a content docshell" thing that I think is a bug...
Attachment #390127 - Flags: review?(bzbarsky) → review+
Note also that UniversalXPConnect code will still fail here; do we want to be testing for that instead of system principal?  Might not be worth worrying about.
(Assignee)

Comment 13

8 years ago
(In reply to comment #11)
> r=me, but I'd rather the test explicitly opened both its windows, so we don't

Would you mind if the first test is an iframe instead of a window?

(In reply to comment #12)
> Note also that UniversalXPConnect code will still fail here; do we want to be
> testing for that instead of system principal?  Might not be worth worrying
> about.

There's no way to do that test given our current setup. I'm also not sure what it would mean: |new function() { enableprivilege("UniversalXPConnect") }|? |enableprivielege("UniversalXPConnect"); new Object()|? Either way, by the time this code runs, it's likely that the privilege isn't enabled any more.
> Would you mind if the first test is an iframe instead of a window?

With type="content"?  Not at all; sounds great.

Updated

8 years ago
Attachment #390127 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/94fc37d58985 ... using the same file for both the dialog and the frame was a little ugly, but not too bad.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 506249
Depends on: 509824
This is needed for 1.9.1 to fix blocker bug 504021.
status1.9.1: --- → ?

Updated

8 years ago
Blocks: 504021
blocking1.9.1: --- → ?
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

This applies cleanly to 1.9.1 and passes mochitest locally.
Attachment #390127 - Flags: approval1.9.1.6?
blocking1.9.1: ? → .7+
status1.9.1: ? → wanted
Attachment #390127 - Flags: approval1.9.1.6? → approval1.9.1.7?
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #390127 - Flags: approval1.9.1.7? → approval1.9.1.7+
Whiteboard: [needs 1.9.1 landing]
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cba2e8706df4
status1.9.1: wanted → .8-fixed
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.18+
Flags: blocking1.9.0.18+ → blocking1.9.0.19?

Comment 20

7 years ago
Created attachment 424751 [details] [diff] [review]
1.8.0 patch
Flags: blocking1.9.0.19? → blocking1.9.0.19-
(Reporter)

Updated

7 years ago
Whiteboard: [needs 1.9.1 landing]
Depends on: 1002313
You need to log in before you can comment on or make changes to this bug.