[FIXr]XPCOM interface spoofing by using XBL <implementation>

RESOLVED FIXED in mozilla1.8beta4



12 years ago
11 years ago


(Reporter: moz_bug_r_a4, Assigned: bz)


({fixed-aviary1.0.7, fixed1.7.12, fixed1.8})

fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b3 -
blocking1.8b5 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [sg:fix])


(4 attachments)



12 years ago
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="http://www.mozilla.org/xbl">
      <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

12 years ago
Created attachment 188082 [details]
testcase 1 - |instanceof| tests

Comment 2

12 years ago
Created attachment 188083 [details]
testcase 2 - Making C++ call content-defined functions

Comment 3

12 years ago
attachment 188088 [details] in Bug 299520 comment 2
A exploit testcase that is using XBL <implementation>.
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?

Comment 4

12 years ago
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.
Assignee: dveditz → jst
Ever confirmed: true
Flags: blocking1.8b4? → blocking1.8b4+
Whiteboard: [sg:fix]
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

12 years ago
> 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.
> 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

12 years ago
Any time the chrome wants to change the state of the content in any way for any
reason, say....

Comment 9

12 years ago
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

12 years ago
not going to make b3, let's get fixed and tested for b4
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
This is a branch problem as well.
Flags: blocking1.7.9?
Flags: blocking1.7.10+
Flags: blocking-aviary1.0.6+
Flags: blocking-aviary1.0.5?
Flags: blocking1.7.9?
Flags: blocking-aviary1.0.5?


12 years ago
Flags: blocking1.7.11+ → blocking1.7.12+
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

12 years ago
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,
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

12 years ago
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.
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

12 years ago
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.
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

12 years ago
> 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.
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

12 years ago
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.
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.
bz kindly agreed to take this bug, reassigning.
Assignee: jst → bzbarsky

Comment 24

12 years ago
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...
Attachment #193890 - Flags: superreview?(shaver)
Attachment #193890 - Flags: review?(jst)


12 years ago
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: XPCOM interface spoofing by using XBL <implementation> → [FIX]XPCOM interface spoofing by using XBL <implementation>
Target Milestone: --- → mozilla1.8beta4
Comment on attachment 193890 [details] [diff] [review]
Patch for now

Attachment #193890 - Flags: review?(jst) → review+
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. =)
Attachment #193890 - Flags: superreview?(shaver)
Attachment #193890 - Flags: superreview+
Attachment #193890 - Flags: approval1.8b4+

Comment 27

12 years ago
Fixed on trunk.  Will land on branch on Sunday, probably, if nothing comes up by
Summary: [FIX]XPCOM interface spoofing by using XBL <implementation> → [FIXr]XPCOM interface spoofing by using XBL <implementation>

Comment 28

12 years ago
Checked in on 1.8 branch.
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Comment 29

12 years ago
Filed bug 306397 on fixing this the "right" way and bug 306398 on the necessary
security manager changes.


12 years ago
Flags: blocking-aviary1.0.7?

Comment 30

12 years ago
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.
Flags: blocking1.7.12?
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

12 years ago
It looks like this patch applies to the 1.7 branch (with some fuzz) and should
work fine over there.

Comment 33

12 years ago
Created attachment 195802 [details] [diff] [review]
Patch against 1.7 branch


12 years ago
Attachment #195802 - Flags: approval1.7.12?
Attachment #195802 - Flags: approval-aviary1.0.7?


12 years ago
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
Flags: blocking1.7.12? → blocking1.7.12+
Checked attachment 195802 [details] [diff] [review] in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Keywords: fixed-aviary1.0.7, fixed1.7.12
Attachment #195802 - Flags: approval1.7.12?
Attachment #195802 - Flags: approval1.7.12+
Attachment #195802 - Flags: approval-aviary1.0.7?
Attachment #195802 - Flags: approval-aviary1.0.7+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+

Comment 35

12 years ago
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

12 years ago
Can we get final verification?

Comment 37

12 years ago
Firefox 1.0.7 20050915 on Windows XP and Linux pass testcase 1 and testcase 2.
Any Mac users around to test this?
Group: security


12 years ago
Flags: testcase+


11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.