Last Comment Bug 403331 - (jarxss2) Sort out jar: behavior on HTTP redirects
: Sort out jar: behavior on HTTP redirects
: fixed1.8.0.15, verified1.8.1.10
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: Trunk
: x86 Linux
: P1 major (vote)
: ---
Assigned To: Dave Camp (:dcamp)
: Patrick McManus [:mcmanus]
Depends on: 405570
Blocks: jarxss 403552
  Show dependency treegraph
Reported: 2007-11-10 10:11 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2008-03-20 12:35 PDT (History)
24 users (show)
mtschrep: blocking1.9+
dveditz: blocking1.8.1.10+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
dcamp: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (2.56 KB, patch)
2007-11-11 14:11 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v2 (1.91 KB, patch)
2007-11-12 11:31 PST, Dave Camp (:dcamp)
bzbarsky: review-
dveditz: superreview+
Details | Diff | Splinter Review
now with cloneWithJarFile (3.80 KB, patch)
2007-11-12 16:16 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
let's make that cloneWithJARFile (3.80 KB, patch)
2007-11-12 16:53 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
right patch this time (3.80 KB, patch)
2007-11-12 16:54 PST, Dave Camp (:dcamp)
bzbarsky: review+
Details | Diff | Splinter Review
clone the uri in cloneWithJARFile() (4.18 KB, patch)
2007-11-12 23:00 PST, Dave Camp (:dcamp)
dveditz: superreview+
Details | Diff | Splinter Review
branch patch (7.44 KB, patch)
2007-11-13 03:00 PST, Dave Camp (:dcamp)
dveditz: approval1.8.1.10+
Details | Diff | Splinter Review
original testcase, with "safe" mime type (445 bytes, application/java-archive)
2007-11-15 18:04 PST, Daniel Veditz [:dveditz]
no flags Details
verification testcase (try again) (725 bytes, application/java-archive)
2007-11-15 18:28 PST, Daniel Veditz [:dveditz]
no flags Details
mochitest (6.21 KB, patch)
2007-11-19 17:20 PST, Dave Camp (:dcamp)
jwalden+bmo: review+
Details | Diff | Splinter Review
updated mochitests (5.98 KB, patch)
2007-11-26 19:33 PST, Dave Camp (:dcamp)
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2007-11-10 10:11:46 PST
Bug 369814 points out that jar: doesn't update its principal (or URI, for that matter) when an HTTP redirect happens.  Arguably, it should.  

The simplest thing to do here seems to be to replace the "jar file" URI with the post-load URI that the channel loading the jar file ends up with (using NS_GetFinalChannelURI).  Then everything else should Just Work, I think.

Does that make sense?
Comment 1 Daniel Veditz [:dveditz] 2007-11-10 10:42:00 PST
Split off from bug 369814 comment 50 and 51, issue originally posted at where the example jar:!/htm.htm can read your Google contacts

This works on trunk and 1.8 branch.

This blocks bug 369814.  A solution that depends on MIME type to decide the site intended to host active content is completely defeated if a 3rd party attacker can supply the content and headers.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2007-11-11 12:22:26 PST
dcamp, do you want to give this a shot?  Or should I do it?

We probably need to update our URI when the jar file finishes loading, make sure our originalURI is correct as needed, and set our LOAD_REPLACE flag on the basis of the underlying channel. 
Comment 3 Dave Camp (:dcamp) 2007-11-11 14:11:45 PST
Created attachment 288232 [details] [diff] [review]
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2007-11-11 15:16:28 PST
Comment on attachment 288232 [details] [diff] [review]

>+++ b/modules/libjar/nsIJARURI.idl
>+    attribute nsIURI JARFile;

This really bothers me.  I don't think we want people doing that in general.  Could we instead add a method here to "clone with new jar file" or something?  Seems safer.

The rest looks great.
Comment 5 Daniel Veditz [:dveditz] 2007-11-12 10:03:10 PST
I'm with bz, better to create a new JARURI. and call SetOriginalURI() on the old one, don't hack out that part of the code.

You can get the Entry from the original URI, and then compose that together with the new base URI.

This is going to propagate up in a nested jar:, right?
Comment 6 Dave Camp (:dcamp) 2007-11-12 11:31:36 PST
Created attachment 288351 [details] [diff] [review]

This version composes a new URI from the new inner URI and the existing Entry.
Comment 7 Daniel Veditz [:dveditz] 2007-11-12 14:12:53 PST
Comment on attachment 288351 [details] [diff] [review]

Works fine, but I'd rather see a single newSpec.Assign( ... + ...)
just to cut down on the number of string mallocs and copies.

sr=dveditz with that change
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2007-11-12 14:27:57 PST
Any reason not to make some of those testcases into mochitests and commit them when the patch goes in?  I can't think of any.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2007-11-12 15:16:43 PST
Comment on attachment 288351 [details] [diff] [review]

I really do think we should do what I suggested: have a method on nsIJARURI that creates a "clone with different jar file" thing.  That way you don't end up serializing and reparsing nsIURI objects (which is what this patch ends up doing).  That is, all the clone method would do is create an nsJARURI, set its mJARFile to the passed-in URI, set its mJAREntry to a clone of our mJAREntry, and return the whole thing as an nsIJARURI.
Comment 11 Dave Camp (:dcamp) 2007-11-12 16:16:49 PST
Created attachment 288396 [details] [diff] [review]
now with cloneWithJarFile
Comment 12 Dave Camp (:dcamp) 2007-11-12 16:53:36 PST
Created attachment 288404 [details] [diff] [review]
let's make that cloneWithJARFile
Comment 13 Dave Camp (:dcamp) 2007-11-12 16:54:58 PST
Created attachment 288405 [details] [diff] [review]
right patch this time

sorry to vomit all over everyone's bugmail
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2007-11-12 22:18:10 PST
Comment on attachment 288405 [details] [diff] [review]
right patch this time

>+++ b/modules/libjar/nsJARChannel.cpp
>+            rv = channel->GetURI(getter_AddRefs(innerURI));

I think you want to clone this and then NS_TryToSetImmutable the clone, and use that immutable clone as the inner URI...  Otherwise you'll pretty seriously regress performance on signed jar stuff (because every single security check will clone URIs).

>+++ b/modules/libjar/nsJARURI.cpp
>+nsJARURI::CloneWithJARFile(nsIURI *jarFile, nsIJARURI **result)

This should return error if (!jarFile).

With those two nits, this looks wonderful.  r=bzbarsky
Comment 15 Dave Camp (:dcamp) 2007-11-12 23:00:46 PST
Created attachment 288448 [details] [diff] [review]
clone the uri in cloneWithJARFile()
Comment 16 Daniel Veditz [:dveditz] 2007-11-13 00:38:02 PST
Comment on attachment 288448 [details] [diff] [review]
clone the uri in cloneWithJARFile()


Will need a separate 1.8-branch version due to the interface change, unless you want to go with attachment 288351 [details] [diff] [review] for the 1.8 branch.
Comment 17 Dave Camp (:dcamp) 2007-11-13 03:00:34 PST
Created attachment 288472 [details] [diff] [review]
branch patch
Comment 18 Daniel Veditz [:dveditz] 2007-11-13 10:56:22 PST
Comment on attachment 288472 [details] [diff] [review]
branch patch

approved for, a=dveditz for release-drivers
Comment 19 Dave Camp (:dcamp) 2007-11-13 12:00:27 PST
Checked in on the branch:

Checking in nsIJARURI.idl;
/cvsroot/mozilla/modules/libjar/nsIJARURI.idl,v  <--  nsIJARURI.idl
new revision:; previous revision: 1.5
Checking in nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision:; previous revision:
Checking in nsJARURI.cpp;
/cvsroot/mozilla/modules/libjar/nsJARURI.cpp,v  <--  nsJARURI.cpp
new revision:; previous revision: 1.51
Checking in nsJARURI.h;
/cvsroot/mozilla/modules/libjar/nsJARURI.h,v  <--  nsJARURI.h
new revision:; previous revision: 1.18
Comment 20 Al Billings [:abillings] 2007-11-15 16:47:52 PST
Boris, can you verify that this is fixed in the RC1 ( aka
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2007-11-15 16:57:08 PST
I have no idea how to do that short of writing my own tests for it... I doubt I'll be able to do that soon enough to be useful.
Comment 23 Daniel Veditz [:dveditz] 2007-11-15 17:41:49 PST
Someone with a throw-away gmail account could add a contact or two and try the original PoC from comment 1.
Comment 24 Daniel Veditz [:dveditz] 2007-11-15 18:04:12 PST
Created attachment 288940 [details]
original testcase, with "safe" mime type

Sorry, that's naive... the fix for bug 369814 would stop the original PoC regardless of whether we fixed this one. Should work served here with the correct MIME-type to be served as a "we know this is active content" file.
Comment 25 Al Billings [:abillings] 2007-11-15 18:07:31 PST
I'm trying to parse this last remark, Dan. Does this mean that the new attached test case should reproduce the bug here or not?
Comment 26 Daniel Veditz [:dveditz] 2007-11-15 18:28:41 PST
Created attachment 288946 [details]
verification testcase (try again)
Comment 27 Daniel Veditz [:dveditz] 2007-11-15 18:35:23 PST
Here's a locally-hosted testcase that can be used for verification. The MIME-type is set to one that the fix for bug 369814 will allow to run scripts.

Steps to reproduce:
1. Sign in to GMail
  1a. add contacts if you don't have any
2. load the link below:

Actual results in Jar-Jar taunts you and lists your contacts
Expected results: blank page, security error in js console
(NB: if you get an error page something's wrong with the test)
Comment 28 Daniel Veditz [:dveditz] 2007-11-16 14:36:54 PST
You don't even need a GMail account or be signed in to test this: if you see Jar-Jar then you've seen the bug. Jar-Jar is drawn in the XMLdoc onload, and we won't even try to load that doc if we think we're somewhere other than Google. The PoC attack may not succeed unless you're logged on to GMail but the bug we're fixing is visible regardless.

In a fixed version you should get a blank page as mentioned in comment 27

If at some point Google blocks this redirect it will break the testcase. Verify that the redirect still works (such as by demonstrating the bug in a vulnerable version for comparison purposes) before declaring a blank page as an indicator that the bug remains fixed.
Comment 29 Al Billings [:abillings] 2007-11-16 16:23:16 PST
This is verified1.8.1.10. Running a build with a gmail account that I made, I can see Jar Jar. If I run this in RC1, I get a blank page.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2007111504 Firefox/
Comment 30 Dave Camp (:dcamp) 2007-11-19 17:20:32 PST
Created attachment 289416 [details] [diff] [review]
Comment 31 Jeff Walden [:Waldo] (remove +bmo to email) 2007-11-19 19:23:31 PST
Comment on attachment 289416 [details] [diff] [review]

>diff -r b274e2ac8fcf modules/libjar/test/mochitest/test_bug403331.html

>+  // We need because the test server might not
>+  // serve jars with the right content type.

You should add a^headers^ file containing "Content-Type: application/x-jar" to force the desired type; see also:

>diff -r b274e2ac8fcf testing/mochitest/server.js

>+  server.registerPathHandler("/redirect", redirect);

File a bug to remove these changes after bug 401649 lands, please, and mark the dependency?

With those changes this looks fine.
Comment 32 Dave Camp (:dcamp) 2007-11-26 19:33:12 PST
Created attachment 290325 [details] [diff] [review]
updated mochitests
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2007-11-26 19:40:03 PST
Comment on attachment 290325 [details] [diff] [review]
updated mochitests

>+++ b/modules/libjar/test/mochitest

>+  testFrame.src = "jar:!/test.html";
>+  testFrame.onload = function() {

I'd probably set onload before setting src myself.

>+      ok(item.textContent == "testcontents", "Should be able to access the child document");

And I'd use is() here, no ok().

>+    } catch (e) {

For what it's worth, an uncaught JS exception makes a mochitest fail anyway.  If you want the more informative message here that's fine, but if you want to you could take the try/catch out altogether.

r=bzbarsky with the is() nit; either way on the other stuff.
Comment 34 Dave Camp (:dcamp) 2007-11-26 20:35:47 PST
Checking in modules/libjar/nsIJARURI.idl;
/cvsroot/mozilla/modules/libjar/nsIJARURI.idl,v  <--  nsIJARURI.idl
new revision: 1.8; previous revision: 1.7
Checking in modules/libjar/nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.126; previous revision: 1.125
Checking in modules/libjar/nsJARURI.cpp;
/cvsroot/mozilla/modules/libjar/nsJARURI.cpp,v  <--  nsJARURI.cpp
new revision: 1.57; previous revision: 1.56
Checking in modules/libjar/test/;
/cvsroot/mozilla/modules/libjar/test/,v  <--
new revision: 1.3; previous revision: 1.2
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/,v
Checking in modules/libjar/test/mochitest/;
/cvsroot/mozilla/modules/libjar/test/mochitest/,v  <--
initial revision: 1.1
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/,v
Checking in modules/libjar/test/mochitest/;
/cvsroot/mozilla/modules/libjar/test/mochitest/,v  <--
initial revision: 1.1
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/^headers^,v
Checking in modules/libjar/test/mochitest/^headers^;
/cvsroot/mozilla/modules/libjar/test/mochitest/^headers^,v  <--^headers^
initial revision: 1.1
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/test_bug403331.html,v
Checking in modules/libjar/test/mochitest/test_bug403331.html;
/cvsroot/mozilla/modules/libjar/test/mochitest/test_bug403331.html,v  <--  test_bug403331.html
initial revision: 1.1
Checking in testing/mochitest/server.js;
/cvsroot/mozilla/testing/mochitest/server.js,v  <--  server.js
new revision: 1.21; previous revision: 1.20
Comment 35 Dave Camp (:dcamp) 2007-11-26 20:47:16 PST
Filed bug 405570 about the upcoming testing changes.

I also bumped the uuid of nsIJARURI before committing.
Comment 36 Alexander Sack 2008-02-28 07:11:33 PST
Comment on attachment 288472 [details] [diff] [review]
branch patch

a=asac for
Comment 37 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 12:35:53 PDT
Fix committed to the 1.8.0 branch

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