Last Comment Bug 299518 - [FIXr]XPCOM interface spoofing by using XBL <implementation>
: [FIXr]XPCOM interface spoofing by using XBL <implementation>
: fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta4
Assigned To: Boris Zbarsky [:bz]
Depends on:
  Show dependency treegraph
Reported: 2005-07-03 01:34 PDT by moz_bug_r_a4
Modified: 2007-04-01 15:12 PDT (History)
15 users (show)
dbaron: blocking1.7.12+
mtschrep: blocking‑aviary1.0.7+
cbeard: blocking1.8b3-
dveditz: blocking1.8b5+
cbeard: blocking‑aviary1.5+
bob: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase 1 - |instanceof| tests (1.37 KB, application/xml)
2005-07-03 01:43 PDT, moz_bug_r_a4
no flags Details
testcase 2 - Making C++ call content-defined functions (4.23 KB, application/xml)
2005-07-03 01:45 PDT, moz_bug_r_a4
no flags Details
Patch for now (2.45 KB, patch)
2005-08-25 21:00 PDT, Boris Zbarsky [:bz]
jst: review+
shaver: superreview+
shaver: approval1.8b4+
Details | Diff | Splinter Review
Patch against 1.7 branch (1.87 KB, patch)
2005-09-12 15:12 PDT, Boris Zbarsky [:bz]
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2005-07-03 01:34:10 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050702 Firefox/1.0+

|node instanceof Components.interfaces.*| tests can be deceived by XBL using 
|implements| attribute.

    <bindings xmlns="">
      <binding id="x">
        <implementation implements="nsIImageLoadingContent"/>

    <i id="i" style="-moz-binding:url(#x)"/>

    i instanceof Components.interfaces.nsIImageLoadingContent is |true|.
    i.QueryInterface(Components.interfaces.nsIImageLoadingContent) succeeds.

This trick can be combined with other bug to execute arbitrary code.

And, content can make C++ call content-defined methods of bogus XPCOM 
interfaces. (I'm not sure if this is exploitable.)

For example:

1931 // static
1932 PRBool
1933 nsContentUtils::IsDraggableImage(nsIContent* aContent)
1934 {
1935   NS_PRECONDITION(aContent, "Must have content node to test");
1937   nsCOMPtr<nsIImageLoadingContent> imageContent(do_QueryInterface(aContent));
1938   if (!imageContent) {
1939     return PR_FALSE;
1940   }
1942   nsCOMPtr<imgIRequest> imgRequest;
1943   imageContent->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
1944                            getter_AddRefs(imgRequest));
1946   // XXXbz It may be draggable even if the request resulted in an error.  Why?
1947   // Not sure; that's what the old nsContentAreaDragDrop/nsFrame code did.
1948   return imgRequest != nsnull;
1949 }

The above code can call the following bogus getRequest() function.

        <implementation implements="nsIImageLoadingContent, imgIRequest">
          <method name="getRequest">
              return this;

Reproducible: Always

Steps to Reproduce:
Comment 1 moz_bug_r_a4 2005-07-03 01:43:15 PDT
Created attachment 188082 [details]
testcase 1 - |instanceof| tests
Comment 2 moz_bug_r_a4 2005-07-03 01:45:28 PDT
Created attachment 188083 [details]
testcase 2 - Making C++ call content-defined functions
Comment 3 moz_bug_r_a4 2005-07-03 02:32:39 PDT
attachment 188088 [details] in Bug 299520 comment 2
A exploit testcase that is using XBL <implementation>.
Comment 4 moz_bug_r_a4 2005-07-04 00:41:56 PDT
attachment 188175 [details] in Bug 299520 comment 6
new exploit testcase that is using XBL <implementation>.

188088 doesn't work on Deer Park a1. 188175 works on both Deer Park a1 and 
Firefox/1.0+ 2005-07-03-06.
Comment 5 Benjamin Smedberg [:bsmedberg] 2005-07-05 11:25:10 PDT
Are we calling instanceof on the xpcnativewrapper? I'm presuming the bug here is
that the xpcnativewrapper should not reflect interfaces from xbl:implements?
Comment 6 Boris Zbarsky [:bz] 2005-07-05 11:29:55 PDT
> Are we calling instanceof on the xpcnativewrapper?

Yes.  All instanceof does is QI, of course.

The problem is that if content uses XUL defined via xul.css an chrome bindings,
we _do_ want to see those interfaces... the bug is thus that our security model
for this case is rather ill-defined and that we need to define it.
Comment 7 Benjamin Smedberg [:bsmedberg] 2005-07-06 08:17:07 PDT
> The problem is that if content uses XUL defined via xul.css an chrome bindings,
> we _do_ want to see those interfaces...

Is that true? In what cases is it necessary for our chrome code to see content
methods bound with XBL?
Comment 8 Boris Zbarsky [:bz] 2005-07-06 10:00:11 PDT
Any time the chrome wants to change the state of the content in any way for any
reason, say....
Comment 9 Boris Zbarsky [:bz] 2005-07-06 19:08:23 PDT
bsmedberg and I talked about this a bit today.  One thought I had was to only
allow an XBL binding to implement XPCOM interfaces if the binding's principal
has UniversalXPConnect privileges (so a chrome binding or one from a signed jar,
etc).  One problem with this is there is no clear way to request those
privileges; perhaps using implements= in XBL should implicitly do so?  Would it
be safe to pose the request dialog from there (in the XBL content sink)?
Comment 10 Chris Beard 2005-07-07 12:06:54 PDT
not going to make b3, let's get fixed and tested for b4
Comment 11 Daniel Veditz [:dveditz] 2005-07-10 11:51:58 PDT
This is a branch problem as well.
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-11 11:20:42 PDT
Let's try posing the dialog, and see how far that gets us. bz: can you take a
swing at this, or do we need to find someone else?
Comment 13 Boris Zbarsky [:bz] 2005-08-14 19:23:57 PDT
Posing the dialog at an "unsafe" point will lead to crashes.  I'm pretty sure
that inside the content sink is not safe.  So I'm not very happy with any patch
that just poses a dialog from the content sink.

Ideally we'd post the dialog async and delay binding instantiation until that's
done with, but I suspect that chrome's little "everything is sync" mind would
get blown by this.

If we have a way to check for privs and request them only if they're not already
there, we might be able to make chrome happy and keep the rest of XBL async as
it already is.

All that said, I have no idea when I'll be able to take a stab at this.  I have
all sorts of other stuff that's gotten dumped on me in the last two weeks,
Comment 14 Benjamin Smedberg [:bsmedberg] 2005-08-15 06:31:36 PDT
I think we should just disallow untrusted XBL from implementing any interface: I
don't understand what we have to lose in this case except perhaps some
accessibility interfaces for remote XUL (which never worked anyway).
Comment 15 Boris Zbarsky [:bz] 2005-08-15 07:13:25 PDT
We lose the ability to have remote XUL apps implement custom widgets.

More to the point, if something has UniversalXPConnect it IS trusted.  That's
the whole point of the enablePrivilege setup -- it allows users to allow a
specific site to bypass out security checks.  We keep getting bug reports every
so often on code that checks URIs or something without checking privileges and
breaks apps as a result.  I'd rather not add more such code.
Comment 16 Benjamin Smedberg [:bsmedberg] 2005-08-15 09:24:42 PDT
Which custom widgets? Other than the accessibility interfaces (which don't work
in remote XUL anyway) what interfaces are remote widgets required to implement
to get the functionality they need?
Comment 17 Boris Zbarsky [:bz] 2005-08-15 13:19:21 PDT
nsIImageLoadingContent comes to mind offhand; I can see a widget that
xbl:extends html:image needing that.  In general, any interface we create; the
ability to implement them in XBL is an important part of our platform story.
Comment 18 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-15 13:44:49 PDT
While I agree that we need to let non-chrome content implement interfaces in the
future, I don't yet see a path forward for Gecko 1.8 that lets us do that
without requiring more change than we have time for, or leaving us with this
significant security problem.

Can we Just Say No if the binding's principal doesn't have the right privilege,
for 1.8, and get a design for this problem space worked out for solving in 1.9?
Comment 19 Boris Zbarsky [:bz] 2005-08-15 14:18:03 PDT
> Can we Just Say No if the binding's principal doesn't have the right privilege,

I'm not sure we can ask for that information given our current APIs; hence my
ccing the security owners in the hope that they can shed light on this bit.  The
basic problem is that there's no way for a binding to _enable_ said privilege,
so just checking is not useful.
Comment 20 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-15 15:07:45 PDT
Just checking is useful if it lets us distinguish chrome from non-chrome,
without adding another URL-scheme check.  If and when we grow the ability to
request that a given binding have UniversalXPConnect, then people can use that
mechanism to gain the ability to implement interfaces from non-chrome privileged
Comment 21 Boris Zbarsky [:bz] 2005-08-15 15:11:27 PDT
All I'm saying is that we should treat the attempt to use implements="" in a
binding that doesn't have the privilege already as such a request.
Comment 22 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-15 15:17:27 PDT
All _I_'m saying is that doing so is not necessary to fix the security issues in
this bug, and that we may want to live without that implied privilege request in
gecko 1.8, in the interests of shipping it a) in finite time, and b) without
this significant security problem.
Comment 23 Johnny Stenback (:jst, 2005-08-24 22:33:47 PDT
bz kindly agreed to take this bug, reassigning.
Comment 24 Boris Zbarsky [:bz] 2005-08-25 21:00:50 PDT
Created attachment 193890 [details] [diff] [review]
Patch for now

There's no way to make this work the way I'd like to without some heavy lifting
in the security manager, so let's just do the simple thing for now.  Dammit, I
hate how shaver's always right.  ;)

I've made sure that this fixes the two testcases here and doesn't break our own
UI (and I _did_ test that simply disabling xbl:implements breaks our UI quite
nicely, so it not being broken is an indication that things are working right).

Once we have this fixed, I'll file a followup bug on what I want to happen here
with bugs on security manager blocking it...
Comment 25 Johnny Stenback (:jst, 2005-08-25 22:35:16 PDT
Comment on attachment 193890 [details] [diff] [review]
Patch for now

Comment 26 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-26 06:54:25 PDT
Comment on attachment 193890 [details] [diff] [review]
Patch for now

sr+a=shaver.  I'm flattered!

We might want to bake this over the weekend on the trunk before putting it on
the branch, though I have atypically high confidence in a double landing due to
the provenance of this patch. =)
Comment 27 Boris Zbarsky [:bz] 2005-08-26 10:12:59 PDT
Fixed on trunk.  Will land on branch on Sunday, probably, if nothing comes up by
Comment 28 Boris Zbarsky [:bz] 2005-08-29 20:51:34 PDT
Checked in on 1.8 branch.
Comment 29 Boris Zbarsky [:bz] 2005-08-29 21:25:12 PDT
Filed bug 306397 on fixing this the "right" way and bug 306398 on the necessary
security manager changes.
Comment 30 Boris Zbarsky [:bz] 2005-09-12 11:02:49 PDT
If we need this on the 1.7 branch let me know and I'll try to figure out whether
the same code could work.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-12 14:45:30 PDT
Could you look into whether this applies to the branch and also give us some
information as to how risky it would be?
Comment 32 Boris Zbarsky [:bz] 2005-09-12 14:56:03 PDT
It looks like this patch applies to the 1.7 branch (with some fuzz) and should
work fine over there.
Comment 33 Boris Zbarsky [:bz] 2005-09-12 15:12:20 PDT
Created attachment 195802 [details] [diff] [review]
Patch against 1.7 branch
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-12 17:28:32 PDT
Checked attachment 195802 [details] [diff] [review] in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Comment 35 Bob Clary [:bc:] 2005-09-14 16:26:16 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716

testcase 1: all true, failed.
testcase 2: can drag target with messages, failed.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050914


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050914 Firefox/1.4

testcase 1: all false, passed
testcase 2: can not drag target, no message, passed
Comment 36 Mike Schroepfer 2005-09-19 18:34:51 PDT
Can we get final verification?
Comment 37 Bob Clary [:bc:] 2005-09-19 19:44:36 PDT
Firefox 1.0.7 20050915 on Windows XP and Linux pass testcase 1 and testcase 2.
Any Mac users around to test this?

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