Closed Bug 369814 (jarxss) Opened 18 years ago Closed 17 years ago

jar: protocol is an XSS hazard due to ignoring mime type and being considered same-origin with hosting site

Categories

(Core :: Networking: JAR, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: dcamp)

References

(Depends on 1 open bug, )

Details

(5 keywords, Whiteboard: [sg:high] XSS against sites that allow uploads of files such as images - looking for new networking owner)

Attachments

(16 files, 8 obsolete files)

397 bytes, application/zip
Details
606 bytes, application/zip
Details
21.29 KB, patch
Details | Diff | Splinter Review
282 bytes, image/jpeg
Details
234 bytes, image/jpeg
Details
2.76 KB, application/octet-stream
Details
9.78 KB, patch
bzbarsky
: review+
dveditz
: superreview-
Details | Diff | Splinter Review
2.48 KB, patch
dveditz
: superreview+
Details | Diff | Splinter Review
34.84 KB, patch
bzbarsky
: review+
dveditz
: superreview+
beltzner
: ui-review+
Details | Diff | Splinter Review
1.28 KB, application/zip
Details
158 bytes, application/java-archive
Details
396 bytes, text/html
Details
8.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.81 KB, patch
Details | Diff | Splinter Review
19.06 KB, patch
Details | Diff | Splinter Review
14.84 KB, patch
Details | Diff | Splinter Review
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.
This URL demonstrates "XSS" using a jar file served as image/png: jar:http://www.squarefree.com/bug369814/xss/test.png!/test.html
> * 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. :(
There are also sites that let users upload zips (in order to share their contents), but probably fewer.
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.
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.
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...
Whiteboard: [sg:moderate] XSS against sites that allow uploads of files such as images
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
But much less likely to have entrenched uses we might break.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.9?
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
> 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.
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.
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.
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.
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.
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.
Flags: blocking1.9? → blocking1.9+
Whiteboard: [sg:moderate] XSS against sites that allow uploads of files such as images → [sg:moderate] XSS against sites that allow uploads of files such as images - looking for new networking owner
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.
A new MIME type is warranted. Is any name including "sanitized" the best one? Naming, yay. /be
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?
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 :-).
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.
> 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.
(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...
> 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.
Assignee: nobody → dveditz
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Whiteboard: [sg:moderate] XSS against sites that allow uploads of files such as images - looking for new networking owner → [sg:high] XSS against sites that allow uploads of files such as images - looking for new networking owner
Keywords: arch
OS: Mac OS X → All
Hardware: PC → All
Any such change needs more baking time on trunk, not making 1.8.1.4
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+
Target Milestone: --- → mozilla1.9alpha6
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Assignee: dveditz → jwalden+bmo
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
M8 is in the past and probably off people's radar. Moving to next upcoming milestone.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Is this a beta blocker?
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.
OK. I'm moving this to the next release. Please let me know if anyone objects.
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
pdp discovered this bug independently and disclosed it: http://www.gnucitizen.org/blog/web-mayhem-firefoxs-jar-protocol-issues
Group: security
Priority: -- → P1
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.
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
dcamp taking per discussion on IRC
Assignee: jwalden+bmo → dcamp
Alias: jarxss
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.
Flags: blocking1.8.1.11+ → blocking1.8.1.10+
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
(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?
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.
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.
Oh, hmm. "null principal" is kinda ambiguous... ;)
Attached patch first stab (obsolete) — Splinter Review
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.
Attachment #287901 - Flags: review?(bzbarsky)
Attachment #287901 - Flags: review?(dveditz)
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?
(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.
> 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.
Attached patch v2 (obsolete) — Splinter Review
* 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.
Attachment #287901 - Attachment is obsolete: true
Attachment #288030 - Flags: review?(bzbarsky)
Attachment #287901 - Flags: review?(dveditz)
Attachment #287901 - Flags: review?(bzbarsky)
(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.
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).
> 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 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.
(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 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.
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.
> 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.
Depends on: jarxss2
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.
> other than the human-assisted possibility of link-based phishing, of course Yeah, that's the key problem. ;)
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'
Attached patch v3 (obsolete) — Splinter Review
uses NS_ParseContentType() on the raw header, fixed other nits.
Attachment #288030 - Attachment is obsolete: true
Attachment #288233 - Flags: review?(bzbarsky)
Attachment #288030 - Flags: review?(bzbarsky)
Comment on attachment 288233 [details] [diff] [review] v3 Looks great. Thanks for doing this!
Attachment #288233 - Flags: review?(bzbarsky) → review+
> 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
Attachment #288233 - Flags: superreview?
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...
Attachment #288233 - Attachment is obsolete: true
Attachment #288233 - Flags: superreview?
Attachment #288383 - Attachment filename: bug369814c57.jar → bug369814c57.zip
Attachment #288383 - Attachment filename: bug369814c57.zip → bug369814c57.jar
Attachment #288383 - Attachment mime type: application/octet-stream → application/zip
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.
Hmm. It's odd that object/iframe don't run script. I would expect them to be able to...
nsScriptSecurityManager::CanExecuteScript() checks parent docshells, I imagine that's preventing it object/iframes from working.
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.
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...
Attached patch v4 (obsolete) — Splinter Review
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)
Attachment #288428 - Flags: superreview?(dveditz)
Attachment #288428 - Flags: review?(bzbarsky)
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!
Attachment #288428 - Flags: review?(bzbarsky) → review+
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".
Attachment #288428 - Flags: superreview?(dveditz) → superreview+
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
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>
Whiteboard: [sg:high] XSS against sites that allow uploads of files such as images - looking for new networking owner → [sg:high] [need 1.8 patch] XSS against sites that allow uploads of files such as images - looking for new networking owner
Attached patch naming fixes (obsolete) — Splinter Review
Attachment #288428 - Attachment is obsolete: true
Attached patch branch patch (obsolete) — Splinter Review
Attachment #288538 - Flags: approval1.8.1.10?
(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
The trunk and branch patches both depend on the patches in 403331.
(In reply to comment #73) > The trunk and branch patches both depend on the patches in 403331. Thanks, it applies fine now.
(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?
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?
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?
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.
Attachment #288598 - Flags: review?(bzbarsky)
(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 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?
Attachment #288598 - Flags: review?(bzbarsky) → review+
Attached patch inherited loads v2 (obsolete) — Splinter Review
bumps uuid and returns NS_ERROR_DOM_SECURITY_ERR.
Attachment #288598 - Attachment is obsolete: true
Attachment #288609 - Flags: superreview?(dveditz)
Attached file test cases
jar:https://bugzilla.mozilla.org/attachment.cgi?id=288615!/bug369814.html includes various attempts to get around the unsafe jars
I was thinking something more like a mochitest diff that can be easily checked in the day this bug is opened up....
(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?
Ah, indeed. In which case, please land the tests with the patch!
Attached patch new branch patchSplinter Review
New branch patch incorporates both trunk patches from this bug, including nit fixes. I'll get the mochitests written up tomorrow.
Attachment #288538 - Attachment is obsolete: true
Attachment #288623 - Flags: approval1.8.1.10?
Attachment #288538 - Flags: approval1.8.1.10?
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 on attachment 288609 [details] [diff] [review] inherited loads v2 sr=dveditz
Attachment #288609 - Flags: superreview?(dveditz) → superreview+
hm, after applying the patch, can't see any html inside jar with malformed content type. is this on purpose? attached is jar
link to tar1.jpg: jar:https://bugzilla.mozilla.org/attachment.cgi?id=288634!/tar1.html works before the patch, no html after the patch
(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 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.
Attachment #288623 - Flags: approval1.8.1.10? → approval1.8.1.10+
(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.
with network.jar.open-unsafe-types = true middle clicking on <a href="javascript:..."> executes js with null document.domain
testcase for middle click when ...unsafe = true
when ...unsafe=true at least the flash plugin works via: jar:http://SEVER/flash3.bin!/flash3.swf
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
> 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...
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.
Attachment #288695 - Flags: superreview?(dveditz)
Attachment #288695 - Flags: review?(bzbarsky)
Comment on attachment 288695 [details] [diff] [review] newer branch patch Looks reasonable
Attachment #288695 - Flags: review?(bzbarsky) → review+
Comment on attachment 288623 [details] [diff] [review] new branch patch Checked the approved patch into the 1.8 branch
The patch that was checked in didn't have the error page, here's a patch for that.
Attachment #288772 - Flags: approval1.8.1.10?
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
Attachment #288772 - Flags: superreview+
Attachment #288772 - Flags: approval1.8.1.10?
Attachment #288772 - Flags: approval1.8.1.10+
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.
Attachment #288695 - Flags: superreview?(dveditz)
Attachment #288695 - Flags: superreview-
Attachment #288695 - Flags: approval1.8.1.10-
Comment on attachment 288772 [details] [diff] [review] use the malformedURI error page on the branch landed the error page patch on branch
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.
Keywords: fixed1.8.1.10
Whiteboard: [sg:high] [need 1.8 patch] XSS against sites that allow uploads of files such as images - looking for new networking owner → [sg:high] XSS against sites that allow uploads of files such as images - looking for new networking owner
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/)?
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).
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?
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?
Attachment #288536 - Attachment is obsolete: true
Attachment #288609 - Attachment is obsolete: true
Attachment #289040 - Flags: ui-review?(beltzner)
Attachment #289040 - Flags: superreview?(dveditz)
Attachment #289040 - Flags: review?(bzbarsky)
Attached file zipfile for mochitest
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.
Attachment #289040 - Flags: review?(bzbarsky) → review+
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.
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
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
That shouldn't be happening! Is that on trunk or branch?
(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.
Yeah, I can reproduce that on branch... That seems wrong.
(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.
> 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!
(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.
Attached patch test updatesSplinter Review
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.
Attachment #290297 - Flags: review?(bzbarsky)
Attachment #290297 - Attachment is patch: true
Attachment #290297 - Attachment mime type: application/octet-stream → text/plain
> Bug 285055 seems to cover this. I'm not sure it does. I thought we fired pageshow independently of the background/foreground business.
Comment on attachment 290297 [details] [diff] [review] test updates Looks great
Attachment #290297 - Flags: review?(bzbarsky) → review+
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
Attachment #289040 - Flags: superreview?(dveditz) → superreview+
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
Attachment #289040 - Flags: ui-review?(beltzner) → ui-review+
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).
Depends on: 405571
Filed bug 405571 about the stuck spinner.
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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)
Depends on: 405676
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!).
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.
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.
Flags: blocking1.8.0.15?
Flags: blocking1.8.0.14-
Flags: blocking1.8.0.14+
Depends on: 407303
Flags: blocking1.8.0.15? → blocking1.8.0.15+
caillon, please approve
Attachment #306280 - Flags: approval1.8.0.15?
Attachment #306279 - Attachment description: 1.8.1_combined (for review) → 1.8.1_combined (as reference)
Comment on attachment 306280 [details] [diff] [review] same for 1.8.0 patch a=caillon for 1.8.0.15
Attachment #306280 - Flags: approval1.8.0.15? → approval1.8.0.15+
patch committed to 1.8.0
Keywords: fixed1.8.0.15
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.
(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 142 is about bug 434544 as far as I can tell, not about this bug.
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
Keywords: dev-doc-needed
Depends on: 508369
Is there anyone that understands this well that can write up a quick explanation of it?
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.
> 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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: