Closed Bug 403331 (jarxss2) Opened 12 years ago Closed 12 years ago

Sort out jar: behavior on HTTP redirects


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






(Reporter: bzbarsky, Assigned: dcamp)




(Keywords: fixed1.8.0.15, verified1.8.1.10, Whiteboard: [sg:high])


(5 files, 6 obsolete files)

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?
Flags: blocking1.9?
Flags: blocking1.8.1.10?
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.
Blocks: jarxss
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.10?
Flags: blocking1.8.1.10+
Whiteboard: [sg:high]
Alias: jarxss2
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. 
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → dcamp
Attachment #288232 - Flags: review?(bzbarsky)
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.
Summary: Sort our jar: behavior on HTTP redirects → Sort out jar: behavior on HTTP redirects
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?
Attached patch v2 (obsolete) — Splinter Review
This version composes a new URI from the new inner URI and the existing Entry.
Attachment #288232 - Attachment is obsolete: true
Attachment #288351 - Flags: superreview?(dveditz)
Attachment #288351 - Flags: review?(bzbarsky)
Attachment #288232 - Flags: review?(bzbarsky)
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
Attachment #288351 - Flags: superreview?(dveditz) → superreview+
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.
Flags: in-testsuite?
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.
Attachment #288351 - Flags: review?(bzbarsky) → review-
Attached patch now with cloneWithJarFile (obsolete) — Splinter Review
Attachment #288351 - Attachment is obsolete: true
Attachment #288396 - Flags: review?(bzbarsky)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Attached patch let's make that cloneWithJARFile (obsolete) — Splinter Review
Attachment #288396 - Attachment is obsolete: true
Attachment #288404 - Flags: review?(bzbarsky)
Attachment #288396 - Flags: review?(bzbarsky)
Attached patch right patch this time (obsolete) — Splinter Review
sorry to vomit all over everyone's bugmail
Attachment #288404 - Attachment is obsolete: true
Attachment #288405 - Flags: review?(bzbarsky)
Attachment #288404 - Flags: review?(bzbarsky)
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
Attachment #288405 - Flags: review?(bzbarsky) → review+
Attachment #288405 - Attachment is obsolete: true
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.
Attachment #288448 - Flags: superreview+
Attached patch branch patchSplinter Review
Attachment #288472 - Flags: approval1.8.1.10?
Comment on attachment 288472 [details] [diff] [review]
branch patch

approved for, a=dveditz for release-drivers
Attachment #288472 - Flags: approval1.8.1.10? → approval1.8.1.10+
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
Keywords: fixed1.8.1.10
Boris, can you verify that this is fixed in the RC1 ( aka
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.
Someone with a throw-away gmail account could add a contact or two and try the original PoC from comment 1.
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.
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?
Attachment #288940 - Attachment is obsolete: true
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)
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.
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/
Attached patch mochitestSplinter Review
Attachment #289416 - Flags: review?(jwalden+bmo)
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.
Attachment #289416 - Flags: review?(jwalden+bmo) → review+
Attachment #290325 - Flags: review?(bzbarsky)
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.
Attachment #290325 - Flags: review?(bzbarsky) → review+
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
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 405570
Filed bug 405570 about the upcoming testing changes.

I also bumped the uuid of nsIJARURI before committing.
Flags: in-testsuite? → in-testsuite+
Comment on attachment 288472 [details] [diff] [review]
branch patch

a=asac for
Attachment #288472 - Flags: approval1.8.0.15+
Fix committed to the 1.8.0 branch
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.