1408990 - (CVE-2017-7830) Resource Timing API leaks URL after subframe navigation

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

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

Steps to reproduce:

1. Go to https://attack.shhnjk.com/resource_check.html
2. Wait for 3 seconds


Actual results:

Cross-origin URL after redirect is leaked.


Expected results:

This should not be leaked.

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

Seems very similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1185256 but my PoC does not use cache. Is it regression?

Comment 2

[Daniel Veditz [:dveditz]]

Dragana: you fixed bug 1185256, is this similar and in your area?

Comment 3

[Dragana Damjanovic [:dragana]]

taking it.

Comment 4

[Dragana Damjanovic [:dragana]]

This problem are not redirects. The redirects are handle correctly, i.e. we do not show uri of redirects only the originalURI.
The resource at the address indicated by iframe changes the location and reloads the iframe, itself.

We can detect that as: a "load_replace"(I  need to check the correct name of the flag) of a page that is triggered by the iframe context.

Comment 5

[Jun (Request needinfo for comment, I ignore other updates)]

Interesting.
So, with server side redirect:

https://test.shhnjk.com/iframer.php?url=//vuln.shhnjk.com/location.php?url=//shhnjk.com
Console: performance.getEntriesByType("resource")

Redirected URL is not leaked.

But if framed content had meta refresh or script like location.replace:

https://test.shhnjk.com/iframer.php?url=//vuln.shhnjk.com/meta.php?url=//shhnjk.com
Console: performance.getEntriesByType("resource")

Redirected URL is leaked.

I still feel that this is a leak since some website uses script to redirect the page (Bing as an example).

Comment 6

[Dragana Damjanovic [:dragana]]

I could detect that referrerUri is different(checking only for cross-origin is not enough, we will not leak anything but it is in my opinion not correct) from the owning doc uri and that the load type is nsIDocShellLoadInfo::loadStopContentAndReplace. This should be sufficient, but I need to think if it is going to catch cases that we do not want to block... probably it is ok to block all such cases.... all in all I think from HttpChannelChild I cannot properly detect that is iframe reloading itself

Anyway I think it is better to detect this in dom(probably in dome/base/Location.cpp) instead of necko.

bz, I am not sure how to do this in dom properly. I will need some help, or maybe someone from dome team could take it this bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The first question is: What does the spec say to do?

Does it have to be a "redirect", even?  What happens if user clicks a link in the iframe?  If I do a simple test:

1) Load data:text/html,<iframe src="http://example.com">
2) Click the "More information" link in the iframe (probably need to scroll down to see it).
3) Examine performance.getEntriesByType("resource")[1].name in the console

I get "http://www.iana.org/domains/example".

Using a testcase not affected by X-frame-options, like so:

1) Load data:text/html,<iframe%20src="http://web.mit.edu"></iframe>
2) Scroll down to the "admissions" option in the menu and click it.
3) Click "undergrad".
4) Examine performance.getEntriesByType("resource")[1].name

then I see "http://www.mitadmissions.org/" in Firefox and Safari.  So we're leaking that information cross-site.  This seems like a fundamental bug in the resource timing spec that needs to be fixed in the spec, assuming we have per-spec behavior here.  If we don't have per-spec behavior, we should fix it to be per-spec and add tests as needed.

For what it's worth, using my mit.edu testcase in Chrome and Edge performance.getEntriesByType("resource")[1] is undefined and performance.getEntriesByType("resource").length == 1.

> Anyway I think it is better to detect this in dom(probably in dome/base/Location.cpp)

That wouldn't address the link click case.

I believe valentin has been generally following the webperf specs?

Comment 8

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

From what I can tell, there isn't anything in the spec specifically about this case. That left room to implementation issues such as this.

We do correctly report the first load of the iframe. However, a reload of the iframe triggered by JS or the meta tag or even user activity will cause the navigations to be shown in the parent document's performance.
Boris, do we have a way of identifying these types of loads triggered from the iframe, as opposed to loads triggered from the parent document?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> there isn't anything in the spec specifically about this case

Um... does the spec not define a processing model that says exactly when things get added to resource timing?

> do we have a way of identifying these types of loads triggered from the iframe, as opposed to loads
> triggered from the parent document?

Identifying them at what point?

It seems like for iframes, we only want to add things to resource timing if the load is triggered by modification of the iframe's "src" attribute, right?  We certainly know at the point where that happens that this is what's triggering the load, but it sounds like the information is needed somewhere else?

Again, ideally we would just implement the spec's processing model and this would fall out naturally...

Comment 10

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> > there isn't anything in the spec specifically about this case
> 
> Um... does the spec not define a processing model that says exactly when
> things get added to resource timing?
> 

It does, but the case of iframe navigation isn't explicitly specified.

https://w3c.github.io/resource-timing/#resources-included
"Sub-resources requested by the IFRAME document will be included in the IFRAME document's Performance Timeline and not the parent document's Performance Timeline. "

> > do we have a way of identifying these types of loads triggered from the iframe, as opposed to loads
> > triggered from the parent document?
> 
> Identifying them at what point?

It would be great if we already had a flag set on the channel, so that we'd only need to change HttpBaseChannel::GetPerformance().

> 
> It seems like for iframes, we only want to add things to resource timing if
> the load is triggered by modification of the iframe's "src" attribute,
> right?  

Correct.

> We certainly know at the point where that happens that this is
> what's triggering the load, but it sounds like the information is needed
> somewhere else?

I can see us performing this check either in HttpBaseChannel::GetPerformance() or PerformanceMainThread::AddEntry()

> Again, ideally we would just implement the spec's processing model and this
> would fall out naturally...

I agree, but the spec doesn't seem to specify much regarding iframe interaction apart from the snippet I quoted above.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> It does, but the case of iframe navigation isn't explicitly specified.

I'm not sure I follow.  If there's an actual step-by-step processing model, then it explicitly defines what things are or are not inserted in the resource timing list; you just step through it.

> It would be great if we already had a flag set on the channel, so that we'd only need to change HttpBaseChannel::GetPerformance().

Hmm.  So the issue is that the loadingDocument is the parent doc for all navigations in an iframe, right?  Should it really be, if the navigations are triggered by the subframe itself?

Assuming it is, we can either add a channel flag, or add a new loadinfo member for which document is "really" responsible for the load.  In either case, we'd need to thread that information down to where docshell creates the channel, starting from ... well, it depends on how fake-plugin <object> should behave.  But probably from nsFrameLoader::ReallyStartLoadingInternal though if we want to we could restrict that to cases in which mURIToLoad comes from nsFrameLoader::LoadFrame.

> I agree, but the spec doesn't seem to specify much regarding iframe interaction apart from the snippet I quoted above.

Then the spec is broken and we need a spec issue filed.

Comment 12

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> > It does, but the case of iframe navigation isn't explicitly specified.
> 
> I'm not sure I follow.  If there's an actual step-by-step processing model,
> then it explicitly defines what things are or are not inserted in the
> resource timing list; you just step through it.
> 
> > It would be great if we already had a flag set on the channel, so that we'd only need to change HttpBaseChannel::GetPerformance().
> 
> Hmm.  So the issue is that the loadingDocument is the parent doc for all
> navigations in an iframe, right?  Should it really be, if the navigations
> are triggered by the subframe itself?

The loadingDocument will correctly point to the iframe document for all resources loaded by the iframe, but when a navigation happens in the iframe, the loadingDocument is still that of the parent.

> Assuming it is, we can either add a channel flag, or add a new loadinfo
> member for which document is "really" responsible for the load.  In either
> case, we'd need to thread that information down to where docshell creates
> the channel, starting from ... well, it depends on how fake-plugin <object>
> should behave.  But probably from nsFrameLoader::ReallyStartLoadingInternal
> though if we want to we could restrict that to cases in which mURIToLoad
> comes from nsFrameLoader::LoadFrame.

Thanks for the tip. Dragana, do you still want to work on this?
 
> > I agree, but the spec doesn't seem to specify much regarding iframe interaction apart from the snippet I quoted above.
> 
> Then the spec is broken and we need a spec issue filed.

Agreed. I'll file an issue as soon as we ship a fix.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You may want to coordinate with Apple, given Safari has behavior that matches ours...

Comment 14

[Dragana Damjanovic [:dragana]]

Valentin, if you want to take over just go a head, if not I will continue. As you wish :)

Comment 15

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

Attachment: bug1408990.patch

What do you think about this approach? It doesn't fix the overall resource-timing issue on same origin iframes, but it does prevent the cross origin leak

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922889 [details] [diff] [review]
bug1408990.patch

Hmm.  It does band-aid the immediate problem, so might be worth taking, with some code comments, including on branches, and filing a followup to do the general "don't add navigations not caused by src changes" thing.  Assuming that's what Edge and Chrome do; we would need to add tests for that...

Furthermore, something like this might be needed anyway, to address other information leaks.  For example, consider a document at origin X that loads a stylesheet at origin Y.  Should background image, font, etc loads from that stylesheet show up in the resource timing of the document?  Looks to me like this change would filter them out.  Again, need to test what other browsers do in that situation, and check what the spec says, possibly raising spec issues in the process.

Comment 17

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

Hi Jun,

Have you also reported this issue to Apple? If so, could you provide the bug number, please?

Thanks!

Comment 18

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

Attachment: Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

Passes the web-platform and mochitests we have for resource timing. Working on other tests right now.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922948 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

The comment needs to explain _why_ the check is being done, not what the check is.  What the check is is obvious from the code.

Comment 20

[Jun (Request needinfo for comment, I ignore other updates)]

Here is a Webkit bug.

https://bugs.webkit.org/show_bug.cgi?id=178433

If you don't have permission to see it, then just let me know the email you want to be CCed and I will add it.

Comment 21

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

(In reply to Jun from comment #20)
> Here is a Webkit bug.
> 
> https://bugs.webkit.org/show_bug.cgi?id=178433
> 
> If you don't have permission to see it, then just let me know the email you
> want to be CCed and I will add it.

My email is [e-mail] Thanks!

Comment 22

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

Attachment: Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

MozReview-Commit-ID: 7o8XKHioP1p

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

r=me.  Thank you!

Please don't forget sec-approval and the followup bugs to:

1) Add tests.
2) Fix the stylesheet issue I mentioned, assuming it's an issue.

as well as the spec bits...

Comment 24

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

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not difficult.

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

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
Since initial landing of resource timing.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Applies without conflicts on release and esr52.

How likely is this patch to cause regressions; how much testing does it need?
Very small risk of regressions. Resource timing has a fair number of tests.

Comment 25

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

Release Management, I'd like to take this on trunk and then Beta and ESR52. I want to run it by you first. The patch is very small.

Comment 26

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

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

Makes sense. 
Let's take this for the beta 13 build on Monday and for ESR 52.5.0 as well.  I'll check back Sunday night.

Comment 27

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

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

sec-approval+ for trunk.
Please nominate patches for Beta and ESR52.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

This grafts cleanly to Beta and ESR52. Just needs approval requests.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

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

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
security-high

User impact if declined: 
Cross origin leak of potentially sensitive URLs via iframes.

Fix Landed on Version:
About to land on trunk - Fx58.

Risk to taking this patch (and alternatives if risky): 
Very low risk. It just adds an extra security check.

String or UUID changes made by this patch: 
None

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

Comment 31

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

Comment on attachment 8922996 [details] [diff] [review]
Only add the entry to the performance object if the loading document's principal is the same as the triggering principal

Considering comment 25 as sec-approval+ from Al.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa8f74f0ad60

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/04e3b5c1f0b2

Comment 34

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #31)

> Considering comment 25 as sec-approval+ from Al.

Yes, this slipped through when I looked this weekend.

Comment 35

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue on Fx56.
Verified fixed on Fx57.0b14 and Fx52.5.0esr.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Daniel, what's the procedure to make sure we don't disclose too much here before Safari ships a fix?

Comment 37

[Daniel Veditz [:dveditz]]

We've already said https://www.mozilla.org/en-US/security/advisories/mfsa2017-24/#﷒0﷓ but we can annotate the whiteboard to make sure we don't unhide the bug prematurely.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Argh.  What should we have done here to ensure that we didn't over-disclose in the advisory until they shipped?  :(

Comment 39

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #38)
> Argh.  What should we have done here to ensure that we didn't over-disclose
> in the advisory until they shipped?  :(

Had someone ask about it before we shipped and released the advisory. Jun and I have already discussed this a bit in email at some length.

Comment 40

[Boris Zbarsky [:bzbarsky]]

OK.  I was mentioned in the bug before then, but I guess it should have been explicitly raised as part of the sec-approval request or something.  Valentin, can you please do that next time you're doing a security fix that's supposed to be coordinated with someone else?

Comment 41

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #40)
> OK.  I was mentioned in the bug before then, but I guess it should have been
> explicitly raised as part of the sec-approval request or something. 
> Valentin, can you please do that next time you're doing a security fix
> that's supposed to be coordinated with someone else?

I'm sorry I missed that. I'll remember it for next time.

Comment 42

[Daniel Veditz [:dveditz]]

Although the default BMO UI makes it less visible, the Whiteboard is still the best place to raise this. As part of writing advisories or unhiding bugs we check and update the whiteboard--it won't be missed. Even a simple "Embargo" or "Keep hidden" will force us to look for details in the comment that added that whiteboard notation.

Comment 43

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 58.0a1 (BuildId:20171016220427).

This issue is no longer reproducible using Firefox 57.0.2 (BuildId:20171217111436), 58.0b12 (BuildId:20171215145727), 59.0a1 (BuildId:20171217094207) and 52.5.2 esr (BuildId:20171206101620) on Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit