User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:220.127.116.11) Gecko/2008120122 Firefox/3.0.5
When using XBL in a signed jar through a css in a xul document, the
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
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
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<?xml-stylesheet href="test.css" type="text/css"?>
<?xml version="1.0" encoding="UTF-8"?>
<xul:button label="Hello world" oncommand="document.getBindingParent(this).hello();"/>
Created attachment 355939 [details]
test xul document
Created attachment 355940 [details]
test css document
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.
Yeah, that's quite odd. This should work...
Created attachment 356146 [details]
Created attachment 356147 [details]
test signed jar with mc.cacert
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!
Created attachment 356201 [details] [diff] [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?
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.
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.
(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.
Created attachment 356780 [details] [diff] [review]
1.9.0 branch patch
Created attachment 356782 [details] [diff] [review]
Ryan backed this out of mozilla-central:
...and I backed it out of 1.9.1:
due to a test crashing on tinderbox.
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
Created attachment 356876 [details] [diff] [review]
1.9.1 patch without async forcing
Created attachment 356878 [details] [diff] [review]
m-c patch without async forcing
Re-pushed on m-c and 1.9.1:
Comment on attachment 356870 [details] [diff] [review]
1.9.0 branch patch with -u
Approved for 18.104.22.168, a=dveditz for release-drivers
Fixed on 1.9.0 branch.
This is a regression from a security bug that also landed on the 1.8 branch so we may this fix there as well.
So, in 22.214.171.124 Firefox, nothing visibly happens when the application.xul inside the application.jar is loaded on a web server.
On 126.96.36.199 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 188.8.131.52) behavior is the desired, "fixed," behavior?
This is with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:184.108.40.206pre) 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 220.127.116.11 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 18.104.22.168 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 22.214.171.124 and 1.9.1 whereas they won't in 126.96.36.199.
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.
Created attachment 364386 [details] [diff] [review]
1.8 branch fix
Comment on attachment 364386 [details] [diff] [review]
1.8 branch fix
Approved for 188.8.131.52, a=dveditz for release-drivers
Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v <-- nsXBLService.cpp
new revision: 184.108.40.206; previous revision: 220.127.116.11