Last Comment Bug 503926 - Ignore QueryInterface exposed by non-chrome-privilege content
: Ignore QueryInterface exposed by non-chrome-privilege content
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on: 506249 509824 1002313
Blocks: 503700 CVE-2010-0179
  Show dependency treegraph
 
Reported: 2009-07-13 13:04 PDT by Jesse Ruderman
Modified: 2014-04-27 20:52 PDT (History)
12 users (show)
benjamin: blocking1.9.2-
benjamin: wanted1.9.2+
mbeltzner: blocking1.9.0.19-
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.8+
.8-fixed


Attachments
Proposed fix (1.19 KB, patch)
2009-07-16 16:02 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated to comments (6.59 KB, patch)
2009-07-22 17:51 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
jst: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review
1.8.0 patch (1.52 KB, patch)
2010-02-02 02:07 PST, Martin Stránský
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2009-07-13 13:04:00 PDT
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.
Comment 1 Nochum Sossonko [:Natch] 2009-07-13 13:06:38 PDT
instanceof will still work, I assume, which flattens interfaces. Does it rely on QI?
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-07-13 13:12:55 PDT
Nochum, that's unrelated, this is QI *implemented by* content script.
Comment 3 Blake Kaplan (:mrbkap) 2009-07-14 21:28:50 PDT
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...
Comment 4 Jesse Ruderman 2009-07-14 21:46:04 PDT
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.
Comment 5 Boris Zbarsky [:bz] 2009-07-14 22:55:10 PDT
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...
Comment 6 Blake Kaplan (:mrbkap) 2009-07-16 16:02:50 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2009-07-17 20:51:32 PDT
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.  :(
Comment 8 Boris Zbarsky [:bz] 2009-07-17 20:52:21 PDT
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...
Comment 9 Blake Kaplan (:mrbkap) 2009-07-18 15:32:23 PDT
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.
Comment 10 Blake Kaplan (:mrbkap) 2009-07-22 17:51:14 PDT
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.
Comment 11 Boris Zbarsky [:bz] 2009-07-22 19:16:09 PDT
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...
Comment 12 Boris Zbarsky [:bz] 2009-07-22 19:18:00 PDT
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.
Comment 13 Blake Kaplan (:mrbkap) 2009-07-22 23:49:45 PDT
(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.
Comment 14 Boris Zbarsky [:bz] 2009-07-23 06:34:18 PDT
> Would you mind if the first test is an iframe instead of a window?

With type="content"?  Not at all; sounds great.
Comment 15 Blake Kaplan (:mrbkap) 2009-07-23 16:06:59 PDT
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.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-10 15:37:30 PST
This is needed for 1.9.1 to fix blocker bug 504021.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-10 16:36:06 PST
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

This applies cleanly to 1.9.1 and passes mochitest locally.
Comment 18 Daniel Veditz [:dveditz] 2009-11-30 14:55:18 PST
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 19 Blake Kaplan (:mrbkap) 2010-01-19 15:40:37 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cba2e8706df4
Comment 20 Martin Stránský 2010-02-02 02:07:32 PST
Created attachment 424751 [details] [diff] [review]
1.8.0 patch

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