[FIX]XBL not working in signed jar

RESOLVED FIXED

Status

()

Core
XBL
P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Maxime Van Assche, Assigned: bz)

Tracking

(4 keywords)

unspecified
x86
Windows XP
fixed1.8.1.21, regression, verified1.9.0.7, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.7 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low])

Attachments

(10 attachments, 2 obsolete attachments)

300 bytes, application/vnd.mozilla.xul+xml
Details
64 bytes, text/css
Details
912 bytes, text/xml
Details
CA
805 bytes, application/x-x509-ca-cert
Details
2.67 KB, application/java-archive
Details
9.58 KB, patch
jst
: review+
Details | Diff | Splinter Review
7.61 KB, patch
Details | Diff | Splinter Review
7.76 KB, patch
Details | Diff | Splinter Review
8.05 KB, patch
Details | Diff | Splinter Review
7.95 KB, patch
jst
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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>
(Reporter)

Updated

8 years ago
Depends on: 424733
(Reporter)

Comment 1

8 years ago
Created attachment 355939 [details]
test xul document
(Reporter)

Comment 2

8 years ago
Created attachment 355940 [details]
test css document
(Reporter)

Comment 3

8 years ago
Created attachment 355941 [details]
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: 424733
No longer depends on: 424733
Keywords: regression
Flags: wanted1.9.0.x?
Yeah, that's quite odd.  This should work...
(Reporter)

Comment 6

8 years ago
Created attachment 356146 [details]
CA
(Reporter)

Comment 7

8 years ago
Created attachment 356147 [details]
test signed jar with mc.cacert
(Reporter)

Comment 8

8 years ago
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
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?
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]

Updated

8 years ago
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
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Created attachment 356780 [details] [diff] [review]
1.9.0 branch patch
Attachment #356780 - Flags: approval1.9.0.7?
Created attachment 356782 [details] [diff] [review]
1.9.1 patch
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/57495e9a03dc
Keywords: fixed1.9.1
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.
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
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...
Created attachment 356870 [details] [diff] [review]
1.9.0 branch patch with -u
Attachment #356780 - Attachment is obsolete: true
Attachment #356870 - Flags: approval1.9.0.7?
Attachment #356780 - Flags: approval1.9.0.7?
Created attachment 356876 [details] [diff] [review]
1.9.1 patch without async forcing
Attachment #356782 - Attachment is obsolete: true
Created attachment 356878 [details] [diff] [review]
m-c patch without async forcing
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
Last Resolved: 8 years ago8 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.
Keywords: fixed1.9.0.7, fixed1.9.1 → verified1.9.0.7, verified1.9.1
(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]
Created attachment 364386 [details] [diff] [review]
1.8 branch fix
Attachment #364386 - Flags: review?
Attachment #364386 - Flags: approval1.8.1.next?
Attachment #364386 - Flags: review? → review?(jst)

Updated

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