When the jar: scheme wraps a URI that returns a Content-disposition: attachment header that will be ignored and the inner zip file unpacked and the specified file interpreted anyway. Since Content-disposition: attachment is often used to prevent potentially dangerous or unknown content from being interpreted by browsers as part of the serving host's domain this can lead to security issues as in bug 472206 comment 27

We probably want to fix this soonish or the bugzilla fix in bug 472206 is pointless.

Thanks for cc'ing us, but I wouldn't mind seeing what the impact of this is. I can't access the URL in bug 472206 comment 27 because I don't have a "Bugzilla Test Instances - LDAP Login". I tried this on our local Bugzilla install but I'm unsure of what it is I'm looking for. Or maybe I just don't need to know.

That test URL should result in a dialog prompting you to save the file, and instead it opens the HTML content in the browser due to this bug. Since bug 472206 is about using Content-disposition: attachment as an anti-XSS measure this is a problem. The specific impact of that should be discussed in that other bug since that tool will have to figure out a workaround until this client bug is fixed.

view-source: has the same issue although it might take a pretty contrived situation to make it dangerous. Would be nice if we could fix it in a central place for all nested schemes rather than have to worry that a future scheme might forget to do the same check, but maybe that's not possible.

I guess this doesn't "block" 472206 since it looks like they'll ship regardless, since they've got a compelling upgrade with or without this problem.

OK, I think I have an initial fix. Patch attached. The patch is pretty simple conceptually: in the existing URILoader check for content-disposition, we pull out the 'transport channel' from JARChannel's (i.e. the channel that actually does the network transport of the file), and use it for the check. This should work for both HTTP and 'multipart' (Question: can JAR URLS use multipart? If so I think there may be some other places I want to check for security holes). Most of the code changes are to change the JARChannel to keep a reference to its inner channel (these can be nested, so they ought to work for nested "jar:jar:http//..." urls: I will test these soon). JST tells me there's a good chance we could wind up with circular references to the inner channel, so I manually release the reference in OnStopRequest(). There is a new nsIJARChannel2.idl file to export the new GetTransportChannel() method, since we can't add the method to the regular IDL (it's frozen). I've tested this on HTTP on my laptop, and we now see a download dialog when you try to download a JAR that has "content-disposition: attach" in its headers. (I'd make this test public, but I need to be able to tweak http.conf--a regular htaccess file can't do what's needed). The non-attach case still loads the JAR. So things seem to be working. I still need to run this through the triserver, plus look into making a mochitest (?), plus test the nested (and multipart?) cases. But I figured I'd post this patch so I can get some feedback in case I'm using the wrong approach.

> Question: can JAR URLS use multipart? Multipart handling is triggered by the MIME type, so if there is an extension that would map to that type then yes. nsIJARChannel isn't frozen, unless I'm missing something... As far as the patch goes, I was wondering whether it's instead time to bite the bullet and finally introduce an interface that exposes content-disposition information (or at least just the whole disposition string) that all three of the relevant channels (jar, http, part) would implement. That would also let extensions expose content-disposition without having to change the core to do so... I thought we had an existing bug on this, but can't find it; it might be that we do not. Note also that we have other callers in our tree that get content-disposition that should probably also be adjusted. As far as the patch goes, you probably need to drop the mInnerChannel reference if AsyncOpen fails, since you won't get an OnStopRequest in that case. In GetTransportChannel you'd want to use NS_ADDREF, not an explicit AddRef call to be consistent with other code. Your indentation in nsJARChannel.h for the "public nsIJARChannel2" line is off; did tabs sneak in? This setup doesn't seem to deal with cases when the underlying channel is an HTTP channel that gets a 3xx response that redirects to an HTTP 200 response with Content-Disposition, since you'll try to get disposition off the pre-redirect channel. If we do decide to keep track of the channel, we'd need to listen for redirects and update on redirect. But if we just hand out the disposition string, it seems like we could do well enough by just grabbing it off the channel passed in to us in OnDownloadComplete, like we do for security info and so forth.

Ah, I found it. See bug 224209 and bug 356086.

OK, here's a second attempt at a patch. I've tested it and it appears to work, including on nested jar:jar:http urls. Patch notes: > finally introduce an interface that exposes content-disposition > information (or at least just the whole disposition string) that > all three of the relevant channels (jar, http, part) would implement. I've made a start at this, but to keep the patch small, I've so far only made the change to nsJARChannel. With a little extra work I could make make multichannels also inherit from nsHashPropertyBag, and then make both it and nsHttpChannel store the Content-Disposition using the same property key, and we'll have a unified interface. This would actually simplify the code a bit, but would also mean more overall code changed. Let me know if that's appropriate, of if we'd rather keep the patch small for the release branch. I'd vote for just making the additional changes. I've changed nsJARChannel to inherit from nsHashPropertyBag (just like the nsHttpChannel and nsBaseChannel classes already do). Rather than trying to keep a pointer to the transport channel (as in the last patch), we now grab the Content-Disposition header from the HTTP channel (or inner JARChannel for nested jar:jar URLs) in OnDownloadComplete, and stuff it into the JARChannel's properties. There are no checks for either Content-Disposition or Content-Type within nsJARChannel::OnDownloadComplete for the multipart case (see comment in patch). Should there be? I created a CGI script that feeds a JAR as the first part of a multipart reply, but it winds up failing (since the root content-type is multipart, not application/java-archive). So I'm not sure it's even a possible case. But Boris mentioned that a MIME extension could be mapped to multipart--I'm not sure if I understand how that would work, or if it's relevant. Also haven't addressed the fact that we're currently using (slightly different) duplicated logic to parse the Content-disposition header in about 3-4 places in the codebase--at some point I'd like to centralize this in a function in nsNetUtils.h. I thought of storing logical values (ATTACHMENT, INLINE) in the properties, rather than the raw Content-Disposition header, so we could truly centralize the parsing, but I'm not sure I know all the use cases, so for now it's the raw header. I'm using Christian's scheme for centralizing Channel property names in nsChannelProperties.h. This required eliminating a define of IMPL_NS_NET in a test program's Makefile, which shouldn't have had it set anyway, as near as I can tell (the only side effect of not having it set is that the program will call NS_LITERAL_STRING, which is slightly less efficient). Finally, while poking through the code I realized we didn't have "Content-Disposition" as a static HTTP_ATOM, so I added it.

Comment on attachment 363038 [details] [diff] [review] Second attempt at fix, using Boris' advice >+++ b/modules/libjar/nsJARChannel.cpp Wed Feb 18 18:16:18 2009 -0800 >+#include "nsChannelProperties.h" Oh, nice. I hadn't realized we had this, actually. ;) We should add other properties here; there's a whole bunch that some channels expose... (base URIs and referrers come to mind offhand). File a followup bug on doing that? >-NS_IMPL_ISUPPORTS6(nsJARChannel, You could use NS_IMPL_ISUPPORTS_INHERITED6(nsJARChannel, nsHashPropertyBag, .... >@@ -747,41 +763,54 @@ nsJARChannel::OnDownloadComplete(nsIDown >+ nsCAutoString header; The indentation here is weird. Did tabs sneak in? >+ // bug 474536: URILoader checks disposition of page-level JAR load >+ rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("Content-Disposition"), >+ header); >+ if (NS_SUCCEEDED(rv)) >+ More weird indentation, especially for "header);". For what it's worth, I wouldn't bother putting the bug# in the comments, or adding comments to the effect that this fixes some bug. That's what hg/cvs blame is for. >+ // TODO: also check for multipart channel? >+ // -- not possible for inner channel to be Multipart? JCD More weird indent. At the moment, the inner channel here in fact cannot be multipart; the only way to get a multipart channel is via the stream converter that decodes multipart responses. So while the underlying channel of a part channel could be a jar channel, the reverse can't happen. > mIsUnsafe = unsafe; >+ Why add the blank line? > } >+ // Soon-to-be common way to get Disposition: right now only nsIJARChannel >+ rv = NS_GetContentDisposition(request, header); >+ if (NS_SUCCEEDED(rv)) >+ SetPropertyAsACString(NS_CHANNEL_PROP_CONTENT_DISPOSITION, header); Again, weird indent. >+++ b/netwerk/base/public/nsChannelProperties.h Wed Feb 18 >+ * HTTP Content-Disposition header of channel. s/HTTP/MIME/, since for part channels there might be no HTTP involved? >+ * Only available from Channels that use HTTP (nsIHttpChannel, nsIJARChannel, >+ * nsIMultiPartChannel). This part is just not true (in that multipart channels may not use http). I'd just take this sentence out. >+++ b/netwerk/base/public/nsNetStrings.h Wed Feb 18 18:16:18 There should be a corresponding change to nsNetStrings.cpp, right? >+++ b/netwerk/protocol/http/src/nsHttpAtomList.h Wed Feb 18 >+HTTP_ATOM(Content_Disposition, "Content-Disposition") I guess this is just an optimization, right? >+++ b/netwerk/streamconv/test/Makefile.in Wed Feb 18 18:16:18 >-DEFINES += -DIMPL_NS_NET Why is this change needed? >+++ b/uriloader/base/nsURILoader.cpp Wed Feb 18 18:16:18 2009 I guess this fixes the issue with showing inline but doesn't actually use the filename for anything. That might be ok, I guess. Fix the above (esp. the nsNetStrings) issue, and looks good. Let's get this in on trunk and branch and file a followup bug for making all three channels use the property on trunk? I'd love to see some code simplification happening here. ;)

OK, here's another patch. I've made all the changes that Boris suggested. Comments/notes: Weird indentations: yes they were tabs. yes, HTTP_ATOM(Content_Disposition) is an optimization. Otherwise we dynamically allocate an atom (I haven't looked to see if that's per lookup, or just the first lookup). > multipart channels may not use http What else can they use? Are we using nsMultipartChannel to handle ftp requests? multipart emails? > There should be a corresponding change to nsNetStrings.cpp, right? Thanks--nice catch. Previously I wasn't initializing this, so any code where IMPL_NS_NET isn't defined (i.e any code outside the same shared object as gNetStrings) would have wound up not finding the property, since we'd be using different strings. Which brings me to my main question: why are we storing channel properties this way (ie. in a hashtable)? It seems like a big kludge to me, compared with something like a new .idl interface (nsIChannelProperties) with separate accessor methods for each stored property. In the latter case the compiler will guarantee that we add logic to every channel when we add a new property, so no channel gets "missed." It will also be more efficient: (accessing a member variable, rather than kludging around with the IMPL_NS_NETWORK hack in order to skip an 8-16 bit string conversion, only to wind up still incurring a hashtable lookup). On all fronts (amount of code, maintainability, efficiency) the whole nsChannelProperties.h/nsNetString.h strategy looks like a stinker to me. Anyone care to defend it? Otherwise I propose adding that our new bug about additional properties to add also include converting to using a new .idl interface. >+++ b/uriloader/base/nsURILoader.cpp Wed Feb 18 18:16:18 2009 > I guess this fixes the issue with showing inline but doesn't actually > use the filename for anything. That might be ok, I guess. I can grab the filename if you tell me how to then pass it up within nsURILoader. Do we want this for the immediate patch? Also, looking at the logic it appears that we treat malformed "Content-Disposition: filename=foo.txt" and/or "name=foo.txt" as "inline", rather than "attachment". Is it common to pass a filename with "inline", or should those be considered to imply attachment instead? I haven't changed it because I'm assuming the logic was added this way for a reason. Finally, the removal of the IMPL_NS_NETWORK from the test makefile is because the code will not compile with it, since the test does not link against the network shared lib, and thus can't find the global gNetStrings variable.

> Are we using nsMultipartChannel to handle ftp requests? We're handling it for any load in a docshell (browser area) or through imagelib which comes back with the MIME type application/x-mixed-replace. This could be an HTTP channel, a data: channel, some extension-implemented channel, an FTP channel pointing to a file whose extension maps to that MIME type for some reason, etc, etc. > why are we storing channel properties this way (ie. in a hashtable) I think mainly because it lets us add new properties without breaking binary compatibility... In particular, we want to be able to add properties that are "optional" in the sense that channels may, but do not have to, set them. This property is a good example of such. I think the nsChannelProperties/nsNetStrings thing doesn't save us all that much; we could be using NS_LITERAL_STRING at all callsites just like we do when not IMPL_NS_NET. It's just good to have a source of documentation for properties people might look for on channels, which is why I suggested adding the other properties to this file. Just in the documentation section is probably fine; I wouldn't worry about adding to gNetStrings, I think. > I can grab the filename if you tell me how to then pass it up within > nsURILoader. Do we want this for the immediate patch? No; let's do that as a followup. It'll be easier once the code for content-disposition is simpler... The relevant callers are scattered about (e.g. nsExternalHelperAppService), and as we unify the API things should Just Work for them for free. We might want to expose the filename/disposition separately from the entire header too, as discussed in that other bug I pointed you at; we can think about that in the followup bug. > we treat malformed "Content-Disposition: filename=foo.txt" and/or > "name=foo.txt" as "inline Yeah. That's unfortunately fairly common, and needs to be treated inline for web compat. :(

Comment on attachment 363135 [details] [diff] [review] Incorporate Boris' feedback Looks good.

Pushed http://hg.mozilla.org/mozilla-central/rev/4bd7dd7645c2 so we can get some testing on this. Jason, do you want to write up some unit tests here too? We should also get this in on branches once it's baked on trunk a bit. Poke me to land on 1.9.1 in a few days, and at that point we should ask for 1.9.0 approval too.

Backed out because it didn't compile: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235075319.1235075381.26575.gz

Pushed again as http://hg.mozilla.org/mozilla-central/rev/e22bfb6a442e with this bustage fix added: - : NS_LITERAL_STRING_INIT(kContentLength, NS_CHANNEL_PROP_CONTENT_LENGTH_STR) - : NS_LITERAL_STRING_INIT(kContentDisposition, NS_CHANNEL_PROP_CONTENT_DISPOSITION_STR) + : NS_LITERAL_STRING_INIT(kContentLength, NS_CHANNEL_PROP_CONTENT_LENGTH_STR), + NS_LITERAL_STRING_INIT(kContentDisposition, NS_CHANNEL_PROP_CONTENT_DISPOSITION_STR) Jason, I'd really like to see some tests here, at least for the jar: part. If you can write some for the URILoader part too, that would be excellent, but it might be a bit of work to detect that we'd put up the helper app dialog without hanging up the browser doing it...

Jason, ping? We need those tests, and need to backport this to 1.9.1 and probably 1.9.0....

The original patch applies fine to 1.9.1/1.9.0, this patch just includes the bustage fix too. I can push this to 1.9.1 today unless we want to wait on those unit tests...

Comment on attachment 367099 [details] [diff] [review] branch patch Approved for 1.9.0.8, a=dveditz for release-drivers

Checking in modules/libjar/nsJARChannel.cpp; /cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v <-- nsJARChannel.cpp new revision: 1.129; previous revision: 1.128 done Checking in modules/libjar/nsJARChannel.h; /cvsroot/mozilla/modules/libjar/nsJARChannel.h,v <-- nsJARChannel.h new revision: 1.48; previous revision: 1.47 done Checking in netwerk/base/public/nsChannelProperties.h; /cvsroot/mozilla/netwerk/base/public/nsChannelProperties.h,v <-- nsChannelProperties.h new revision: 1.3; previous revision: 1.2 done Checking in netwerk/base/public/nsNetStrings.h; /cvsroot/mozilla/netwerk/base/public/nsNetStrings.h,v <-- nsNetStrings.h new revision: 1.2; previous revision: 1.1 done Checking in netwerk/base/public/nsNetUtil.h; /cvsroot/mozilla/netwerk/base/public/nsNetUtil.h,v <-- nsNetUtil.h new revision: 1.112; previous revision: 1.111 done Checking in netwerk/base/src/nsNetStrings.cpp; /cvsroot/mozilla/netwerk/base/src/nsNetStrings.cpp,v <-- nsNetStrings.cpp new revision: 1.2; previous revision: 1.1 done Checking in netwerk/protocol/http/src/nsHttpAtomList.h; /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpAtomList.h,v <-- nsHttpAtomList.h new revision: 1.6; previous revision: 1.5 done Checking in netwerk/streamconv/test/Makefile.in; /cvsroot/mozilla/netwerk/streamconv/test/Makefile.in,v <-- Makefile.in new revision: 1.18; previous revision: 1.17 done Checking in uriloader/base/nsURILoader.cpp; /cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v <-- nsURILoader.cpp new revision: 1.150; previous revision: 1.149 done

One chunk failed to apply added: Checking in nsJARChannel.cpp; /cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v <-- nsJARChannel.cpp new revision: 1.130; previous revision: 1.129 done

OK, at long last, here's a test for this bug. Includes a JAR file and ^headers^ file. Right now just tests that JARChannel contains "content-disposition" property, in PropertyBag interface. Will eventually extend test to cover HTTP and Multi: I'm creating a new bug for that work. I've tested the test, and it works.

OK, I've moved the further work for this bug into bug 356086. I assume someone needs to merge the test code into the 1.9.0 and 1.9.1 branches? I'm reopening this so someone will notice this: feel free to mark closed again if we don't need to merge the test into the branches

Yes, we do need to and the tests, but bug should stay closed. I'll do it after I review them, so it'll certainly be on my todo-list (in my review queue, to be exact).

I wanted to verify it with the unit test but it isn't reviewed and checked in. Dveditz and I went over bug 472206 comment 27 and it isn't a valid testcase anymore. Can someone suggest a valid testcase to verify this fix for 1.9.0.8 since we're making builds in a week?

We've changed how https.js gets loaded in unit tests on the trunk. This version of the patch takes care of this change (and one other file load). The previous patch is still the one to use for older branches.

Comment on attachment 367935 [details] [diff] [review] unit test for this bug Looks good.

Comment on attachment 368988 [details] [diff] [review] Version of last patch that works on trunk Looks good.

(In reply to comment #30) > I wanted to verify it with the unit test but it isn't reviewed and checked in. > Dveditz and I went over bug 472206 comment 27 and it isn't a valid testcase > anymore. > > Can someone suggest a valid testcase to verify this fix for 1.9.0.8 since we're > making builds in a week? Any suggestions on how to validate this fix for 1.9.0.9?

Set up an HTTP server that serves up .jar files with a "content-disposition:attachment" header and then see whether you can open them via a jar:// URI or whether you get a "save as" dialog instead. Then repeat the test without the header to make sure that the header is in fact affecting the behavior you see.

Just in case it matters, I've done the exact tests that Boris has described on my laptop. If the code needs verification by someone else, I can email you the tarball and HTML page with test links (you'd still need to configure apache httpd.conf to add the disposition header for one directory). But like I said, I've already tested myself.

That's good enough for me, Jason. What build did you use?

I tested with the trunk. Give me a URL for whatever version you want me to test and it'll take me 3 minutes to verify.

Sweet. Give it a try with the 1.9.0 nightly at ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/ (use the 3.0.9pre build, not the 3.0.8).

I've verified that this is fixed for 3.0.9pre, using the tests on my laptop. Marking as VERIFIED--remove if that's wrong.

Marking as verified for 1.9.0.9, which is what you checked. Leaving resolved otherwise. We should verify this in a 1.9.1 nightly as well.

Just verified with 3.5b4pre. Now do we mark it VERIFIED? (I'll let you do it :)

Yes, now we mark it verified. :-)

Actually, VERIFIED refers to trunk verification which happened in comment 36 and comment 38. For 1.9.1 verification, we use the "verified1.9.1" keyword, replacing "fixed1.9.1". Ah, Bugzilla, how I love you so.

The Version field has been confusing here too.

1.8.0 doesn't look affected, it refuses to load jar: from http.

Then you're doing something wrong. It works fine, assuming your server is sending the right content-type for the jar.

At-risk sites would have to 1) allow arbitrary user-uploaded content 2) allow users to set arbitrary MIME types, or specifically support .jar files as application/java-archive or application/x-jar 3) Serve them from a domain with sensitive content they need to protect using Content-disposition: attachment to prevent XSS The MIME type restriction was added in bug 369814 (MFSA 2007-37) http://www.mozilla.org/security/announce/2007/mfsa2007-37.html

backport attachment 367099 [details] [diff] [review] for 1.8. I was able to verify that "Content-disposition: attachment" does not trigger the helper dialog on current 1.8 branch; with this patch it properly triggers the helper dialog.

The test for this fix somehow never got checked in. Patch marked "Version of last patch that works on trunk" should be checked into trunk.

Pushed the test as http://hg.mozilla.org/mozilla-central/rev/6ee269c6c118

Backed out the test, because it hangs on tinderbox.

The test is pre-bug 396226, and in particular it assumes nsIHttpServer.stop is synchronous, returning only when shutdown is complete. Control flow should now continue in a callback function passed to the stop() method; using do_test_finished as the argument and replacing the break with a return will probably do the trick, at a glance.