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.

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

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

taking it.

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.

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

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.

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?

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?

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

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

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

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

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

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

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

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

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

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.

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.

(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 valentin.gosu@gmail.com Thanks!

MozReview-Commit-ID: 7o8XKHioP1p

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9e841ff8544053f184d2e425500d5dd13ee14d This grafts cleanly to Beta and ESR52. Just needs approval requests.

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

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

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

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

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.

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

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

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?

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

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.

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