Last Comment Bug 369814 - (jarxss) jar: protocol is an XSS hazard due to ignoring mime type and being considered same-origin with hosting site
(jarxss)
: jar: protocol is an XSS hazard due to ignoring mime type and being considered...
Status: RESOLVED FIXED
[sg:high] XSS against sites that allo...
: arch, dev-doc-complete, fixed1.8.0.15, fixed1.8.1.10, testcase
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla1.9beta2
Assigned To: Dave Camp (:dcamp)
:
Mentors:
jar:http://www.squarefree.com/bug3698...
Depends on: 405571 jarxss2 405643 405676 407303 508369
Blocks: xss 403552
  Show dependency treegraph
 
Reported: 2007-02-08 20:51 PST by Jesse Ruderman
Modified: 2011-05-03 14:02 PDT (History)
55 users (show)
benjamin: blocking1.9+
dveditz: blocking1.8.1.10+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14-
caillon: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first stab (6.99 KB, patch)
2007-11-08 14:23 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
v2 (5.49 KB, patch)
2007-11-09 11:44 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
v3 (5.43 KB, patch)
2007-11-11 14:14 PST, Dave Camp (:dcamp)
bzbarsky: review+
Details | Diff | Review
testcase for comment 57 (397 bytes, application/zip)
2007-11-12 14:50 PST, Daniel Veditz [:dveditz]
no flags Details
v4 (8.25 KB, patch)
2007-11-12 20:58 PST, Dave Camp (:dcamp)
bzbarsky: review+
dveditz: superreview+
Details | Diff | Review
naming fixes (8.43 KB, patch)
2007-11-13 12:50 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
branch patch (17.03 KB, patch)
2007-11-13 12:52 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
block inherited loads from unsafe docshells (3.26 KB, patch)
2007-11-13 17:50 PST, Dave Camp (:dcamp)
bzbarsky: review+
Details | Diff | Review
inherited loads v2 (5.99 KB, patch)
2007-11-13 21:05 PST, Dave Camp (:dcamp)
dveditz: superreview+
Details | Diff | Review
test cases (606 bytes, application/zip)
2007-11-13 21:37 PST, Dave Camp (:dcamp)
no flags Details
new branch patch (21.29 KB, patch)
2007-11-13 23:38 PST, Dave Camp (:dcamp)
dveditz: approval1.8.1.10+
Details | Diff | Review
this is jar file - add !/tar1.html to open (282 bytes, image/jpeg)
2007-11-14 01:43 PST, georgi - hopefully not receiving bugspam
no flags Details
this is jar file - add !/mid.html (234 bytes, image/jpeg)
2007-11-14 06:14 PST, georgi - hopefully not receiving bugspam
no flags Details
this is jar file - add !/flash3.swf (2.76 KB, application/octet-stream)
2007-11-14 06:30 PST, georgi - hopefully not receiving bugspam
no flags Details
newer branch patch (9.78 KB, patch)
2007-11-14 10:01 PST, Dave Camp (:dcamp)
bzbarsky: review+
dveditz: superreview-
dveditz: approval1.8.1.10-
Details | Diff | Review
use the malformedURI error page on the branch (2.48 KB, patch)
2007-11-14 16:42 PST, Dave Camp (:dcamp)
dveditz: superreview+
dveditz: approval1.8.1.10+
Details | Diff | Review
trunk patch with tests (34.84 KB, patch)
2007-11-16 13:28 PST, Dave Camp (:dcamp)
bzbarsky: review+
dveditz: superreview+
mbeltzner: ui‑review+
Details | Diff | Review
zipfile for mochitest (1.28 KB, application/zip)
2007-11-16 13:29 PST, Dave Camp (:dcamp)
no flags Details
valid.jar (158 bytes, application/java-archive)
2007-11-18 11:46 PST, georgi - hopefully not receiving bugspam
no flags Details
the circle may be rotating in the 3rd window (396 bytes, text/html)
2007-11-18 11:52 PST, georgi - hopefully not receiving bugspam
no flags Details
test updates (8.53 KB, patch)
2007-11-26 15:49 PST, Dave Camp (:dcamp)
bzbarsky: review+
Details | Diff | Review
test fixes for bug 392567 (2.81 KB, patch)
2007-11-26 22:39 PST, Dave Camp (:dcamp)
no flags Details | Diff | Review
1.8.1_combined (as reference) (19.06 KB, patch)
2008-02-28 07:20 PST, Alexander Sack
no flags Details | Diff | Review
same for 1.8.0 patch (14.84 KB, patch)
2008-02-28 07:21 PST, Alexander Sack
caillon: approval1.8.0.next+
Details | Diff | Review

Description Jesse Ruderman 2007-02-08 20:51:23 PST
Any site that allows image uploads (e.g. avatar images) without binary content sniffing is likely to be vulnerable to XSS (in Gecko browsers only) as a result.  An attacker would only have to upload a malicious zip file to the site and get users to follow a jar: link.

Possible fixes:

* Refuse to open zip file contents with jar: unless the file has a mime type appropriate for zips, such as application/zip.

* Make jar: not be considered same-origin with the rest of the hosting domain, but only with other contents of the jar.

* Both of the above.
Comment 1 Jesse Ruderman 2007-02-08 20:53:41 PST
This URL demonstrates "XSS" using a jar file served as image/png:

jar:http://www.squarefree.com/bug369814/xss/test.png!/test.html
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-08 21:03:19 PST
> * Refuse to open zip file contents with jar: unless the file has a mime type

I like this one, if it doesn't break existing sites too much...

The other one is actually kinda hard.  Esp. on branches.  And I'm not sure it wouldn't break sites even more.  :(
Comment 3 Jesse Ruderman 2007-02-08 21:08:05 PST
There are also sites that let users upload zips (in order to share their contents), but probably fewer.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-08 21:20:23 PST
Yeah, good point.  :(

So for branch the change is actually simple to change this behavior, at first glance -- just modify the nsIJARURI code in SecurityCompareURIs somehow.  Probably to just get the zip file URI and do an Equals() check on it?  Not sure what that would break, offhand....

For trunk, what do you think of making it so http://foo subsumes jar:http://foo but not vice versa or something?

That would mean you could reach into the jar, but it couldn't reach out.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-02-08 21:30:17 PST
We should probably accept at least application/x-java-archive and application/zip (which are in /etc/mime.types on FC6), and probably do some research to look for other legitimate MIME types.  Some searching on Google turns up application/java-archive, application/x-jar, application/x-zip-compressed, application/x-compressed, and multipart/x-zip, although I didn't look very hard.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-08 21:36:23 PST
Also the XPInstall type.

Note also that if we enforce the type we'll need to force the "jar" extension to be associated with one of these types no matter what our OS settings say...
Comment 7 Daniel Veditz [:dveditz] 2007-02-09 00:47:56 PST
Wow, that sucks. On the one hand our current behavior makes perfect logical and useful sense, and on the other what web site is going to be expecting to have to defend against this?

What "existing sites" are we going to break, maybe a few experimental Firefox-only demos at best? We can afford to neuter this harshly if we have to (but it makes me sad).

This is not unlike the issues raised by bug 255107
Comment 8 Daniel Veditz [:dveditz] 2007-02-09 00:48:54 PST
But much less likely to have entrenched uses we might break.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-09 01:00:59 PST
> What "existing sites" are we going to break,

If anything breaks, it'll be intranet applications for the most part, I suspect.  That's the logical place to use it, and I've seen a number of newsgroup questions along those lines.

For trunk, we plan to use jar: for the offline web app stuff, as I understand.  But that's trunk.
Comment 10 Daniel Veditz [:dveditz] 2007-02-09 08:27:31 PST
You don't have to use jar: for that, you could use moz-offline: which maps to the same thing but only works on local files, or whatever other special properties you need. And if it's for offline apps, it shouldn't need to reach into frames that are on-line.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-09 11:10:03 PST
For offline apps, we are using jar: URLs that point to remote files, not local files, and we definitely need them to be same-origin with regular http: frames from the same domain. There's no such thing as "online" or "offline" frames, just frames that happen to be served from the offline cache, which does not change their security status. The same-origin security model is not changed at all for offline apps.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-09 11:17:42 PST
So I'd favour Jesse's first option, except that I'm afraid that if a server serves all .jar files with type application/zip, there may be a myriad of ways for an untrusted user to get unsanitized .jar files onto the server.

Here's another option: create a new MIME type, say application/x-sanitized-jar. jar: loads of resources with that MIME type are considered same-domain with the inner URL. jar: loads that see other MIME types are considered same-domain with other resources from the same jar only.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-09 13:01:40 PST
I'd also like to point out that thinking in terms of "same-domain" is not the best approach for trunk.  It'll get us in trouble; we should be thinking in terms of what subsumes (or does not subsume) what.

Come to think of it, same for branch -- the "is same domain" check on branch is asymmetric.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-09 13:13:13 PST
OK sure. So rephrase to say that we want to be able to have jar: URLs have the same principal as their inner URL, possibly conditional on the server return application/x-sanitized-jar as I mentioned above.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-22 12:02:57 PST
What do people think about the option in comment #12? We really need to settle this soon before offline apps start using this in the wild.
Comment 16 Brendan Eich [:brendan] 2007-02-22 12:46:39 PST
A new MIME type is warranted. Is any name including "sanitized" the best one? Naming, yay.

/be
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-22 12:58:43 PST
That seems reasonable to me.  So basically in those cases the jar channel would give itself the principal from its inner channel/URI instead of what it does now?

Andthen we make nsScriptSecurityManager::SecurityCompareURIs treat jar: URIs asymmetrically on branch?  And something more interesting on trunk?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-22 13:56:20 PST
I'm not really sure how the implementation of this stuff works, but from my quick look at the code, and knowing that you're almost always right --- sure :-).
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-22 14:14:34 PST
Hum. How does access control to WHATWG storage and cookies work? They're based on domain so I think non-sanitized JARs will just have to be prevented from accessing them. And I don't know how hard that would be. Maybe we should just disable remote jar: loads completely unless the MIME type is application/x-sanitized-jar. 

If we were to just go with Jesse's proposal #1, what kind of risks would we face? I'm not sure if my concern in comment #12 (and comment #3) is valid. Presumably sites have to be careful about what types they allow users to upload, but I have no idea what sort of checks people really use.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-22 14:54:11 PST
> Hum. How does access control to WHATWG storage and cookies work? 

WHATWG storage uses the jar's inner URI.  document.cookie looks like it doesn't work really well for jar:.

What we could do is just disallow access to DOM storage from jar:s that aren't application/x-sanitized-jar.

> Maybe we should just disable remote jar: loads completely unless the MIME type
> is application/x-sanitized-jar

If we hardcoded .jar files to get that type, we _could_, I guess.  But it'd definitely completely break all existing consumers of signed jars, and those exist for sure.

Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-22 15:07:32 PST
(In reply to comment #20)
> What we could do is just disallow access to DOM storage from jar:s that aren't
> application/x-sanitized-jar.

We could do that, but running around patching access checks here and there doesn't give me a good feeling.

> > Maybe we should just disable remote jar: loads completely unless the MIME type
> > is application/x-sanitized-jar
> 
> If we hardcoded .jar files to get that type, we _could_, I guess.  But it'd
> definitely completely break all existing consumers of signed jars, and those
> exist for sure.

Hmm, and making an exception for signed jars would bring us back to the possibility of using XSS attacks via signed jars. But who uses remote signed jars with the jar: protocol?

It sure would be nice if we could just get away with requiring jar: inner URIs to have application/zip or other known-good MIME type...
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-02-22 15:15:00 PST
> We could do that, but running around patching access checks here and there
> doesn't give me a good feeling.

Well...  Right now DOM storage has explicit code to dig into jar:s.  We'd just remove that code.  ;)

> But who uses remote signed jars with the jar: protocol?

Anyone who wants to use enablePrivilege, pretty much.  Intranet apps are probably the most common use case.  We get questions about this in newsgroups not infrequently, so those folks would be the ones to really ask.  ;)

> It sure would be nice if we could just get away with requiring jar: inner URIs
> to have application/zip or other known-good MIME type...

Like I said in comment 3, I'm in favor.
Comment 23 Daniel Veditz [:dveditz] 2007-04-30 18:40:31 PDT
Any such change needs more baking time on trunk, not making 1.8.1.4
Comment 24 Mike Connor [:mconnor] 2007-06-28 07:45:10 PDT
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Comment 25 Daniel Veditz [:dveditz] 2007-10-01 15:27:45 PDT
M8 is in the past and probably off people's radar. Moving to next upcoming milestone.
Comment 26 Damon Sicore (:damons) 2007-10-11 15:12:09 PDT
Is this a beta blocker?
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-11 15:31:33 PDT
I don't think so, for a non-public, non-remote-code-execution bug, although I just might not be familiar enough with the requirements for blocking.

Also, I suspect bonsai watchers will be able to figure out pretty quickly exactly what hole a patch here fixes (i.e. the technical overhead to understanding the code/patch is low, and its purpose can't easily be obscured), so this probably needs to be fixed on branch and trunk in a fairly small window, and I'm not going to be able to fix this in time for the next branch release.
Comment 28 Damon Sicore (:damons) 2007-10-11 15:36:07 PDT
OK.   I'm moving this to the next release.  Please let me know if anyone objects.
Comment 29 Jesse Ruderman 2007-11-06 19:29:41 PST
pdp discovered this bug independently and disclosed it:
http://www.gnucitizen.org/blog/web-mayhem-firefoxs-jar-protocol-issues

Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2007-11-07 00:30:18 PST
Unless this can wait until Friday at the earliest, someone else should take this from me, as I can't really justify devoting the necessary time to it until then.
Comment 31 Giorgio Maone [:mao] 2007-11-07 06:53:29 PST
FYI, Latest NoScript development build (disclosed today, after pdp's post) takes a quite drastic but reasonable (considering the behavior of other browsers) measure about this issue.
JAR resources can still be loaded as images, applet classes and the like, but they cannot be loaded as documents.
A regexp-based whitelist is maintained in the "NoScript Options|Advanced|JAR" panel in order to allow specific intranet applications to behave like they always did.
http://noscript.net/getit#direct
Comment 32 Robert Sayre 2007-11-07 12:10:02 PST
dcamp taking per discussion on IRC
Comment 33 Daniel Veditz [:dveditz] 2007-11-07 17:37:50 PST
This can be blocked in nsDocShell::GetAllowJavascript() by disallowing scripts when the document comes from a JAR channel and doesn't meet some criteria for "OK for scripting".

If we want to use the MIME type of the archive itself we will need to expose that on nsIJarChannel. Intranet apps that use signed code will have to make sure their apps use one of the standard .jar MIME types "application/java-archive" or "application/x-jar" (these do not appear to be defined by default in Apache so any use should be intentional). Since Java applets can "phone home" no one is going to allow the uploading of true unvetted jar files to a host that is at risk of XSS.

An alternate approach would be to only allow scripting from a whitelisted domain. This means we'd have to write whitelisting UI (an infobar at least) and worry that users will incorrectly decide which domains are OK to host .jars (most of them won't have the knowledge or context to know what decision they're making). Or else we bury the whitelist in which case we're just breaking signed sites and dumping a huge tech support load on the intranet IT depts.

Can we disable signed-app support entirely for Firefox 3 or Mozilla2? If sites need to do privileged things have users install an addon. The current system is a big series of "whatever" buttons thrown at the user.
Comment 34 Benjamin Smedberg [:bsmedberg] 2007-11-07 19:03:08 PST
I would like to remove support for netscape.securitymanager.enablePrivilege, but I don't think that will affect this bug in particular: it can be useful (and has been done) to ship an entire normal-privilege app as a JAR file, because it ensure cache consistency and can be downloaded as a unit, among other reasons.

So I think we should definitely try to preserve ordinary scripting from JARs served as application/java-archive and application/x-jar
Comment 35 Dave Camp (:dcamp) 2007-11-07 20:48:14 PST
(In reply to comment #33)
> This can be blocked in nsDocShell::GetAllowJavascript() by disallowing scripts
> when the document comes from a JAR channel and doesn't meet some criteria for
> "OK for scripting".

I wonder if it would it be sufficient to just return a null principal from the JAR channel if it doesn't meet these criteria?
Comment 36 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-07 21:05:25 PST
Right now, any unsigned jar returns a null principal.  Whereupon the page gets a principal based on the jar: URI.  Then there is inner URI magic that happens.

dveditz and I did discuss making that magic not happen when the jar fails those criteria.  That is, having the jar just not be same-origin with the site it's on.  We'd also have to fix all the code that manually checks for jar: URIs and gets the host (and doesn't go through the principal!): cookies, permission manager, that sort of thing.
Comment 37 Daniel Veditz [:dveditz] 2007-11-07 23:30:47 PST
right now an unsigned jar returns nsnull as its owner, it does not return a nsNullPrincipal. If it did it would not get a principal based on its inner URI later on.

I'm not in favor though, I don't have much confidence we can root out all the code that uses the document URI rather than the principal.
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-07 23:35:06 PST
Oh, hmm.  "null principal" is kinda ambiguous... ;)
Comment 39 Dave Camp (:dcamp) 2007-11-08 14:23:47 PST
Created attachment 287901 [details] [diff] [review]
first stab

This patch disallows javascript on docshells viewing documents loaded from JAR channels, unless the JAR file came from the local filesystem or was served with application/java-archive or application/x-jar.
Comment 40 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-08 15:12:10 PST
Comment on attachment 287901 [details] [diff] [review]
first stab

nsIAllowScriptsChannel only seems to be able to forbid scripts, right?

I think it's worth thinking about interface docs here (that is, describing which channels should implement this interface); that might lead to a better name for it too.

>     if (mJarFile) {
>+        mAllowScripts = PR_TRUE;

I see nothing preventing reuse of a jar: channel, in which case on the second time around it'll end up with mAllowScripts = true...

I suggest nulling out mJarFile (and resetting mAllowScripts?) up front in AsyncOpen and Open.

>+    if (channel) {
>+        nsCAutoString contentType;
>+        channel->GetContentType(contentType);
>+
>+        if (contentType == NS_LITERAL_CSTRING("application/java-archive") ||
>+            contentType == NS_LITERAL_CSTRING("application/x-jar")) {
>+            mAllowScripts = PR_TRUE;

Ignoring for the moment that the check should just use EqualsLiteral(), this doesn't seem right.  If the server is HTTP/0.9 (and hence doesn't send back a type), we'll guess a type ourselves.  And then we can easily guess one of those types.

Same thing would happen for non-HTTP jar: URIs, though that wouldn't be as big a problem, because a non-HTTP URI would not be same-origin with an HTTP site...

What behavior do we actually want in those cases?
Comment 41 Dave Camp (:dcamp) 2007-11-08 16:26:08 PST
(In reply to comment #40)
> Ignoring for the moment that the check should just use EqualsLiteral(), this
> doesn't seem right.  If the server is HTTP/0.9 (and hence doesn't send back a
> type), we'll guess a type ourselves.  And then we can easily guess one of those
> types.

We could strip LOAD_CALL_CONTENT_SNIFFERS from the load flags for the inner
channel.  We don't ever use the sniffed content type from the JAR channel.

We probably want to do that regardless of how we deal with non-http channels.
Comment 42 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-08 17:51:44 PST
> We could strip LOAD_CALL_CONTENT_SNIFFERS

That wouldn't help, because HTTP _always_ provides a content type, event if it doesn't call content sniffers.  Content sniffers can _override_ the server type, but if there is no server type we will always invoke the unknown decoder.

I agree that we should probably not call content sniffers on the underlying channel for jar:, though.
Comment 43 Dave Camp (:dcamp) 2007-11-09 11:44:05 PST
Created attachment 288030 [details] [diff] [review]
v2

* I ended up just moving denyScripts into nsIJARChannel instead of its own interface.
* Clear the LOAD_CALL_CONTENT_SNIFFERS flag when fetching the JAR.
* Use GetResponseHeader() on http channels.
* Clear mJARFile/mDenyScripts in Open/AsyncOpen.
Comment 44 Daniel Veditz [:dveditz] 2007-11-09 12:02:08 PST
(In reply to comment #40)
> doesn't seem right.  If the server is HTTP/0.9 (and hence doesn't send back a
> type), we'll guess a type ourselves.  And then we can easily guess one of
> those types.

What do we use to guess, anything more than the extension? As a first pass I'd think that if a site allows a .jar extension we can guess it's intended to be active content (either an actual java-archive or a Mozilla thing).

We don't want to sniff the actual content looking for the PKZIP magic number, though. That would re-enable what we're trying to prevent.

> Same thing would happen for non-HTTP jar: URIs, though that wouldn't be as big
> a problem, because a non-HTTP URI would not be same-origin with an HTTP site...

As far as cookies are concerned they would be. Probably DOM storage as well (we treat them as super-cookies) and the password manager. But not truly same-origin for purposes of viewing frames and such.
Comment 45 Daniel Veditz [:dveditz] 2007-11-09 12:21:36 PST
Didn't care for the name, but one advantage of a separate iface is that we could put it on other types of channels in the future that may have similar concerns. For example, would we want to block scripts on file: uris rather than attempt to limit the access of such scripts (be more like IE)?

I guess until we think of an actual case where we'd want this on another type of channel we can leave it on nsIJarChannel (which was my first thought anyway).
Comment 46 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-09 13:12:24 PST
> What do we use to guess, anything more than the extension?

In general, the data and the extension.  At the moment, I suspect it ends up being just the extension, except maybe for file:// URIs.

> We don't want to sniff the actual content looking for the PKZIP magic number,

Well, eventually we do... but I think at that point we probably want to detect the file as an application/zip file.

I'll try to review this tonight.
Comment 47 Daniel Veditz [:dveditz] 2007-11-09 14:25:22 PST
Comment on attachment 288030 [details] [diff] [review]
v2

Why'd you switch from "allow" to "deny" scripts? Means you have to switch the sense of things when testing it, making the code more verbose.
Comment 48 Dave Camp (:dcamp) 2007-11-09 14:27:19 PST
(In reply to comment #47)
> (From update of attachment 288030 [details] [diff] [review])
> Why'd you switch from "allow" to "deny" scripts? Means you have to switch the
> sense of things when testing it, making the code more verbose.

Well as bz pointed out, this can't really Allow scripts when they wouldn't be allowed, only Deny them when they would otherwise be allowed.  But I don't particularly mind either way..
Comment 49 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-09 21:44:09 PST
Comment on attachment 288030 [details] [diff] [review]
v2

>+++ b/docshell/base/nsDocShell.cpp
>+    if (NS_FAILED(jarChannel->GetDenyScripts(&deny)) || deny) {
>+        *aAllowJavascript = PR_FALSE;
>+        return NS_OK;
>+    }

I'd replace that block with:

  *aAllowJavascript = NS_SUCCEEDED(jarChannel->GetDenyScripts(&deny)) &&
                      !deny;

>+++ b/modules/libjar/nsJARChannel.cpp
>@@ -703,9 +722,27 @@ nsJARChannel::OnDownloadComplete(nsIDown
>+        if (httpChannel) {
>+            httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("Content-Type"),
>+                                           contentType);

If we're getting the header, I have two questions:

1)  Is it still OK to do a case-sensitive comparison?
2)  Is it OK to ignore the fact that there might be params in the header?

>+        if (contentType.EqualsLiteral("application/java-archive") ||
>+            contentType.EqualsLiteral("application/x-jar")) {
>+            mDenyScripts = PR_FALSE;

How about:

  mDenyScripts = !contentType.EqualsLiteral(...) &&
                 !contentType.EqualsLiteral(...);

?

>     PRBool                          mIsPending;
>+    PRBool                          mDenyScripts;

It's probably PRPackedBool time here, by the way.
Comment 50 Michal Zalewski 2007-11-10 05:09:06 PST
Guys,

Please note that the vulnerability is more severe and more difficult to mitigate than initially indicated; it does not require the attacked site to host a malicious JAR file, as the security context is not properly updated on 302 redirects, as discovered here:

http://blog.beford.org/?p=8

...and as such, it affects a good part of web.

I can't help but notice that this is strangely reminiscent of my discovery of wyciwyg: redirect behavior that led to same-origin policy bypass and content spoofing (see bug 387333), and perhaps indicative of a need to audit and fix redirect handling once and for all protocols, to mitigate the impact of future flaws.
Comment 51 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-10 10:13:58 PST
> as the security context is not properly updated on 302 redirects

You mean if a site has an open redirector it has a problem?  Of course such a site has problems in general...

In any case, we should certainly improve that on our end.  I've filed bug 403331 on that.

> audit and fix redirect handling once and for all protocols

There are two aspects to redirect handling:

1)  Is it OK to redirect?
2)  What should happen when the redirect happens?

Your wyciwyg: was an instance of point 1.  That's been fully fixed on trunk; any protocol can easily specify whether it's OK to redirect to it, and all the in-tree ones do.

Point 2 is somewhat more complicated, because it depends on what the caller is doing with the HTTP channel.  It needs to be handled on a case-by-case basis.
Comment 52 Michal Zalewski 2007-11-10 11:23:11 PST
Boris,

Yes; open or partly open redirectors are common, and I would guess that most of the top 500 most popular sites on the Web have some. Far fewer sites allow arbitrary files to be uploaded with no filtering, so this probably means the bug should be considered a bit more significant, hence my note.

I also don't think there are any particular inherent security "problems in general" with redirectors - other than the human-assisted possibility of link-based phishing, of course - so I would not dismiss this as a site design error; 302 behavior on jar: handling could not be reasonably anticipated by any web developer as a legitimate caveat for URL redirection.
Comment 53 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-10 12:17:03 PST
> other than the human-assisted possibility of link-based phishing, of course

Yeah, that's the key problem.  ;)
Comment 54 georgi - hopefully not receiving bugspam 2007-11-11 00:35:36 PST
xss in a sandbox via mime overriding is possible this way:
<link rel='stylesheet' href='style.jpg'></link>	
style.jpg:
p:before {content: url('javascript:throw this');}

another strangeness is:
<object type='text/html' data='html.jpg'></object>
html.jpg isn't parsed as html but 
page info | media shows object of type 'text/html'
Comment 55 Dave Camp (:dcamp) 2007-11-11 14:14:25 PST
Created attachment 288233 [details] [diff] [review]
v3

uses NS_ParseContentType() on the raw header, fixed other nits.
Comment 56 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-11 15:28:58 PST
Comment on attachment 288233 [details] [diff] [review]
v3

Looks great.  Thanks for doing this!
Comment 57 georgi - hopefully not receiving bugspam 2007-11-11 23:56:45 PST
> mDenyScripts = !contentType.EqualsLiteral("application/java-archive") &&
                 !contentType.EqualsLiteral("application/x-jar");

not sure this is effective in all cases.
assuming html in jar with bad content type, does any of these work:

1.
<meta http-equiv="Refresh" content="0;URL=data:text/html;,<script>alert(4)</script>">	
2.
	<object data="data:text/html;,<script>alert('obj')</script>"></object>	
	<iframe src="data:text/html;,vv<script>alert('ifr')</script>"></iframe>	
3.
<applet code="Clock1" archive="clock.jpg"></applet>
java is active content and probably the java way of |open browser
window| may work

as an additional test |data:text/html| may be replaced with
|javascript| and |<script>| removed

Comment 58 Dave Camp (:dcamp) 2007-11-12 14:31:17 PST
Comment on attachment 288233 [details] [diff] [review]
v3

So actually this patch will break for jar:jar:http://foo.com/bad.odf!/bad.jar!/test.html if there's a .jar->application/java-archive mapping in the mime database.  I'll cook up a new patch...
Comment 59 Daniel Veditz [:dveditz] 2007-11-12 14:50:37 PST
Created attachment 288383 [details]
testcase for comment 57
Comment 60 Daniel Veditz [:dveditz] 2007-11-12 15:36:00 PST
testcase for comment 57 (1 & 2, not applets)
jar:https://bugzilla.mozilla.org/attachment.cgi?id=288383!/bug369814c57.html

The object and iframe issues are fine, the meta refresh can still run scripts. Dave is going to disable meta redirects on the jar docshell similarly to how he's blocking scripts.
Comment 61 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-12 15:45:21 PST
Hmm.  It's odd that object/iframe don't run script.  I would expect them to be able to...
Comment 62 Dave Camp (:dcamp) 2007-11-12 15:59:14 PST
nsScriptSecurityManager::CanExecuteScript() checks parent docshells, I imagine that's preventing it object/iframes from working.
Comment 63 Daniel Veditz [:dveditz] 2007-11-12 16:57:50 PST
And we really ought to disable plugins on the docshell, too. Flash and Java can do raw sockets even if they can't get cookies from the page. I don't know what you could do that's bad, but better safe than sorry.
Comment 64 Dave Camp (:dcamp) 2007-11-12 16:59:49 PST
Maybe we should just refuse to unpack the jar?

It's a nice feature to be able to glance at zipfile contents and all, but there's a lot to potentially get wrong here...
Comment 65 Dave Camp (:dcamp) 2007-11-12 20:58:35 PST
Created attachment 288428 [details] [diff] [review]
v4

New version:
* Only trusts http and jar inner channels when checking remote loads, and propagates denyScripts from inner jar channels.
* Disables javascript, plugins, and metaredirects if loaded from an unsafe content type type, but...
* Disables loading entirely from unsafe content types by default (can be changed with a pref)
Comment 66 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-12 22:33:15 PST
Comment on attachment 288428 [details] [diff] [review]
v4

>+++ b/modules/libjar/nsIJARChannel.idl
>+    readonly attribute boolean denyScripts;

Could also call this isUnsafe, to mirror what docshell does with it (and update the comments, member/variable names in nsJARChannel, etc).  Either way is fine by me, really.

>+++ b/modules/libpref/src/init/all.js
>+// If false, remove JAR files that are served with a content type other than

s/remove/remote/

With that nit, looks great.  Thanks for doing this!
Comment 67 Daniel Veditz [:dveditz] 2007-11-12 23:08:26 PST
Comment on attachment 288428 [details] [diff] [review]
v4

Looks great, works great. Even with the pref on it now passes the redirect case.

>+pref("jar.open-bad-types", false);

My only nit was the pref name (as we talked about on IRC). rather than start a new top-level pref namespace please use either security. or network., and "unsafe" might be better than "bad".
Comment 68 georgi - hopefully not receiving bugspam 2007-11-13 01:08:34 PST
is v4 a trunk patch?
fails for me:
Hunk #8 FAILED at 750.
1 out of 9 hunks FAILED -- saving rejects to file
modules/libjar/nsJARChannel.cpp.rej
Comment 69 georgi - hopefully not receiving bugspam 2007-11-13 01:28:23 PST
since can't apply the patch atm this may be worth testing:
<a href="http://SARWAR/" target="st">click 1st</a><br>		
<a href="javascript:alert(document.body.innerHTML)" target="st">click 2nd</a><br>	
Comment 70 Dave Camp (:dcamp) 2007-11-13 12:50:37 PST
Created attachment 288536 [details] [diff] [review]
naming fixes
Comment 71 Dave Camp (:dcamp) 2007-11-13 12:52:32 PST
Created attachment 288538 [details] [diff] [review]
branch patch
Comment 72 İsmail Dönmez 2007-11-13 13:19:48 PST
(In reply to comment #71)
> Created an attachment (id=288538) [details]
> branch patch
> 

Doesn't apply to Firefox 2.0.0.9, got

Hunk #8 FAILED at 707.
1 out of 8 hunks FAILED -- saving rejects to file modules/libjar/nsJARChannel.cpp.rej
Comment 73 Dave Camp (:dcamp) 2007-11-13 13:24:34 PST
The trunk and branch patches both depend on the patches in 403331.
Comment 74 İsmail Dönmez 2007-11-13 13:35:10 PST
(In reply to comment #73)
> The trunk and branch patches both depend on the patches in 403331.

Thanks, it applies fine now.
Comment 75 Dave Camp (:dcamp) 2007-11-13 15:55:04 PST
(In reply to comment #69)
> since can't apply the patch atm this may be worth testing:
> <a href="http://SARWAR/" target="st">click 1st</a><br>          
> <a href="javascript:alert(document.body.innerHTML)" target="st">click
> 2nd</a><br>       

This in fact isn't blocked as it should be (if you have the network.jar.open-unsafe-types pref set)

Maybe we should disallow retargeted loads from unsafe channels?
Comment 76 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-13 17:27:15 PST
I'm not quite sure what the problem comment 69 brings up is.  You click on the first link.  You're no longer on the jar: page.  Then what?
Comment 77 Dave Camp (:dcamp) 2007-11-13 17:32:22 PST
The first link opens a new window.  The second link targets a javascript: load in the new window, inheriting the principal from the first window, but not its unsafe channel.

Disallowing retargeted loads isn't enough.  <a href="data:text/html,<script>..."> will also inherit the security context but not the unsafe channel.

I'm working on a patch that blocks loads in a docshell viewing an unsafe channel that would inherit the security context from that unsafe channel.  Does that make sense?
Comment 78 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-13 17:48:53 PST
Yeah, I guess.  I really wish we could disable script/plugins/redirects on a per-nsIPrincipal basis or something.

Perhaps we should get a followup bug filed on having a way to do this?  If we don't for 1.9, we definitely should for 2.0.
Comment 79 Dave Camp (:dcamp) 2007-11-13 17:50:29 PST
Created attachment 288598 [details] [diff] [review]
block inherited loads from unsafe docshells
Comment 80 Dave Camp (:dcamp) 2007-11-13 17:52:23 PST
(In reply to comment #79)
> Created an attachment (id=288598) [details]
> block inherited loads from unsafe docshells
> 

Hrm, it might be nicer to inherit ChannelIsUnsafe like AllowJavascript etc. rather than walking the tree in InternalLoad.
Comment 81 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-13 18:04:47 PST
Comment on attachment 288598 [details] [diff] [review]
block inherited loads from unsafe docshells

More context would have been really nice when reviewing this, for what it's worth.

>+++ b/docshell/base/nsDocShell.cpp

>@@ -6585,6 +6584,25 @@ nsDocShell::InternalLoad(nsIURI * aURI,

<sigh>.  This bothers me, but I guess it's the best we can do...  This really does depend on script being disabled, though, since there's no guarantee that |this| is where the load originated (e.g. in the cases when a principal was passed in).

>+                if (itemDocShell &&
>+                    NS_SUCCEEDED(itemDocShell->GetChannelIsUnsafe(&isUnsafe)) &&
>+                    isUnsafe) {

if (itemDocShell &&
    (NS_FAILED(itemDocShell->GetChannelIsUnsafe(&isUnsafe) ||
     isUnsafe)) {

>+                    return NS_ERROR_FAILURE;

How about NS_ERROR_DOM_SECURITY_ERR or some such?  For that matter, I forgot to mention that about the earlier patch; it had some NS_ERROR_FAILUREs where a better error code could be used.

>+++ b/docshell/base/nsIDocShell.idl

You need to change the IID here.

With those changes, r=bzbarsky

Please make sure to get regression tests for all the various aspects of this bug at least attached to the bug if not checked in with the patch, ok?
Comment 82 Dave Camp (:dcamp) 2007-11-13 21:05:46 PST
Created attachment 288609 [details] [diff] [review]
inherited loads v2

bumps uuid and returns NS_ERROR_DOM_SECURITY_ERR.
Comment 83 Dave Camp (:dcamp) 2007-11-13 21:37:28 PST
Created attachment 288615 [details]
test cases
Comment 84 Dave Camp (:dcamp) 2007-11-13 21:39:29 PST
jar:https://bugzilla.mozilla.org/attachment.cgi?id=288615!/bug369814.html includes various attempts to get around the unsafe jars
Comment 85 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-13 21:41:25 PST
I was thinking something more like a mochitest diff that can be easily checked in the day this bug is opened up....
Comment 86 Reed Loden [:reed] (use needinfo?) 2007-11-13 21:45:26 PST
(In reply to comment #85)
> I was thinking something more like a mochitest diff that can be easily checked
> in the day this bug is opened up....

Uh, you know this bug is public now, right?
Comment 87 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-13 21:47:41 PST
Ah, indeed.  In which case, please land the tests with the patch!
Comment 88 Dave Camp (:dcamp) 2007-11-13 23:38:46 PST
Created attachment 288623 [details] [diff] [review]
new branch patch

New branch patch incorporates both trunk patches from this bug, including nit fixes.

I'll get the mochitests written up tomorrow.
Comment 89 georgi - hopefully not receiving bugspam 2007-11-13 23:57:52 PST
so this patch doesn't prevent against covert form in a jar posting using user's
cookie - in this case the referer will be correct (the buzz word is 'session
riding' or CSRF)?

btw, in the testcases one click probably may be saved via <frameset> and
<frame>.

will try to hit this more if i manage to apply the patch with the hidden
dependency.
Comment 90 Daniel Veditz [:dveditz] 2007-11-14 01:14:06 PST
Comment on attachment 288609 [details] [diff] [review]
inherited loads v2

sr=dveditz
Comment 91 georgi - hopefully not receiving bugspam 2007-11-14 01:43:27 PST
Created attachment 288634 [details]
this is jar file - add !/tar1.html to open

hm, after applying the patch, can't see any html inside jar with malformed content type. 
is this on purpose?

attached is jar
Comment 92 georgi - hopefully not receiving bugspam 2007-11-14 01:45:58 PST
link to tar1.jpg:
jar:https://bugzilla.mozilla.org/attachment.cgi?id=288634!/tar1.html
works before the patch, no html after the patch
Comment 93 Daniel Veditz [:dveditz] 2007-11-14 02:57:00 PST
(In reply to comment #91)
> hm, after applying the patch, can't see any html inside jar with malformed
> content type.  is this on purpose?

Yes, see comment 64 and comment 65. Paranoia about how easily you found holes in the initial approach (comment 57) led to an outright ban. You can set a pref to enable sanitized content on archives with "unsafe" types.

Comment 94 Daniel Veditz [:dveditz] 2007-11-14 02:58:27 PST
Comment on attachment 288623 [details] [diff] [review]
new branch patch

approved for 1.8.1.10, a=dveditz

Applied and tested a bit, works great on all the existing testcases.
Comment 95 georgi - hopefully not receiving bugspam 2007-11-14 03:28:29 PST
(In reply to comment #93)
> (In reply to comment #91)
> > hm, after applying the patch, can't see any html inside jar with malformed
> > content type.  is this on purpose?
> 
> Yes, see comment 64 and comment 65. Paranoia about how easily you found holes
> in the initial approach (comment 57) led to an outright ban. You can set a pref
> to enable sanitized content on archives with "unsafe" types.
> 

this seems nice. this particular case is very hard to secure correctly -  it seems to need a lot of kludges, so better kill the functionality.

Comment 96 georgi - hopefully not receiving bugspam 2007-11-14 06:11:23 PST
with
network.jar.open-unsafe-types = true

middle clicking on <a href="javascript:..."> executes js with null document.domain
Comment 97 georgi - hopefully not receiving bugspam 2007-11-14 06:14:43 PST
Created attachment 288662 [details]
this is jar file - add !/mid.html

testcase for middle click when ...unsafe = true
Comment 98 georgi - hopefully not receiving bugspam 2007-11-14 06:30:08 PST
Created attachment 288666 [details]
this is jar file - add !/flash3.swf

when ...unsafe=true at least the flash plugin works via:
jar:http://SEVER/flash3.bin!/flash3.swf
Comment 99 georgi - hopefully not receiving bugspam 2007-11-14 07:00:49 PST
shouldn't an error page be displayed for jar with bad content type?

typing jar:... into location bar leaves the old page but changes location.href
Comment 100 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-14 08:16:32 PST
> shouldn't an error page be displayed for jar with bad content type?

Yes.  We should use an error code docshell recognizes, or add one to docshell's list...

Comment 101 Dave Camp (:dcamp) 2007-11-14 10:01:24 PST
Created attachment 288695 [details] [diff] [review]
newer branch patch

So I'm going to suggest we land something like this on the branch, disabling the feature on unsafe mime types without a pref.  I don't think it's worth holding up that release to get everything right in case the pref is set.

On trunk it's probably worth putting something like this in sooner rather than later, and then opening a new bug to restore the pref (possibly dependent on the per-nsIPrincipal blocking bz mentioned in comment 78.)

From what I understand we can't add strings (and therefore useful error pages) on the branch, so I chose the malformedURI as the least bad option of the existing error pages.  For a trunk patch I'll put together a new set of error page strings.
Comment 102 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-14 11:59:44 PST
Comment on attachment 288695 [details] [diff] [review]
newer branch patch

Looks reasonable
Comment 103 Daniel Veditz [:dveditz] 2007-11-14 16:19:14 PST
Comment on attachment 288623 [details] [diff] [review]
new branch patch

Checked the approved patch into the 1.8 branch
Comment 104 Dave Camp (:dcamp) 2007-11-14 16:42:51 PST
Created attachment 288772 [details] [diff] [review]
use the malformedURI error page on the branch

The patch that was checked in didn't have the error page, here's a patch for that.
Comment 105 Daniel Veditz [:dveditz] 2007-11-14 17:30:05 PST
Comment on attachment 288772 [details] [diff] [review]
use the malformedURI error page on the branch

sr=dveditz

approved for 1.8.1.10, a=dveditz
Comment 106 Daniel Veditz [:dveditz] 2007-11-14 17:31:45 PST
Comment on attachment 288695 [details] [diff] [review]
newer branch patch

We don't need to rip jar: support out totally for developers. We've done our best, but if there are remaining issues when a user flips an "unsafe" pref that's a relatively low concern.
Comment 107 Dave Camp (:dcamp) 2007-11-14 17:41:44 PST
Comment on attachment 288772 [details] [diff] [review]
use the malformedURI error page on the branch

landed the error page patch on branch
Comment 108 Daniel Veditz [:dveditz] 2007-11-14 19:43:35 PST
jar:https://bugzilla.mozilla.org/attachment.cgi?id=288662!/mid.html is not an issue. Even in a regular HTML page if you middle-click on a javascript: link it's opened up in a new blank context and doesn't inherit the owner. (This in fact annoys tons of people as lots of sites use javascript: links for tracking and popup content, and middle-clicking just gets you a blank tab.)


jar:https://bugzilla.mozilla.org/attachment.cgi?id=288666!/flash3.swf runs the plugin content if you manually flip the pref to "unsafe", but a jar'd web page containing embed/object/applet tags does not run the plugin content. 

That's safe enough for Firefox 2.0.0.10 since this pref is off. We also need to investigate what origin that content would have.
Comment 109 [On PTO until 6/29] 2007-11-15 16:49:05 PST
Jesse or others with some expertise with this area, can you verify that this is fixed in the RC1 for Firefox 2.0.0.10 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/)?
Comment 110 Matej Spiller-Muys 2007-11-16 09:12:41 PST
2.0.0.10-rc1 broke our web application. We have an xpcom application and our web  application is using it from the javascript. We are using a signed jar to get UniversalXPConnect privilege. And now we are getting Permission denied to get property Window.IsFrameLoaded. Even after enabling network.jar.open-unsafe-types it does not work (even though javascript error is gone).
Comment 111 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-16 09:38:24 PST
Using network.jar.open-unsafe-types will open the jar:, but will not run any script in it.  For a web application, you want to be sure you're sending the jar file with either the application/java-archive MIME type or the application/x-jar MIME type.  Once you do that, things should work fine.

Daniel, we really need to advertise that in the run-up to 2.0.0.10.  At the very least we should have a devmo article, and possibly some posts on the developer blog and to .announce?
Comment 112 Dave Camp (:dcamp) 2007-11-16 13:28:00 PST
Created attachment 289040 [details] [diff] [review]
trunk patch with tests

Here's a collected patch for the trunk, incorporating the two previous patches (the main one and the unsafe-loads patch), adding an error page, and adding a test case.

beltzner, can you check the strings in the error page?
Comment 113 Dave Camp (:dcamp) 2007-11-16 13:29:18 PST
Created attachment 289041 [details]
zipfile for mochitest
Comment 114 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-16 13:53:52 PST
Comment on attachment 289040 [details] [diff] [review]
trunk patch with tests

Patch looks ok.

I have some issues with the test.  Why use a timeout (fragile!) instead of observing events?  It'll lead to bizarre orange if the test machine is under load.  Also, might be good to use EventUtils.js instead of reimplementing click event stuff.  And it'd be great if the test reset the pref to the value it had at test start when it finishes.

I haven't read the test in detail, and it mostly looks fine, but those three jumped out at me.
Comment 115 [On PTO until 6/29] 2007-11-16 16:20:39 PST
We still need someone to verify this in the 2.0.0.10 RC1.

This is not an area of expertise for me so I'm not much use here.
Comment 116 [On PTO until 6/29] 2007-11-16 16:25:53 PST
All right. I ran the test cases in comment 108 (after figuring out what it looked like in FF 2.0.0.9). Neither of these two work in 2.0.0.10. Both give an error page stating that the address isn't valid. Is any more verification necessary for this?

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.10) Gecko/2007111504 Firefox/2.0.0.10
Comment 117 georgi - hopefully not receiving bugspam 2007-11-18 01:11:59 PST
seems fixed according to my tests.

kinda strange sign is after a failed jar load, the dotted rotating circle in upper right corner keeps rotating
Comment 118 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-18 10:00:25 PST
That shouldn't be happening!  Is that on trunk or branch?
Comment 119 georgi - hopefully not receiving bugspam 2007-11-18 11:46:00 PST
Created attachment 289242 [details]
valid.jar
Comment 120 georgi - hopefully not receiving bugspam 2007-11-18 11:52:07 PST
Created attachment 289243 [details]
the circle may be rotating in the 3rd window
Comment 121 georgi - hopefully not receiving bugspam 2007-11-18 11:58:53 PST
(In reply to comment #118)
> That shouldn't be happening!  Is that on trunk or branch?
> 

assuming you mean a rotating circle.
don't have trunk with the latest patch at the moment.

on latest linux branch the attachment:
https://bugzilla.mozilla.org/attachment.cgi?id=289243
gives a rotating circle after the 3rd click.

on macosx have mixed results - the circle in 2nd window is rotating, but restarting the fox may be needed.

may be a race unrelated to this bug.

initially found it from location bar.



Comment 122 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-18 14:22:40 PST
Yeah, I can reproduce that on branch...  That seems wrong.
Comment 123 Dave Camp (:dcamp) 2007-11-19 15:39:15 PST
(In reply to comment #114)
> (From update of attachment 289040 [details] [diff] [review])
> Patch looks ok.
> 
> I have some issues with the test.  Why use a timeout (fragile!) instead of
> observing events?  

In some cases there aren't really any events to observe.  Blocked refreshes and inherited-principal loads are just dropped without any events, and I don't get a load event for the error page.

In my local copy I've changed the simple case (the iframe load) to use load events, and waited for the load event before doing the timeout in some cases (to at least reduce the amount of work we're waiting for).  I'd welcome suggestions for fixing the other ones.

> It'll lead to bizarre orange if the test machine is under
> load.  

It won't lead to unexpected orange, but it can lead to greens where there shouldn't be.  All these tests are of the form "wait for the child page to try to poke us, fail if it does so".  If the timer hits early, we'll lose pokes.  Which is arguably worse :/

>Also, might be good to use EventUtils.js instead of reimplementing click
> event stuff.  And it'd be great if the test reset the pref to the value it had
> at test start when it finishes.

Fixed in my local copy.
Comment 124 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-19 19:49:15 PST
> Blocked refreshes and inherited-principal loads are just dropped without any
> events

That's true...  OK.

> and I don't get a load event for the error page.

Hmm.  That keeps coming up.  We should be firing pageshow at least.  Is there a bug on this?

If we can't eliminate all polling, then we can't eliminate all polling.  Eliminating as much as we can is great; thank you for doing that!
Comment 125 Dave Camp (:dcamp) 2007-11-26 15:39:12 PST
(In reply to comment #124) 
> > and I don't get a load event for the error page.
> 
> Hmm.  That keeps coming up.  We should be firing pageshow at least.  Is there a
> bug on this?

I'm not getting a pageshow either.

Bug 285055 seems to cover this.



Comment 126 Dave Camp (:dcamp) 2007-11-26 15:49:25 PST
Created attachment 290297 [details] [diff] [review]
test updates

this is just the test updates.  I added an optional window argument to sendMouseEvent(), but I could add a sendMouseEventToWindow() instead if you'd like.
Comment 127 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-26 16:34:05 PST
> Bug 285055 seems to cover this.

I'm not sure it does.  I thought we fired pageshow independently of the background/foreground business.
Comment 128 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-11-26 16:36:00 PST
Comment on attachment 290297 [details] [diff] [review]
test updates

Looks great
Comment 129 Daniel Veditz [:dveditz] 2007-11-26 16:58:44 PST
Comment on attachment 289040 [details] [diff] [review]
trunk patch with tests

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

The message is much more specific than "unsafeContentType" might lead someone to believe. Either make the message more generic or the property more specific (unsafeJarContentType).

A generic message might be useful in the future so the next firedrill doesn't have to fall back on the malformed URI error :-)

"The page you are trying to view cannot be shown due to its Content-Type. Please contact the website owners to inform them of this problem."  (btw don't leave two spaces after a period in web text.)

This is merely a suggestion, if you want to land as-is it's not that wrong.

sr=dveditz
Comment 130 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-26 18:20:48 PST
Comment on attachment 289040 [details] [diff] [review]
trunk patch with tests

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

- as dveditz says, remove the double space
- s/archive file/file type/ to cover dveditz's other comment without getting into content-type terminology

> <!ENTITY contentEncodingError.longDesc "
>+<ul>
>+  <li>Please contact the website owners to inform them of this problem.</li>
>+</ul>
>+">

>+<!ENTITY unsafeContentType.title "Unsafe Content Type">
>+<!ENTITY unsafeContentType.longDesc "
> <ul>
>   <li>Please contact the website owners to inform them of this problem.</li>
> </ul>
> ">

 - s/Content/File/ in the unsafeContentType.title

with those comments, ui-r=beltzner
Comment 131 Takeshi Nishimura 2007-11-26 20:15:11 PST
Please update your www.mozilla.org's MIME types.
Ex. http://www.mozilla.org/projects/security/components/signed-script-demo.jar
(filed in Bug 358436).
Comment 132 Dave Camp (:dcamp) 2007-11-26 20:57:11 PST
Filed bug 405571 about the stuck spinner.
Comment 133 Dave Camp (:dcamp) 2007-11-26 21:33:19 PST
Checking in browser/locales/en-US/chrome/overrides/appstrings.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/overrides/appstrings.properties,v  <--  appstrings.properties
new revision: 1.10; previous revision: 1.9
done
Checking in browser/locales/en-US/chrome/overrides/netError.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/overrides/netError.dtd,v  <--  netError.dtd
new revision: 1.14; previous revision: 1.13
done
Checking in docshell/base/Makefile.in;
/cvsroot/mozilla/docshell/base/Makefile.in,v  <--  Makefile.in
new revision: 1.66; previous revision: 1.65
done
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.871; previous revision: 1.870
done
Checking in docshell/base/nsIDocShell.idl;
/cvsroot/mozilla/docshell/base/nsIDocShell.idl,v  <--  nsIDocShell.idl
new revision: 1.95; previous revision: 1.94
done
Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.698; previous revision: 1.697
done
Checking in docshell/resources/content/netError.xhtml;
/cvsroot/mozilla/docshell/resources/content/netError.xhtml,v  <--  netError.xhtml
new revision: 1.26; previous revision: 1.25
done
Checking in docshell/test/Makefile.in;
/cvsroot/mozilla/docshell/test/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/docshell/test/bug369814.zip,v
done
Checking in docshell/test/bug369814.zip;
/cvsroot/mozilla/docshell/test/bug369814.zip,v  <--  bug369814.zip
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/docshell/test/test_bug369814.html,v
done
Checking in docshell/test/test_bug369814.html;
/cvsroot/mozilla/docshell/test/test_bug369814.html,v  <--  test_bug369814.html
initial revision: 1.1
done
Checking in dom/locales/en-US/chrome/appstrings.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/appstrings.properties,v  <--  appstrings.properties
new revision: 1.7; previous revision: 1.6
done
Checking in dom/locales/en-US/chrome/netError.dtd;
/cvsroot/mozilla/dom/locales/en-US/chrome/netError.dtd,v  <--  netError.dtd
new revision: 1.14; previous revision: 1.13
done
Checking in modules/libjar/nsIJARChannel.idl;
/cvsroot/mozilla/modules/libjar/nsIJARChannel.idl,v  <--  nsIJARChannel.idl
new revision: 1.8; previous revision: 1.7
done
Checking in modules/libjar/nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.127; previous revision: 1.126
done
Checking in modules/libjar/nsJARChannel.h;
/cvsroot/mozilla/modules/libjar/nsJARChannel.h,v  <--  nsJARChannel.h
new revision: 1.47; previous revision: 1.46
done
Checking in modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.706; previous revision: 3.705
done
Checking in netwerk/base/public/nsNetError.h;
/cvsroot/mozilla/netwerk/base/public/nsNetError.h,v  <--  nsNetError.h
new revision: 1.12; previous revision: 1.11
done
Checking in testing/mochitest/tests/SimpleTest/EventUtils.js;
/cvsroot/mozilla/testing/mochitest/tests/SimpleTest/EventUtils.js,v  <--  EventUtils.js
new revision: 1.5; previous revision: 1.4
done
Comment 134 Dave Camp (:dcamp) 2007-11-26 22:39:49 PST
Created attachment 290344 [details] [diff] [review]
test fixes for bug 392567

After this patch we no longer trust data: uris as inner jar channels (noted in comment 65), which broke the test case for 392567.  I checked in this patch to fix the breakage (it breaks that data uri into a separate jar file to be loaded over http)
Comment 135 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-27 15:14:20 PST
Until someone figures out a way to ship a default branding package and an override system (see most of the discussion in bug 302309), any of these dtds that people want to use branding in are going to be forked across the tree.  

When making these sorts of changes (adding/changing entities), it would be helpful if folks making the changes would check across the tree for other copies of the files and make the equivalent changes or cc someone from the other app(s)--you can cc me for Camino--since not having the changes will break functionality in those apps.  Right now I believe Camino is the only app besides Firefox to have any forked dtd/properties files, but I've seen some discussion about SeaMonkey starting to do this as well.

I'm mentioning this here not to fault Dave, but simply because there are a good number of relevant people cc'd here and I want to raise awareness just a little bit (some developers are already making changes to all copies of the files across the tree, and that's great!).
Comment 136 Volkmar Kostka 2007-11-29 02:57:01 PST
I'm not sure if this is really related to this bug fix but if someone can take a look here:
http://forums.mozillazine.org/viewtopic.php?t=607422
It broke on .10 .

In short the OP loads a jar archive in an iframe and can not access it when using localhost as domain but can when using the real computer name.
If i understood it right the jar is still accessed through localhost so it seems to be on a different domain but it works.
Comment 137 Daniel Veditz [:dveditz] 2007-12-03 14:48:50 PST
Not necessary for Thunderbird 1.5.0.x, possibly wanted for any vendor 1.8.0.x browser release but would have to be weighed against the various web sites this fix broke.
Comment 138 Alexander Sack 2008-02-28 07:20:22 PST
Created attachment 306279 [details] [diff] [review]
1.8.1_combined (as reference)

what went in: combined patch of attachment 288623 [details] [diff] [review] and attachment 288772 [details] [diff] [review]
Comment 139 Alexander Sack 2008-02-28 07:21:26 PST
Created attachment 306280 [details] [diff] [review]
same for 1.8.0 patch

caillon, please approve
Comment 140 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-11 07:23:19 PDT
Comment on attachment 306280 [details] [diff] [review]
same for 1.8.0 patch

a=caillon for 1.8.0.15
Comment 141 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 12:40:40 PDT
patch committed to 1.8.0
Comment 142 Taka 2008-07-03 06:41:44 PDT
There is a problem with the fix. It seems to have broken my application. See
http://forums.mozillazine.org/viewtopic.php?f=25&t=717295

It leaves me quite desperate, I reported the same issue with FF 2.0.0.10 (which was quickly followed up with 2.0.0.11 in which there were no problems) and with FF 3.0.

I got no useful responses (if any) so far. 
Comment 143 Samuel Sidler (old account; do not CC) 2008-07-03 09:04:59 PDT
(In reply to comment #142)
> There is a problem with the fix. It seems to have broken my application. See
> http://forums.mozillazine.org/viewtopic.php?f=25&t=717295

Please file a new bug for this. CC me.
Comment 144 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-07-03 13:49:12 PDT
Comment 142 is about bug 434544 as far as I can tell, not about this bug.
Comment 145 Nickolay_Ponomarev 2009-07-07 08:46:18 PDT
The "Unsafe File Type" error message should be documented on developer.mozilla.org:
1) suggested way of fixing the site (using one of the "safe" content types)
2) ways for user to work around the problem (setting the pref)

Right now the first hit on google for the full message is a useless thread on mozillazine: http://forums.mozillazine.org/viewtopic.php?f=38&t=744495
Comment 146 Eric Shepherd [:sheppy] 2009-11-05 06:15:11 PST
Is there anyone that understands this well that can write up a quick explanation of it?
Comment 147 Eric Shepherd [:sheppy] 2009-11-09 16:06:36 PST
Now documented; see:

https://developer.mozilla.org/en/Security_and_the_jar_protocol

https://developer.mozilla.org/en/Security_in_Firefox_2#Security_improved_for_the_jar.3a_protocol

Let me know (or go ahead and edit) if there are any issues with wording or accuracy. I did my best to interpret what Waldo told me in IRC today.
Comment 148 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-03 14:02:34 PDT
> block inherited loads from unsafe docshells

For what it's worth, the patch did NOT block those.  And the test was buggy, so did not catch that when running in the harness.  It _did_ catch the problem when run standalone....  I'll fix this in bug 508369.

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