1310183 - (CVE-2016-5288) Remote web content can read about:cache entries in 49

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

This seems fixed in my testing on beta and later, but in 49 without electrolysis the test case works:

https://cdn.cliqz.com/browser-f/fun-demo/xkcd.html

What happens is that after a while (or immediately in a rather empty profile) the page displays the contents of about:cache instead of the XKCD comic. Remember you need to disable e10s for this attack to work. This came from our partners at Cliqz and they also have an additional demo attack page that uses a service worker to outlive the tab and keep sending the private data in the background.

If my testing is correct, can someone find out which patch actually fixed this and whether we should release a hotfix?

Bug 1195757 and bug 1253673 seem to be related, but the release information doesn't correspond with my testing.

CCing Gijs and Tanvi in case they know the answer already.

Comment 1

[:Gijs (he/him)]

Regression range for when this became possible (it wasn't possible on 48, in my testing, and I also can't make it work on 50, as noted in comment 0):

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=864cdd00360cdf62ea5132a457ee53a17f9e31aa&tochange=111970c738234569c8c180319155327316335deb

Comment 2

[:Gijs (he/him)]

I suspect this regressed through bug 1275898, and then (looking at "hg log" for nsAboutCache.cpp) was probably fixed in bug 1282750. I'll verify as soon as I can.

If I'm right, this isn't a URI flag issue, but an issue with the asyncopen implementation of about:cache not doing what it's supposed to do. I don't know why it isn't implemented as just another html page and goes through the normal about: code. Probably worth verifying that the checks are now correct when compared to the normal about: code, and if we have any other "manually implemented" handlers for particular about pages.

Comment 3

[:Gijs (he/him)]

Regressed:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c6d42fc8dafa475d6d077421c6499ae4848a7c96&tochange=7182dee7bb1c5d234c48e36e665fd4c5cd0dc552


Fixed:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e45890951ce77c3df05575bd54072b9f300d77b0&tochange=39dffbba764210b25bfc1e749b4f16db77fa0d46

Still getting a more exact range for the latter, but I expect that my suspicions in comment 2 were right.

Liz, you said elsewhere we're doing a 49.0.2. What are the dates for that and is there any chance a fix from 1282750 could be uplifted to be included in that? Christoph, would that be safe to do?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Regressed:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c6d42fc8dafa475d6d077421c6499ae4848a7c96&tochange=7182
> dee7bb1c5d234c48e36e665fd4c5cd0dc552
> 
> 
> Fixed:
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=e45890951ce77c3df05575bd54072b9f300d77b0&tochange=39df
> fbba764210b25bfc1e749b4f16db77fa0d46
> 
> Still getting a more exact range for the latter, but I expect that my
> suspicions in comment 2 were right.
> 
> Liz, you said elsewhere we're doing a 49.0.2. What are the dates for that
> and is there any chance a fix from 1282750 could be uplifted to be included
> in that? Christoph, would that be safe to do?

+lizzard

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Fixed:
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=e45890951ce77c3df05575bd54072b9f300d77b0&tochange=39df
> fbba764210b25bfc1e749b4f16db77fa0d46
> 
> Still getting a more exact range for the latter, but I expect that my
> suspicions in comment 2 were right.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5ecebd971de4d59c5fd54cf47842606d025fd0be&tochange=733e6ac3ee54688ebb80acea0aa23820c7527ec2

So I think it is indeed bug 1282750 that fixed this. Not marking deps because if we don't fix this on 49 I don't want to publicize the regressor and fixing bugs are linked to a sec-sensitive bug.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #3)
> Liz, you said elsewhere we're doing a 49.0.2. What are the dates for that
> and is there any chance a fix from 1282750 could be uplifted to be included
> in that? Christoph, would that be safe to do?

I suppose you mean if it's safe to uplift the changes from Bug 1282750, right? That should be totally fine to do.

Comment 7

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Liz, you said elsewhere we're doing a 49.0.2. What are the dates for that
> > and is there any chance a fix from 1282750 could be uplifted to be included
> > in that? Christoph, would that be safe to do?
> 
> I suppose you mean if it's safe to uplift the changes from Bug 1282750,
> right? That should be totally fine to do.

Yes.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Dan, what do you think here? This doesn't have a sec rating yet, so over to you. At first glance I would rather let this ride with 50 (Nov. 8th release).   I am aiming to do a 49.0.2 release later this week, though.

Comment 9

[Honza Bambas (:mayhemer)]

Thanks for tracking this!  I actually re-routed AsyncOpen2 to AsyncOpen in bug 1275898.  Ups.  Still I think I just copied that from somewhere else... not sure where, but I expect that place to be fixed now too.

Thanks again.

Comment 10

[Daniel Veditz [:dveditz]]

Liz: This is a pretty horrible information disclosure bug -- the URLs in the cache contain a lot of potentially private information, both the sites you're visiting and possibly passwords or session IDs etc in parameters. If there's any way to slip this safe/small fix into a 49 update we should; three weeks is a long time to wait for spillage this bad.

How did the cliqz folks find this?!

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> If I'm right, this isn't a URI flag issue, but an issue with the asyncopen
> implementation of about:cache not doing what it's supposed to do. I don't
> know why it isn't implemented as just another html page and goes through the
> normal about: code. Probably worth verifying that the checks are now correct
> when compared to the normal about: code, and if we have any other "manually
> implemented" handlers for particular about pages.

FWIW, looking at https://dxr.mozilla.org/mozilla-central/search?q=NS_ABOUT_MODULE_CONTRACTID_PREFIX I think about:cache and about:blank are the only special cases here.

Christoph, any chance you can double-triple-check that the checks asyncopen and friends do are now fully correct? What about the suggestion in comment #18 that there were other places where we redirected asyncopen2 to asyncopen and dropped security checks, as it were? How many of these implementations are there and is it possible to audit we're not doing anything dumb anywhere else? (I'd do this myself but I don't have a lot of cycles right now and also am not 100% sure what bit of the changes is responsible for fixing this). Note that actually, looking at this:

     if (!mChannel) {
         return NS_ERROR_UNEXPECTED;
     }
 
     // Kick the walk loop.
     rv = VisitNextStorage();
     if (NS_FAILED(rv)) return rv;
 
-    rv = mChannel->AsyncOpen(aListener, aContext);
+    MOZ_ASSERT(!aContext, "asyncOpen2() does not take a context argument");
+    rv = NS_MaybeOpenChannelUsingAsyncOpen2(mChannel, aListener);

it kind of looks like we're still doing stuff (VisitNextStorage) even before we try to use asyncopen2 and would potentially bail out for security reasons? Shouldn't that be in reverse order?

It looks like https://hg.mozilla.org/integration/mozilla-inbound/rev/4306097418bf is a straight graft onto release on a unified tree (checked locally), so it would be straightforward to take it from that perspective. Liz, per comment #10, can we make this happen?

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #11)
> -    rv = mChannel->AsyncOpen(aListener, aContext);
> +    MOZ_ASSERT(!aContext, "asyncOpen2() does not take a context argument");
> +    rv = NS_MaybeOpenChannelUsingAsyncOpen2(mChannel, aListener);
> 
> it kind of looks like we're still doing stuff (VisitNextStorage) even before
> we try to use asyncopen2 and would potentially bail out for security
> reasons? Shouldn't that be in reverse order?

Yes, you can swap these.  There is no risk in it.  Both just start an async operation, doesn't matter in which order that should happen.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Daniel, Gijs, We can make this happen if I do a build 2 today and we test again and release tomorrow. Can you request uplift in Bug 1282750?  How can we test this (or make sure it doesn't break something else?)

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

If you are sure of your fix let's land it today. If you are still exploring and need another day, then I can delay the dot release till Friday. But please let us know.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #11)
> Christoph, any chance you can double-triple-check that the checks asyncopen
> and friends do are now fully correct? What about the suggestion in comment
> #18 that there were other places where we redirected asyncopen2 to asyncopen
> and dropped security checks, as it were? How many of these implementations
> are there and is it possible to audit we're not doing anything dumb anywhere
> else? (I'd do this myself but I don't have a lot of cycles right now and
> also am not 100% sure what bit of the changes is responsible for fixing
> this). Note that actually, looking at this:

Summary for our IRC discussion.

1) NS_MaybeOpenChannelUsingAsyncOpen2() landed in FF50.
   https://hg.mozilla.org/mozilla-central/rev/063d90be43fc#l3.47
   Inlining the three lines of code works however, we do that in other places.

2) In 99.9% the channel has a loadInfo attached (with the right securityflags) and hence security checks will be executed as intended. We have assertions within our codebase to make sure all channels (besides docshell loads at this point; which are loads of TYPE_DOCUMENT) have a loadinfo attached. The only time it can happen that a channel does not have a loadInfo attached is when an addon implements NewChannel2() and does not attach a Loadinfo, see [1]. Please note that this can't happen for compiled addons, only for JS implemented NewChannel2() implementations. I doubt there are that many that implement NewChannel2() but don't attach a loadInfo. Please further note, that if an addon just implements NewChannel() [not NewChannel2()] our wrapping mechanisms kicks in [1] and security checks are again performed as intended.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#787

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gijs Kruitbosch from comment #1)
> Regression range for when this became possible (it wasn't possible on 48, in
> my testing, and I also can't make it work on 50, as noted in comment 0):

The Cliqz engineer mentioned today that he could reproduce it on 48.0 - 48.0.2, but I doubt that matters a great deal now.

Comment 17

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-release/rev/7356baae8e736a6c9444bdd21562df806a39766b


(In reply to Panos Astithas [:past] from comment #16)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Regression range for when this became possible (it wasn't possible on 48, in
> > my testing, and I also can't make it work on 50, as noted in comment 0):
> 
> The Cliqz engineer mentioned today that he could reproduce it on 48.0 -
> 48.0.2, but I doubt that matters a great deal now.

It wasn't reproducible on 48 on nightly, but bug 1275898 was uplifted to 48 while on aurora, which explains the discrepancy.

It would still be nice if they can also verify that this is gone on 50.

(In reply to Daniel Veditz [:dveditz] from comment #10)
> How did the cliqz folks find this?!

I would still be interested in the answer to this question.

Comment 18

[:Gijs (he/him)]

(fwiw, I built and tested that release patch against release locally, and it seems to work, so...)

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

He did confirm 50 was unaffected when I was there. As to how he found it, keep in mind they are building a very privacy-centric product. In his own words:

"I came across this while testing the caches in FF / CLIQZ to see what kind of privacy concerns could be there; given an extension or an adversary has access to the same, that’s when I started to goof around about:cache through XMLHttpRequest and Fetch and was able to access it from an external webpage."