Last Comment Bug 472648 - [FIX]XBL not working in signed jar
: [FIX]XBL not working in signed jar
Status: RESOLVED FIXED
[sg:low]
: fixed1.8.1.21, regression, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: XBL (show other bugs)
: unspecified
: x86 Windows XP
: P1 normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 424488
Blocks: CVE-2008-5023
  Show dependency treegraph
 
Reported: 2009-01-08 03:42 PST by Maxime Van Assche
Modified: 2009-03-01 15:42 PST (History)
8 users (show)
benjamin: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test xul document (300 bytes, application/vnd.mozilla.xul+xml)
2009-01-08 03:44 PST, Maxime Van Assche
no flags Details
test css document (64 bytes, text/css)
2009-01-08 03:45 PST, Maxime Van Assche
no flags Details
test xbl document (912 bytes, text/xml)
2009-01-08 03:45 PST, Maxime Van Assche
no flags Details
CA (805 bytes, application/x-x509-ca-cert )
2009-01-09 01:02 PST, Maxime Van Assche
no flags Details
test signed jar with mc.cacert (2.67 KB, application/java-archive)
2009-01-09 01:03 PST, Maxime Van Assche
no flags Details
Proposed fix (9.58 KB, patch)
2009-01-09 09:25 PST, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
1.9.0 branch patch (3.96 KB, patch)
2009-01-13 11:47 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
1.9.1 patch (6.77 KB, patch)
2009-01-13 11:54 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
1.9.0 branch patch with -u (7.61 KB, patch)
2009-01-13 17:57 PST, Boris Zbarsky [:bz]
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review
1.9.1 patch without async forcing (7.76 KB, patch)
2009-01-13 18:19 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
m-c patch without async forcing (8.05 KB, patch)
2009-01-13 18:20 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
1.8 branch fix (7.95 KB, patch)
2009-02-26 13:52 PST, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
dveditz: approval1.8.1.next+
Details | Diff | Splinter Review

Description Maxime Van Assche 2009-01-08 03:42:18 PST
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>
Comment 1 Maxime Van Assche 2009-01-08 03:44:23 PST
Created attachment 355939 [details]
test xul document
Comment 2 Maxime Van Assche 2009-01-08 03:45:03 PST
Created attachment 355940 [details]
test css document
Comment 3 Maxime Van Assche 2009-01-08 03:45:31 PST
Created attachment 355941 [details]
test xbl document
Comment 4 Daniel Veditz [:dveditz] 2009-01-08 13:42:07 PST
Could you attach the signed version? Would save me having to track someone down with a valid signing cert.
Comment 5 Boris Zbarsky [:bz] 2009-01-08 13:57:23 PST
Yeah, that's quite odd.  This should work...
Comment 6 Maxime Van Assche 2009-01-09 01:02:19 PST
Created attachment 356146 [details]
CA
Comment 7 Maxime Van Assche 2009-01-09 01:03:32 PST
Created attachment 356147 [details]
test signed jar with mc.cacert
Comment 8 Maxime Van Assche 2009-01-09 01:19:41 PST
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!
Comment 9 Boris Zbarsky [:bz] 2009-01-09 09:25:20 PST
Created attachment 356201 [details] [diff] [review]
Proposed fix

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?
Comment 10 Daniel Veditz [:dveditz] 2009-01-09 10:49:35 PST
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.
Comment 11 Boris Zbarsky [:bz] 2009-01-09 11:11:58 PST
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...
Comment 12 Daniel Veditz [:dveditz] 2009-01-09 11:26:08 PST
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.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2009-01-13 00:08:09 PST
(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.
Comment 14 Boris Zbarsky [:bz] 2009-01-13 11:35:41 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/e40649461b57
Comment 15 Boris Zbarsky [:bz] 2009-01-13 11:47:34 PST
Created attachment 356780 [details] [diff] [review]
1.9.0 branch patch
Comment 16 Boris Zbarsky [:bz] 2009-01-13 11:54:37 PST
Created attachment 356782 [details] [diff] [review]
1.9.1 patch
Comment 17 Boris Zbarsky [:bz] 2009-01-13 11:55:15 PST
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/57495e9a03dc
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-13 15:08:41 PST
Ryan backed this out of mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/a05af2663727
http://hg.mozilla.org/mozilla-central/rev/7165ce2d87c7
...and I backed it out of 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b4f040b41e5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d2165c8133c3
due to a test crashing on tinderbox.
Comment 19 Boris Zbarsky [:bz] 2009-01-13 17:51:49 PST
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...
Comment 20 Boris Zbarsky [:bz] 2009-01-13 17:57:35 PST
Created attachment 356870 [details] [diff] [review]
1.9.0 branch patch with -u
Comment 21 Boris Zbarsky [:bz] 2009-01-13 18:19:24 PST
Created attachment 356876 [details] [diff] [review]
1.9.1 patch without async forcing
Comment 22 Boris Zbarsky [:bz] 2009-01-13 18:20:09 PST
Created attachment 356878 [details] [diff] [review]
m-c patch without async forcing
Comment 24 Daniel Veditz [:dveditz] 2009-01-21 15:32:47 PST
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
Comment 25 Boris Zbarsky [:bz] 2009-01-27 16:59:06 PST
Fixed on 1.9.0 branch.
Comment 26 Daniel Veditz [:dveditz] 2009-02-12 01:14:33 PST
This is a regression from a security bug that also landed on the 1.8 branch so we may this fix there as well.
Comment 27 Al Billings [:abillings] 2009-02-17 14:14:38 PST
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.
Comment 28 Boris Zbarsky [:bz] 2009-02-17 14:21:34 PST
> 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?
Comment 29 Boris Zbarsky [:bz] 2009-02-17 14:22:23 PST
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.
Comment 30 Al Billings [:abillings] 2009-02-17 14:30:08 PST
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.
Comment 31 Daniel Veditz [:dveditz] 2009-02-18 17:33:00 PST
(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.
Comment 32 Samuel Sidler (old account; do not CC) 2009-02-23 07:21:50 PST
Boris, can you work up a 1.8 branch patch for this bug?
Comment 33 Boris Zbarsky [:bz] 2009-02-23 14:37:22 PST
I'll see what I can do...
Comment 34 Daniel Veditz [:dveditz] 2009-02-25 13:09:15 PST
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.
Comment 35 Boris Zbarsky [:bz] 2009-02-26 13:52:06 PST
Created attachment 364386 [details] [diff] [review]
1.8 branch fix
Comment 36 Daniel Veditz [:dveditz] 2009-02-27 13:49:00 PST
Comment on attachment 364386 [details] [diff] [review]
1.8 branch fix

Approved for 1.8.1.21, a=dveditz for release-drivers
Comment 37 Reed Loden [:reed] (use needinfo?) 2009-03-01 15:42:52 PST
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

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