Closed Bug 299518 Opened 19 years ago Closed 19 years ago

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

Categories

(Core :: Security, defect, P1)

Product:
Core
Component:
Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: moz_bug_r_a4, Assigned: bzbarsky)

Details

(Keywords: fixed-aviary1.0.7, fixed1.7.12, fixed1.8, Whiteboard: [sg:fix])

Attachments

(4 files)

testcase 1 - |instanceof| tests
1.37 KB, application/xml
Details
testcase 2 - Making C++ call content-defined functions
4.23 KB, application/xml
Details
Patch for now
2.45 KB, patch
jst
: review+
shaver
: superreview+
shaver
: approval1.8b4+
Details | Diff | Splinter Review
Patch against 1.7 branch
1.87 KB, patch
dbaron
: approval-aviary1.0.7+
dbaron
: approval1.7.12+
Details | Diff | Splinter Review 
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"/>
      </binding>
    </bindings>

    <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");
1936 
1937   nsCOMPtr<nsIImageLoadingContent> imageContent(do_QueryInterface(aContent));
1938   if (!imageContent) {
1939     return PR_FALSE;
1940   }
1941 
1942   nsCOMPtr<imgIRequest> imgRequest;
1943   imageContent->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
1944                            getter_AddRefs(imgRequest));
1945 
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.

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


Reproducible: Always

Steps to Reproduce:

    
    

    
  
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?

    
    

    
  
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
Status: UNCONFIRMED → NEW
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?

    
    

    
  
> 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?

    
    

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

    
    

    
  
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)?

    
    

    
  
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?

    
  
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?

    
    

    
  
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,
unfortunately.

    
    

    
  
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).

    
    

    
  
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?

    
    

    
  
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?

    
    

    
  
> 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
XUL.

    
    

    
  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for now
        — 
          — Splinter Review
      

    


  
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)

    
  
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

r=jst

        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+

    
    

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

    
    

    
  
Checked in on 1.8 branch.
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

    
    

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

    
  
Flags: blocking-aviary1.0.7?

    
    

    
  
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.

    
    

    
  
Could you look into whether this applies to the branch and also give us some
information as to how risky it would be?

    
    

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

    
  

        Attachment #195802 -
        Flags: approval1.7.12?

        Attachment #195802 -
        Flags: approval-aviary1.0.7?

    
  
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+

    
    

    
  
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716
Firefox/1.0.6

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
Firefox/1.0.7

and 

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


    
    

    
  
Can we get final verification?

    
    

    
  
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
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: