403331 - (jarxss2) Sort out jar: behavior on HTTP redirects

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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]]

Split off from bug 369814 comment 50 and 51, issue originally posted at http://blog.beford.org/?p=8 where the example jar:http://groups.google.com/searchhistory/url?url=http://beford.org/stuff/htm.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 [:bzbarsky]]

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)]

Attachment: v1

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 288232 [details] [diff] [review]
v1

>+++ 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]]

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)]

Attachment: v2

This version composes a new URI from the new inner URI and the existing Entry.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 288351 [details] [diff] [review]
v2

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 8

[Daniel Veditz [:dveditz]]

handy nested-jar test link
jar:jar:http://releases.mozilla.org/pub/mozilla.org/addons/1000/qm_toolbar-2.0-fx.xpi!/chrome/qmtoolbar.jar!/content/qmtoolbar.js

same with a redirect:
jar:jar:http://groups.google.com/searchhistory/url?url=http://releases.mozilla.org/pub/mozilla.org/addons/1000/qm_toolbar-2.0-fx.xpi!/chrome/qmtoolbar.jar!/content/qmtoolbar.js

Comment 9

[Jeff Walden [:Waldo]]

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 [:bzbarsky]]

Comment on attachment 288351 [details] [diff] [review]
v2

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)]

Attachment: now with cloneWithJarFile

Comment 12

[Dave Camp (:dcamp)]

Attachment: let's make that cloneWithJARFile

Comment 13

[Dave Camp (:dcamp)]

Attachment: right patch this time

sorry to vomit all over everyone's bugmail

Comment 14

[Boris Zbarsky [: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

Comment 15

[Dave Camp (:dcamp)]

Attachment: clone the uri in cloneWithJARFile()

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 288448 [details] [diff] [review]
clone the uri in cloneWithJARFile()

sr=dveditz

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)]

Attachment: branch patch

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 288472 [details] [diff] [review]
branch patch

approved for 1.8.1.10, a=dveditz for release-drivers

Comment 19

[Dave Camp (:dcamp)]

Checked in on the branch:

Checking in nsIJARURI.idl;
/cvsroot/mozilla/modules/libjar/nsIJARURI.idl,v  <--  nsIJARURI.idl
new revision: 1.5.28.1; previous revision: 1.5
done
Checking in nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.116.2.2; previous revision: 1.116.2.1
done
Checking in nsJARURI.cpp;
/cvsroot/mozilla/modules/libjar/nsJARURI.cpp,v  <--  nsJARURI.cpp
new revision: 1.51.12.1; previous revision: 1.51
done
Checking in nsJARURI.h;
/cvsroot/mozilla/modules/libjar/nsJARURI.h,v  <--  nsJARURI.h
new revision: 1.18.12.1; previous revision: 1.18
done

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Boris, can you verify that this is fixed in the 2.0.0.10 RC1 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/) aka 1.8.1.10?

Comment 21

[Boris Zbarsky [:bzbarsky]]

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 22

[Dave Camp (:dcamp)]

Trying with 2.0.0.10-rc1:

jar:http://groups.google.com/searchhistory/url?url=http://beford.org/stuff/htm.jar!/htm.htm 
properly updates the url to jar:http://beford.org/stuff/htm.jar!/htm.htm

jar:jar:http://groups.google.com/searchhistory/url?url=http://releases.mozilla.org/pub/mozilla.org/addons/1000/qm_toolbar-2.0-fx.xpi!/chrome/qmtoolbar.jar!/content/qmtoolbar.js properly updates the url to jar:jar:http://releases.mozilla.org/pub/mozilla.org/addons/1000/qm_toolbar-2.0-fx.xpi!/chrome/qmtoolbar.jar!/content/qmtoolbar.js

Comment 23

[Daniel Veditz [:dveditz]]

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]]

Attachment: 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 - ex-MoCo]]

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]]

Attachment: verification testcase (try again)

Comment 27

[Daniel Veditz [:dveditz]]

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:
jar:http://groups.google.com/searchhistory/url?url=https://bugzilla.mozilla.org/attachment.cgi?id=288946!/jar.html

Actual results in 1.8.1.9: 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]]

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 - ex-MoCo]]

This is verified1.8.1.10. Running a 2.0.0.9 build with a gmail account that I made, I can see Jar Jar. If I run this in 2.0.0.10 RC1, I get a blank page.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.10) Gecko/2007111504 Firefox/2.0.0.10

Comment 30

[Dave Camp (:dcamp)]

Attachment: mochitest

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 289416 [details] [diff] [review]
mochitest

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

>+  // We need network.jar.open-unsafe-types because the test server might not
>+  // serve jars with the right content type.

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

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F


>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)]

Attachment: updated mochitests

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290325 [details] [diff] [review]
updated mochitests

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

>+  testFrame.src = "jar:http://example.org:80/redirect?http://localhost:8888/tests/modules/libjar/test/mochitest/bug403331.zip!/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)]

Checking in modules/libjar/nsIJARURI.idl;
/cvsroot/mozilla/modules/libjar/nsIJARURI.idl,v  <--  nsIJARURI.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.126; previous revision: 1.125
done
Checking in modules/libjar/nsJARURI.cpp;
/cvsroot/mozilla/modules/libjar/nsJARURI.cpp,v  <--  nsJARURI.cpp
new revision: 1.57; previous revision: 1.56
done
Checking in modules/libjar/test/Makefile.in;
/cvsroot/mozilla/modules/libjar/test/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/Makefile.in,v
done
Checking in modules/libjar/test/mochitest/Makefile.in;
/cvsroot/mozilla/modules/libjar/test/mochitest/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/bug403331.zip,v
done
Checking in modules/libjar/test/mochitest/bug403331.zip;
/cvsroot/mozilla/modules/libjar/test/mochitest/bug403331.zip,v  <--  bug403331.zip
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/bug403331.zip^headers^,v
done
Checking in modules/libjar/test/mochitest/bug403331.zip^headers^;
/cvsroot/mozilla/modules/libjar/test/mochitest/bug403331.zip^headers^,v  <--  bug403331.zip^headers^
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/test/mochitest/test_bug403331.html,v
done
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
done
Checking in testing/mochitest/server.js;
/cvsroot/mozilla/testing/mochitest/server.js,v  <--  server.js
new revision: 1.21; previous revision: 1.20
done

Comment 35

[Dave Camp (:dcamp)]

Filed bug 405570 about the upcoming testing changes.

I also bumped the uuid of nsIJARURI before committing.

Comment 36

[Alexander Sack]

Comment on attachment 288472 [details] [diff] [review]
branch patch

a=asac for 1.8.0.15

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix committed to the 1.8.0 branch