1185256 - (CVE-2015-7207) performance.getEntries() shows x-domain URLs after a redirect when loading from cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[cgvwzq]

Attachment: PoC code (does not work from file://)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

It is possible to read x-domain URLs after a redirect if the page can be iframed. What I think is a violation of the SOP and could be used to steal sensitive data from several pages.

If http://victim/ redirects to http://victim/?secret an attacker can iframe the first page and obtain the "secret" of the second one.

The exploit abuses what seems a bug in performance.getEntries() when dealing with cached pages.

Details and PoC: http://vwzq.net/lab/xreadurl/


Actual results:

performance.getEntries() contains the destination URL of the redirection after using history.back() and loading the page from browser's cache


Expected results:

performance.getEntries() should show the original URL instead of the redirection's destination

Comment 1

[cgvwzq]

A tracking bug for W3C has been created: https://github.com/w3c/resource-timing/issues/29

Comment 2

[cgvwzq]

Firefox specific and minimized PoC: http://vwzq.net/lab/xreadurl/ff.html

Comment 3

[Daniel Veditz [:dveditz]]

I'm not seeing this in Nightly (Firefox 42), it might be fixed already? Anne: you familiar with this issue (since you're in the spec)?

Comment 4

[Daniel Veditz [:dveditz]]

Matt can't reproduce on 39 either. What are we doing differently than the reported

Comment 5

[Anne (:annevk)]

I did not know about this one, only bug 1180145. But it does seem like we should do another security review of resource timing, since this is the second SOP violation. And in particular look at cross-origin and any redirect behavior.

Comment 6

[cgvwzq]

I have tested it in several OSes with both 39 and 42 (default configurations), and it works like a charm. I just vist the page and the redirect-to final url (eg: http://demo.vwzq.net/php/token_redirect.php?secret=c1869f5ce8edf8023cbcd6c15ce30fe8) is displayed.

Did you tried the Firefox specific PoC? http://vwzq.net/lab/xreadurl/ff.html

Since performance.getEntries() shows cross-origin resources (despite not follow redirects), when performing the history.back() trick and loading a cached page, the page loaded will be the redirect-to final url instead of the original one. And we have access to that location.

Comment 7

[Daniel Veditz [:dveditz]]

I reproduced this twice (in 39) but mostly can't: mostly either nothing happens or I end up going back one in history.

Not sure we need to keep this bug hidden if the github issue is public and points to the generic POC. Chrome's version is hidden though.

Comment 8

[Andrew McCreight [:mccr8]]

Valentin, is this something you could look into? Thanks.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

I'll take a look.

Comment 10

[Jason Duell]

Andrew: any chance you can find someone on the DOM team with chops (or willingness to learn) the resource timing stuff who can work on this?  Valentin is slammed with B2G stuff right now.

Comment 11

[Jason Duell]

I'm going to see if dragana can take this.  Dragana, here's what valentin brain-dumped to me about the fix:

so... each window has a performance object, which has a list of entries... and in nsHttpChannel::OnStopRequest we just call doc->AddEntry on that window/document's performance object
I am not sure how the BF cache works for an iframe but apparently we shouldn't be able to find that resourceTimingEntry in the iframe's list
so, we should either not add it, if it's an iframe... change the URI, or not return it in this case.

Comment 12

[Andrew Overholt [:overholt]]

I've also asked Kyle (who doesn't know this code AFAIK) to help; maybe for reviews if nothing else?

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_v1.patch

I do not know much about this code so maybe I am a bit off.

So the problem with  history.goBack is that we do not save OriginalURI. The originalURI is only different than mURI (this is from nsHttpChannel) if we have a redirect and this info is not save in history entry.

I am not sure about SessionHistory.jsm, I have not change that one.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8658252 [details] [diff] [review]
bug_1185256_v1.patch

I think Andrew may have meant Kyle Machulis here ... he certainly didn't talk to me.

Comment 15

[Dragana Damjanovic [:dragana]]

Comment on attachment 8658252 [details] [diff] [review]
bug_1185256_v1.patch

Let's see if I have the right Kyle this time.

Comment 16

[Andrew Overholt [:overholt]]

Yeah, sorry, I meant qDot.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8658252 [details] [diff] [review]
bug_1185256_v1.patch

Review of attachment 8658252 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, made some nit comments, but I am in no way qualified to review this, never even seen this part of gecko before.

::: docshell/base/nsIDocShell.idl
@@ +125,5 @@
>     * @param aURI            - The URI to load.
> +   * @param aOriginalURI    - The redirected channels have this set to the uri
> +   *                          they are redirected from. This parameter is used
> +   *                          when we are loading from history (e.g.
> +   *                          history.goBack) to resotore the original channel

nit: restore

::: docshell/base/nsIDocShellLoadInfo.idl
@@ +27,5 @@
>  
> +    /**
> +     * For a redirected channel the orifinalURI is the uri we are redirecting
> +     * from.
> +     */ 

nit: extra whitespace, originalURI

::: docshell/shistory/nsISHEntry.idl
@@ +40,5 @@
>       */
>      readonly attribute nsIURI URI;
>  
>      /**
> +     * A readonly property that returns the riginal URI of the current entry.

nit: original

Comment 18

[Dragana Damjanovic [:dragana]]

Comment on attachment 8658252 [details] [diff] [review]
bug_1185256_v1.patch

bz, if you do not have time give it to someone, I do not know how else know docshell, thanks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

The other obvious possibility is smaug, but he's swamped with reviews.

ccing mconley in case he's interested, but I can get to this review on Tuesday (I'm off on Monday, so can't do it then).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658252 [details] [diff] [review]
bug_1185256_v1.patch

OK, it's not Tuesday, but...

>@@ -5240,16 +5243,17 @@ nsDocShell::Reload(uint32_t aReloadFlags
>+                      nullptr,

That doesn't look right to me.  If page A iframes cross-origin page B and then B does a location.reload(), won't this end showing the post-redirect URI for B?  I guess only if mOSHE and mLSHE are both null, and I'm not sure how to hit that case.  But in any case, would it make sense to grab the originalURI off the document's channel here?

>+++ b/docshell/base/nsDocShell.h

Please document the semantics of the aOriginalURI arg of DoURILoad.

>+++ b/docshell/base/nsIDocShell.idl

Need to change the IID.

>+   * @param aOriginalURI    - The redirected channels have this set to the uri
>+   *                          they are redirected from. This parameter is used
>+   *                          when we are loading from history (e.g.
>+   *                          history.goBack) to resotore the original channel
>+   *                          state.

I think this should document what the argument does, not who uses it.  So perhaps:

  @param aOriginalURI - The URI to set as the originalURI on the channel that
                        does the load.  If null, aURI will be set as the
                        originalURI.

>+++ b/docshell/base/nsIDocShellLoadInfo.idl

Need to change the IID.

>+     * For a redirected channel the orifinalURI is the uri we are redirecting
>+     * from.

Again, this should document what the member does. Something like:

  * The originalURI to be passed to nsIDocShell.internalLoad.  May be null.

>+++ b/docshell/shistory/nsISHEntry.idl

Change the IID.

>+     * A readonly property that returns the riginal URI of the current entry.
>+     * If a entry is the result of a redirect this attribute holds original
>+     * URI. The object returned is of type nsIURI

"returns the original URI", "If an entry" (spelling and grammar respectively).

r=me with those nits fixed.  Sorry for the lag here... :(

Comment 21

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_v2.patch

I have incorporated you comments and change one additional line.
I am not very familiar with this code therefore I want you to take a look.

When replaceState is called we need to reset originalURI

@@ -11512,16 +11536,17 @@ nsDocShell::AddState(JS::Handle<JS::Valu

     // AddToSessionHistory may not modify mOSHE.  In case it doesn't,
     // we'll just set mOSHE here.
     mOSHE = newSHEntry;

   } else {
     newSHEntry = mOSHE;
     newSHEntry->SetURI(newURI);
+    newSHEntry->SetOriginalURI(newURI);


I should have used mozreview, sorry!

Comment 22

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d695b509c9c1

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

Ah, good catch.  r=me based on https://bugzilla.mozilla.org/attachment.cgi?oldid=8658252&action=interdiff&newid=8664195&headers=1

Comment 24

[Carsten Book [:Tomcat]]

since this is a sec-high bug it needs sec-approval per https://wiki.mozilla.org/Security/Bug_Approval_Process before landing (clearing checkin-needed here)

Comment 25

[Dragana Damjanovic [:dragana]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch does not really show what it is fixing, considering security issue. It fixes the cause,but I think it is hard to figure out the security vulnerability it fixes. Anyway the github issue showing vulnerability is public.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no, 
Which older supported branches are affected by this flaw?
all
If not all supported branches, which bug introduced the flaw?
added in bug 822480, FF31
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Do not have, but not hard to make one. There is a risk, it could need a bit of testing before backporting
How likely is this patch to cause regressions; how much testing does it need?
I prefer that it is running in nightly for a 1-2 weeks (it change a bit history, "go back", code)

Comment 26

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

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

sec-approval+

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c634c30551b0

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c634c30551b0

Comment 29

[Daniel Veditz [:dveditz]]

This should have been backported.

Comment 31

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

Let's get patches for Beta and ESR38.

Comment 32

[Dragana Damjanovic [:dragana]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: This is a sec-high bug and the user impact is described in comment #0.
Fix Landed on Version: 44
Risk to taking this patch (and alternatives if risky): The patch is in m-c since Nightly 44, and 2 regressions had been discovered (these additional patches need to be backported as well - bugs 1211269 and 1213267). This patch changed History entry nsISHEntry that is heavily used, but for more than a month and a half there was no issues. 
String or UUID changes made by this patch: 3 UUIDs have changed
docshell/base/nsIDocShell.idl
docshell/base/nsIDocShellLoadInfo.idl
docshell/shistory/nsISHEntry.idl

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]:822480
[User impact if declined]: This is a sec-high bug and the user impact is described in comment #0.
[Describe test coverage new/current, TreeHerder]: There is a test case provided.
[Risks and why]: The patch is in m-c since Nightly 44, and 2 regressions had been discovered (these additional patches need to be backported as well - bugs 1211269 and 1213267). This patch changed History entry nsISHEntry that is heavily used, but for more than a month and a half there was no issues.
 [String/UUID change made/needed]: 3 UUIDs have changed
docshell/base/nsIDocShell.idl
docshell/base/nsIDocShellLoadInfo.idl
docshell/shistory/nsISHEntry.idl

Comment 33

[Dragana Damjanovic [:dragana]]

I have asked for approval for other 2 bugs as well:
1211269 - this is more important to backport as well
1213267 - very low user impact

Comment 34

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

While this is sec-high and normally I would want to get it into beta, here I'm reluctant because it includes uuid changes, and it's messing with a part of the code that may not be very well understood.  

Is this something we could let ride the train with 44 -- since this is already on 44 and so are the fixes for the couple of regressions it caused?

Comment 35

[Daniel Veditz [:dveditz]]

The tension is that this does look risky given the size of the patch, the scope (IDL changes), and the regressions, but it's also now a public issue with the fix released two months ago in Chrome: http://googlechromereleases.blogspot.ro/2015/09/stable-channel-update.html

Firefox 43 is already 5-6 weeks away, Firefox 44 is two and a half months!

Comment 37

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

This strikes me as something we should take on all branches, including ESR38.

Comment 38

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

OK. If we end up with some addon related crashes with ESR, from the uuid changes, we will respond when that happens. Jorge may want to mention this on the AMO blog once we do land it.  

How should the timing work? Is this something we should land for beta 3, or wait till near the end of the beta cycle when ESR will go out?

Comment 39

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

I'd land it sooner since we're concerned about the changes.

Comment 40

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

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

Let's take this in beta, along with its dependencies.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

I am sorry but I cannot take this patch (27kb) and the patch in bug 1211269 (28kb). This adds too much risks for this release, especially as we don't have any pre-release population.
As the sec fix landed a month ago and the bug has been present for a while, I will take my chance.

Comment 42

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Comment on attachment 8664195 [details] [diff] [review]
> bug_1185256_v2.patch
> 
> Let's take this in beta, along with its dependencies.

landed https://hg.mozilla.org/releases/mozilla-beta/rev/8b8a66145292

Jorge: for your info, this landed on beta togehter with the dependencies just in case this breake some addons

Comment 43

[Jorge Villalobos [:jorgev] (he/him)]

Thanks for the reminder.

Comment 44

[Ada [:adalucinet]]

Reproduced with 42.0 RC build 2 under Mac OS X 10.11.
Verified fixed with 43.0b9 build 2 (Build ID: 20151203163240), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Dragana, please nominate this one for uplift to esr38 as well and I will approve it. Thanks!

Comment 46

[Dragana Damjanovic [:dragana]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch



[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for bug 1246956
User impact if declined: This was a sec-high bug and the user impact is described in comment #0.
Fix Landed on Version: 44
Risk to taking this patch (and alternatives if risky): The patch is in m-c since Nightly 44, and 2 regressions had been discovered (these additional patches need to be backported as well - bugs 1211269 and 1213267). This patch changed History entry nsISHEntry that is heavily used, but for long time there has been no issues. 
String or UUID changes made by this patch: 3 UUIDs have changed
docshell/base/nsIDocShell.idl
docshell/base/nsIDocShellLoadInfo.idl
docshell/shistory/nsISHEntry.idl

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 47

[Dragana Damjanovic [:dragana]]

Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

I need to rebase the patch.

Comment 48

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_esr38.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: the user impact is described in comment #0.
Fix Landed on Version: 44
Risk to taking this patch (and alternatives if risky): low 
String or UUID changes made by this patch: yes

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 49

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8723473 [details] [diff] [review]
bug_1185256_esr38.patch

Sec-high fix that is needed for uplifting fix for bug 1246956 (another sec-high). Let's uplift to ESR38.7.

Comment 50

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jorge, I approved this uplift to esr38 as it is needed for fixing another sec-high issue as mentioned in comment 49. It does have a UUID change, adding a parameter. Is this a breaking change that we should not uplift to esr38.7? Please let me know. Thanks!

Comment 51

[Jorge Villalobos [:jorgev] (he/him)]

This could have some impact on add-ons with binary components, though we wouldn't know which ones are affected, if any. There is some risk of this breaking add-ons or even causing crashes, so it needs to be weighed with the impact of the vulnerability.

We dropped support for binary XPCOM in Firefox 41, so this won't be a problem for ESR 45 and above.

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jorge Villalobos [:jorgev] from comment #51)
> This could have some impact on add-ons with binary components, though we
> wouldn't know which ones are affected, if any. There is some risk of this
> breaking add-ons or even causing crashes, so it needs to be weighed with the
> impact of the vulnerability.
> 
> We dropped support for binary XPCOM in Firefox 41, so this won't be a
> problem for ESR 45 and above.

Thanks Jorge for helping me understand the impact of this change. I am leaning towards wontfixing this issue given the potential addon-compat risk.

Comment 53

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_esr38.patch

We need to uplift this to esr38, because of too much  changes a new review is needed.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724426 [details] [diff] [review]
bug_1185256_esr38.patch

You need to add those new interfaces to the corresponding QI implementations.  Right now QI to those new interfaces will always fail for all three objects (shentry, loadinfo, docshell), no?

r=me with that fixed.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Oh, and we clearly need to actually test this on ESR38...

Comment 56

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_esr38_v2.patch

Comment 57

[Dragana Damjanovic [:dragana]]

The patch diff:

https://bugzilla.mozilla.org/attachment.cgi?oldid=8724426&action=interdiff&newid=8725822&headers=1

Comment 58

[Dragana Damjanovic [:dragana]]

Boris, do you want to take another look?

Comment 59

[Dragana Damjanovic [:dragana]]

I have tested the issue locally.

Comment 60

[Dragana Damjanovic [:dragana]]

Comment on attachment 8725822 [details] [diff] [review]
bug_1185256_esr38_v2.patch

I am putting a flag for :ritu to decide.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Looks good, thanks.

Comment 62

[Dragana Damjanovic [:dragana]]

Comment on attachment 8725822 [details] [diff] [review]
bug_1185256_esr38_v2.patch

need to change one thing. Locally it is not complaining but try is complaining :(

Comment 63

[Dragana Damjanovic [:dragana]]

Attachment: bug_1185256_esr38.patch

I needed to rename a function. Try was complaining. Locally it was building ok.

Boris, do you want to take another look? Sorry for all this ...

Comment 64

[Dragana Damjanovic [:dragana]]

Comment on attachment 8725849 [details] [diff] [review]
bug_1185256_esr38.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 65

[Boris Zbarsky [:bzbarsky]]

r=me on that last diff, fwiw.

Comment 66

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8725849 [details] [diff] [review]
bug_1185256_esr38.patch

This is a pretty big code change but Dan suggested it is worth the risk. Taking it in esr38.7

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/532544c91db7