Bug 1185256 (CVE-2015-7207)

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

RESOLVED FIXED in Firefox 43

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: pvtolkien, Assigned: dragana)

Tracking

(4 keywords)

39 Branch
mozilla44
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox43+ verified, firefox44+ fixed, firefox-esr3845+ fixed)

Details

(Whiteboard: [adv-main43+][adv-esr38.7+], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Severity: normal → major
Component: Untriaged → Security
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 1

4 years ago
A tracking bug for W3C has been created: https://github.com/w3c/resource-timing/issues/29
(Reporter)

Comment 2

4 years ago
Firefox specific and minimized PoC: http://vwzq.net/lab/xreadurl/ff.html
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)?
Component: Security → DOM
Product: Firefox → Core
Matt can't reproduce on 39 either. What are we doing differently than the reported
Flags: needinfo?(pvtolkien)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
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.
Flags: needinfo?(pvtolkien)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

4 years ago
Group: core-security → dom-core-security
Valentin, is this something you could look into? Thanks.
Flags: needinfo?(valentin.gosu)
I'll take a look.
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)

Comment 10

4 years ago
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.
Assignee: valentin.gosu → nobody
Flags: needinfo?(overholt)

Comment 11

4 years ago
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.
Assignee: nobody → dd.mozilla
Flags: needinfo?(overholt)
I've also asked Kyle (who doesn't know this code AFAIK) to help; maybe for reviews if nothing else?
(Assignee)

Comment 13

4 years ago
Posted patch bug_1185256_v1.patch (obsolete) — Splinter Review
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.
Attachment #8658252 - Flags: feedback?(khuey)
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.
Attachment #8658252 - Flags: feedback?(khuey)
(Assignee)

Comment 15

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

Let's see if I have the right Kyle this time.
Attachment #8658252 - Flags: feedback?(kyle)
Yeah, sorry, I meant qDot.
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
Attachment #8658252 - Flags: feedback?(kyle) → feedback+
(Assignee)

Comment 18

4 years ago
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.
Attachment #8658252 - Flags: review?(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 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... :(
Attachment #8658252 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 21

4 years ago
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!
Attachment #8658252 - Attachment is obsolete: true
Attachment #8664195 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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)
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
(Assignee)

Comment 25

4 years ago
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)
Flags: needinfo?(dd.mozilla)
Attachment #8664195 - Flags: sec-approval?
Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

sec-approval+
Attachment #8664195 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c634c30551b0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: dom-core-security → core-security-release
Depends on: 1213267
This should have been backported.
Blocks: 822480
Keywords: regression
Duplicate of this bug: 1221681
Let's get patches for Beta and ESR38.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 32

4 years ago
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
Flags: needinfo?(dd.mozilla)
Attachment #8664195 - Flags: approval-mozilla-esr38?
Attachment #8664195 - Flags: approval-mozilla-beta?
(Assignee)

Comment 33

4 years ago
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
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?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
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!
Flags: needinfo?(dveditz)
Flags: sec-bounty?
This strikes me as something we should take on all branches, including ESR38.
Flags: needinfo?(abillings)
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?
Flags: needinfo?(abillings)
I'd land it sooner since we're concerned about the changes.
Flags: needinfo?(abillings)
Comment on attachment 8664195 [details] [diff] [review]
bug_1185256_v2.patch

Let's take this in beta, along with its dependencies.
Attachment #8664195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8664195 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
(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
Flags: needinfo?(jorge)
Thanks for the reminder.
Flags: needinfo?(jorge)
Keywords: addon-compat
Flags: sec-bounty? → sec-bounty+
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
Whiteboard: [adv-main43+]
Alias: CVE-2015-7207
Group: core-security-release
Dragana, please nominate this one for uplift to esr38 as well and I will approve it. Thanks!
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 46

3 years ago
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.
Flags: needinfo?(dd.mozilla)
Attachment #8664195 - Flags: approval-mozilla-esr38- → approval-mozilla-esr38?
(Assignee)

Comment 47

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

I need to rebase the patch.
Attachment #8664195 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 48

3 years ago
Posted patch bug_1185256_esr38.patch (obsolete) — Splinter Review
[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.
Attachment #8723473 - Flags: approval-mozilla-esr38?
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.
Attachment #8723473 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
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!
Flags: needinfo?(jorge)
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.
Flags: needinfo?(jorge)
(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.
(Assignee)

Comment 53

3 years ago
Posted patch bug_1185256_esr38.patch (obsolete) — Splinter Review
We need to uplift this to esr38, because of too much  changes a new review is needed.
Attachment #8723473 - Attachment is obsolete: true
Attachment #8724426 - Flags: review?(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.
Attachment #8724426 - Flags: review?(bzbarsky) → review+
Oh, and we clearly need to actually test this on ESR38...
(Assignee)

Comment 56

3 years ago
Posted patch bug_1185256_esr38_v2.patch (obsolete) — Splinter Review
Attachment #8724426 - Attachment is obsolete: true
Attachment #8725822 - Flags: review+
(Assignee)

Comment 58

3 years ago
Boris, do you want to take another look?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 59

3 years ago
I have tested the issue locally.
(Assignee)

Comment 60

3 years ago
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.
Attachment #8725822 - Flags: approval-mozilla-esr38?
Looks good, thanks.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 62

3 years ago
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 :(
Attachment #8725822 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 63

3 years ago
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 ...
Attachment #8725822 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8725849 - Flags: review+
(Assignee)

Comment 64

3 years ago
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.
Attachment #8725849 - Flags: approval-mozilla-esr38?
r=me on that last diff, fwiw.
Flags: needinfo?(bzbarsky)
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
Attachment #8725849 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [adv-main43+] → [adv-main43+][adv-esr38.7+]
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.