Closed Bug 472648 Opened 16 years ago Closed 16 years ago

[FIX]XBL not working in signed jar

Categories

(Core :: XBL, defect, P1)

Product:
Core
Component:
XBL
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mva, Assigned: bzbarsky)

References

Details

(4 keywords, Whiteboard: [sg:low])

Attachments

(10 files, 2 obsolete files)

test xul document
300 bytes, application/vnd.mozilla.xul+xml
Details
test css document
64 bytes, text/css
Details
test xbl document
912 bytes, text/xml
Details
CA
805 bytes, application/x-x509-ca-cert
Details
test signed jar with mc.cacert
2.67 KB, application/java-archive
Details
Proposed fix
9.58 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
1.9.0 branch patch with -u
7.61 KB, patch
dveditz
: approval1.9.0.7+
Details | Diff | Splinter Review
1.9.1 patch without async forcing
7.76 KB, patch
Details | Diff | Splinter Review
m-c patch without async forcing
8.05 KB, patch
Details | Diff | Splinter Review
1.8 branch fix
7.95 KB, patch
jst
: review+
jst
: superreview+
dveditz
: approval1.8.1.next+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5

When using XBL in a signed jar through a css in a xul document, the
javascript in the bindings are not executed nor attached to the bound elements.


Reproducible: Always

Steps to Reproduce:
1.create a xul file with a css reference and some xml element to bind using xbl
2. create the css file with a -moz-binding to bind the xml element
3. create an xbl binding with some javascript
4. package all these files in a jar, and sign the jar
5. deploy the jar on a web server
6. access the xul document
Actual Results:  
The javascript of the xbl is not attached and not executed.


example files:

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<?xml-stylesheet href="test.css" type="text/css"?>

<window id="main_window"
    title="XBL-CSS-JAR-Test"
    xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
    <element_to_bind/>
</window>

element_to_bind
{
    -moz-binding: url('test.xml#test');
}


<?xml version="1.0" encoding="UTF-8"?>
<bindings xmlns="http://www.mozilla.org/xbl"
    xmlns:xbl="http://www.mozilla.org/xbl"
    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
    xmlns:html="http://www.w3.org/1999/xhtml">

    <binding id="test">
        <content>
            <xul:button label="Hello world" oncommand="document.getBindingParent(this).hello();"/>
        </content>
        <implementation>
            <constructor>
                <![CDATA[
                    this.hello();
                ]]>
            </constructor>
            <method name="hello">
                <body>
                    <![CDATA[
                    alert("Hello world");
                    ]]>
                </body>
            </method>
        </implementation>
    </binding>
</bindings>
Depends on: CVE-2008-5023
Attached file test xul document — 

    
    

    
  

      
      
      
      

        Attached file
          
        test css document
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        test xbl document
        — 
      

    


  

    
    

    
  
Could you attach the signed version? Would save me having to track someone down with a valid signing cert.
Assignee: dveditz → nobody
Blocks: CVE-2008-5023
No longer depends on: CVE-2008-5023
Keywords: regression
Flags: wanted1.9.0.x?

    
    

    
  
Yeah, that's quite odd.  This should work...

    
    

    
  

      
      
      
      

        Attached file
          
        CA
        — 
      

    


  

    
    

    
  


  

    
    

    
  
There you have a test signed jar.
It is signed with a home-brew self-signed CA;
thus, to test it, you will first have to accept/import that CA.
(unfortunately, I don't have a proper cert signed by a commercial CA as of today)

What is odd is that it works if the CA is not imported,
but as soon as it is, it does not work anymore.
The mc.cacert looks valid to me, and the jar properly signed...
For sure, it works on versions without https://bugzilla.mozilla.org/show_bug.cgi?id=424733 bug fix.

Thank you for looking at it!

    
  

        Attachment #356146 -
        Attachment mime type: application/octet-stream → application/x-x509-ca-cert

    
  
Depends on: 424488

    
  
Component: Security: CAPS → XBL
QA Contact: caps → xbl

    
  
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
  
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Summary: XBL not working in signed jar → [FIX]XBL not working in signed jar

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Proposed fix
        — 
          — Splinter Review
      

    


  
So what's going on is that XBL calls StartDocumentLoad immediately after doing newChannel.  At that point, the certificate for the channel is, of course, not available.

Not only that, but it does this on the channel it creates, whereas normally StartDocumentLoad should happen on the channel that calls OnStartRequest (after all the redirects, etc).

So this patch changes things to call StartDocumentLoad in OnStartRequest, with the correct data, for the async load case.  In the sync load case we have to keep doing what we used to, sadly.

This means the sync load case is not safe to use with http://, basically.  So the other change in this patch is to force all non-local-resource loads to be async.

This affects loads done via the loadBindingDocument API (which could affect web pages or web apps, since the function will now return before the XBL is loaded), as well as some of our windows key handler stuff (which includes a user pref for an XBL file to load).  For the latter, if people are using HTTP it might be ok.  For the former, I think we want the new behavior, but could live with not making the change on branch.  It'd mean that loadBindingDocument in a signed jar wouldn't work right, of course...  jst, let me know which you prefer for the branch?

        Attachment #356201 -
        Flags: superreview?(jst)

        Attachment #356201 -
        Flags: review?(jst)

    
    

    
  
nominating for next branch release because this is a regression (broken in 3.0.4), but we'll have to see if making the loads async breaks other things before deciding which way is least broken.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.0.7?

    
    

    
  
I should not that I suspect the same-site redirect check we do is the only thing that saves us from security bugs due to giving the binding document the wrong principal here...

    
    

    
  
We need this in 1.9.1b3 to get real-world testing to make sure we're not breaking other things with this change.

rating "sg:moderate" because using the wrong principal is always scary, although in this case we appear to be safe as long as there's no other way around those existing checks.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
Flags: blocking1.9.0.7+
Whiteboard: [sg:moderate]

        Attachment #356201 -
        Flags: superreview?(jst)

        Attachment #356201 -
        Flags: superreview+

        Attachment #356201 -
        Flags: review?(jst)

        Attachment #356201 -
        Flags: review+

    
    

    
  
(In reply to comment #9)
[...]
> This affects loads done via the loadBindingDocument API (which could affect web
> pages or web apps, since the function will now return before the XBL is
> loaded), as well as some of our windows key handler stuff (which includes a
> user pref for an XBL file to load).  For the latter, if people are using HTTP
> it might be ok.  For the former, I think we want the new behavior, but could
> live with not making the change on branch.  It'd mean that loadBindingDocument
> in a signed jar wouldn't work right, of course...  jst, let me know which you
> prefer for the branch?

I don't know that I feel strongly either way, but I think if I had to pick one I'd leave loadBindingdocument() with a signed jar broken on the branch.

    
  
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P1

    
    

    
  
Pushed http://hg.mozilla.org/mozilla-central/rev/e40649461b57
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.9.0 branch patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #356780 -
        Flags: approval1.9.0.7?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.9.1 patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
OK, the reason this was crashing is that the test does an addBinding call, which tries to do a sync binding load with a null bound element.  The load points to an http:// URI.  Apparently we support sync loads with null bound element but not async ones.

We could probably fix that, but for now I'll just take out the async-forcing change, which should fix that and have the extra benefit of us landing the same change on all three branches. addBinding and loadBindingDocument just won't do the right thing in signed jars...

    
    

    
  


  

        Attachment #356780 -
        Attachment is obsolete: true

        Attachment #356870 -
        Flags: approval1.9.0.7?

        Attachment #356780 -
        Flags: approval1.9.0.7?

    
    

    
  


  

        Attachment #356782 -
        Attachment is obsolete: true

    
    

    
  
Re-pushed on m-c and 1.9.1:
http://hg.mozilla.org/mozilla-central/rev/e0ed5852481a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/744228ccf0ad
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 356870 [details] [diff] [review]
1.9.0 branch patch with -u

Approved for 1.9.0.7, a=dveditz for release-drivers

        Attachment #356870 -
        Flags: approval1.9.0.7? → approval1.9.0.7+

    
    

    
  
Fixed on 1.9.0 branch.
Keywords: fixed1.9.0.7

    
    

    
  
This is a regression from a security bug that also landed on the 1.8 branch so we may this fix there as well.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+

    
    

    
  
So, in 1.9.0.6 Firefox, nothing visibly happens when the application.xul inside the application.jar is loaded on a web server. 

On 1.9.0.7 and 1.9.1, the user receives an error page stating:

Unsafe File Type

The page you are trying to view cannot be shown because it is contained in a file type that may not be safe to open. Please contact the website owners to inform them of this problem.

    *   Please contact the website owners to inform them of this problem.


All of this happens whether the custom cert is imported or not.

I assume that the 1.9.1 (and 1.9.0.7) behavior is the desired, "fixed," behavior?

This is with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021704 GranParadiso/3.0.7pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.

    
    

    
  
> On 1.9.0.7 and 1.9.1, the user receives an error page stating:

Sounds like your server is misconfigured.  Is there a reason you're not loading it from Bugzilla directly?

    
    

    
  
And to answer your question, no.  The behavior you see means you're not testing this bug at all.  I have no idea why you see something different in 1.9.0.6 there.  You shouldn't.

    
    

    
  
I didn't even think of running if from bugzilla, I just put it on my own server...

In any case, I just ran it with jar:https://bug472648.bugzilla.mozilla.org/attachment.cgi?id=356147!/application.xul and now I'm seeing the kind of behavior that I expect. The alerts will fire in 1.9.0.7 and 1.9.1 whereas they won't in 1.9.0.6. 

Marking this as verified.

    
    

    
  
(In reply to comment #27)
> Unsafe File Type

To execute code in a zip archive it must be served with the MIME type application/java-archive, otherwise we will not load the content and you will get this error.

    
    

    
  
Boris, can you work up a 1.8 branch patch for this bug?

    
    

    
  
I'll see what I can do...

    
    

    
  
Dropping this further to sg:low, "moderate" implies there's some set of circumstances that make this really bad (uncommon user action, non-default setting) and for this bug we don't actually know of any way to take advantage of the mistaken principal.
Whiteboard: [sg:moderate] → [sg:low]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.8 branch fix
        — 
          — Splinter Review
      

    


  

        Attachment #364386 -
        Flags: review?

        Attachment #364386 -
        Flags: approval1.8.1.next?

    
  

        Attachment #364386 -
        Flags: review? → review?(jst)

        Attachment #364386 -
        Flags: superreview+

        Attachment #364386 -
        Flags: review?(jst)

        Attachment #364386 -
        Flags: review+

    
    

    
  
Comment on attachment 364386 [details] [diff] [review]
1.8 branch fix

Approved for 1.8.1.21, a=dveditz for release-drivers

        Attachment #364386 -
        Flags: approval1.8.1.next? → approval1.8.1.next+

    
    

    
  
MOZILLA_1_8_BRANCH:

Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v  <--  nsXBLService.cpp
new revision: 1.204.4.6; previous revision: 1.204.4.5
done
Keywords: fixed1.8.1.21

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: