1414425 - Resource Timing API leaks URL after subframe navigation again

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

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

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36

Steps to reproduce:

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


Actual results:

Cross-origin URL after redirect is leaked.


Expected results:

Bug 1408990 seems to be fixed on Nightly, but PoC on https://bugzilla.mozilla.org/show_bug.cgi?id=1408990#c5 still works (i.e. subframe redirect with meta refresh). This should also be fixed. I will try to see if I have other navigation that triggers same bug.

Now you can coordinate with Chrome too :)

Comment 1

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

Attachment: Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: 4Kn6P4hnx74

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925512 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

>+    // Since the load is triggered by the meta tag in the current document,
>+    // we need to set the triggering principal to reflect that.

What guarantees it's the current document?

It would be better, imo, if the principal were stored at the point when we call SetupRefreshURIFromHeader().

Also, we have explicit CheckLoadURI checks in that code, that can maybe go away if we set a useful triggering principals here....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and we desperately need tests here.  :(

Comment 4

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> Comment on attachment 8925512 [details] [diff] [review]
> Make sure the appropriate triggeringPrincipal is set for a meta refresh
> 
> >+    // Since the load is triggered by the meta tag in the current document,
> >+    // we need to set the triggering principal to reflect that.
> 
> What guarantees it's the current document?

OK, so I've noticed SetupRefreshURIFromHeader can also get called from nsDocument and txBufferingHandler.cpp
And this is why it could also be triggered by a different document?

> It would be better, imo, if the principal were stored at the point when we
> call SetupRefreshURIFromHeader().

So in nsDocShell::SetupRefreshURI I should pass in the principal of the current document, and create the loadInfo at that point?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> And this is why it could also be triggered by a different document?

Sort of.  Mainly it's because I'm not willing to bet my life that the document that calls it is the current document of the docshell.

> So in nsDocShell::SetupRefreshURI I should pass in the principal of the current document

No, you should pass the principal of the channel's result.  Which is what we do already.

That is, SetupRefreshURIFromHeader already has the principal the refresh uri string comes from and that should be considered the triggering principal.  We already use it for the CheckLoadURIWithPrincipal() call in that function.  We should just make sure we stash that principal away and use it as the triggering principal for the load.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

AFAICT, all supported branches are affected.

Comment 7

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

Attachment: Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: 4Kn6P4hnx74

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926361 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

>+      * @param principal the associated principal
>+      * @param principal the associated principal

"associated principal" is meaningless.  Please document what the argument actually means: that it will be used as the triggering principal for the load.

Also, please document that null is allowed, if it is, and what the behavior is when null is passed.

Comment 9

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

Attachment: Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: 4Kn6P4hnx74

Comment 10

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

Attachment: bug1414425-tests.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926440 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

>+      *   May be null, in which case a principal will be built based on the
>+      *   referrer URI, or will use the system principal when there is none.

What referrer URI?  This API doesn't take a referrer URI...

Comment 12

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> Comment on attachment 8926440 [details] [diff] [review]
> Make sure the appropriate triggeringPrincipal is set for a meta refresh
> 
> >+      *   May be null, in which case a principal will be built based on the
> >+      *   referrer URI, or will use the system principal when there is none.
> 
> What referrer URI?  This API doesn't take a referrer URI...

We get the referrer from the docshell:
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/docshell/base/nsDocShell.cpp#6940-6942

And we use that if there is no triggeringPrincipal
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/docshell/base/nsDocShell.cpp#1571-1579

Indeed, there is no referrer in the API, which makes it confusing.
Should I say that it "uses the referrer of the original docshell load"?

Comment 13

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

Won't fixing for 57. This is too late.

Comment 14

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

Attachment: Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: 4Kn6P4hnx74

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Should I say that it "uses the referrer of the original docshell load"?

Yes, or more precisely "uses the referrer of the thing currently loaded in the docshell", right?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926888 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

>+++ b/browser/base/content/tab-content.js

OK, so now that we understand the setup here, this code looks like a security bug.  It will use the referrer of the page in the docshell as the triggering principal, whereas it should probably be using that page's principal or so...  Please file a followup bug on this?

>+++ b/browser/base/content/test/general/browser_refreshBlocker.js

Similar.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926441 [details] [diff] [review]
bug1414425-tests.patch

> SpecialPowers.pushPrefEnv({"set": [["dom.enable_resource_timing", true]]}, start);

If the iframe/object loads have already started, or finished, by now, will they then appear in the resource timing somehow?  Or do we skip capturing it at all when the pref is not set?

This is what the other test was trying to avoid with its subwindow thing...

>+  setTimeout(function() {

Why do you need the timeout?  Why can't you just have the tests phone home via postMessage when they are done loading?

Comment 18

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

Comment on attachment 8926888 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, unless one was also aware of the resource timing leak.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, you can figure out that meta refreshes used the wrong principal for the load.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Applies cleanly on release. I have a patch ready for esr52.

How likely is this patch to cause regressions; how much testing does it need?
Low risk of regressions.

Comment 19

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

Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on other branches so branch patches should be made and nominated at that time as well.

Comment 20

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

Attachment: [esr] Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: 4Kn6P4hnx74

Comment 21

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

Comment on attachment 8931457 [details] [diff] [review]
[esr] Make sure the appropriate triggeringPrincipal is set for a meta refresh

[Approval Request Comment]
User impact if declined: 
cross origin URL leak with meta redirected iframes.

Fix Landed on Version:
firefox 59 (will land on November 28)

Risk to taking this patch (and alternatives if risky):
Changes IDL interface nsIRefreshURI which might break addons (NoScript and FireFTP seem to be using it)
Alternatively we could make the extra arguments be optional. So we don't break them.

String or UUID changes made by this patch: 
None.

Comment 22

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

(In reply to Valentin Gosu [:valentin] from comment #21)
> Changes IDL interface nsIRefreshURI which might break addons (NoScript and
> FireFTP seem to be using it)
> Alternatively we could make the extra arguments be optional. So we don't
> break them.

Do you think we should do this?

Comment 23

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

Attachment: bug1414425-tests.patch

I kept the setTimeout as postMessage from file_empty.html didn't work. The parent page wouldn't receive the message, although if I tried it from the developer console, it worked.

Comment 24

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

(In reply to Valentin Gosu [:valentin] from comment #22)
> (In reply to Valentin Gosu [:valentin] from comment #21)
> > Changes IDL interface nsIRefreshURI which might break addons (NoScript and
> > FireFTP seem to be using it)
> > Alternatively we could make the extra arguments be optional. So we don't
> > break them.
> 
> Do you think we should do this?

Also, I noticed that the patch introduces an error on ESR, that does not occur on central or beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41df815ddc6aa7c82109ff9d6b76151f350b56c&selectedJob=147596370

It seems that this makes the iframe same-origin with a javascript: URI
Any idea where I should look to fix this issue?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Can you please attach the non-working version with postMessage?

Comment 26

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

Attachment: bug1414425-tests.patch

I don't know what I was doing differently the first time, or if I needed to clobber my build, but it works now.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/229546747cbf04cf71a235275d7c2eb2520a298b

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/229546747cbf

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 30

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

Comment on attachment 8926888 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

Approval Request Comment
[Feature/Bug causing the regression]:
has always existed
[User impact if declined]:
cross origin URL leak with meta redirected iframes.
[Is this code covered by automated tests?]:
Yes. Tests are awaiting review.
[Has the fix been verified in Nightly?]:
Just landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No
[Why is the change risky/not risky?]:
No. This patch just makes sure the appropriate principal is used when for a meta refresh.
[String changes made/needed]:
None.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Do you think we should do this?

For backporting to ESR, please make the argument optional, yes.

Comment 32

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31)
> > Do you think we should do this?
> 
> For backporting to ESR, please make the argument optional, yes.

Any thoughts on Comment 24 ?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> It seems that this makes the iframe same-origin with a javascript: URI

Which iframe?  In general, javascript: URIs lead to same-origin documents, yes....

> Any idea where I should look to fix this issue?

I'm not quite understanding what the issue is.  Is it that the iframe ends up same-origin when the test expects it to?  Is it that it ends up with a different url from the one the test expects?  Something else?  Which of the subtests in https://searchfox.org/mozilla-central/source/docshell/test/file_bug475636.sjs are we looking at here?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932225 [details] [diff] [review]
bug1414425-tests.patch

>+    window.parent.postMessage("done", "*");

Might be better to do this onload, no from inline script...

r=me with that.

Comment 35

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #33)
> I'm not quite understanding what the issue is.  Is it that the iframe ends
> up same-origin when the test expects it to?  Is it that it ends up with a
> different url from the one the test expects?

The test expects meta redirects to JS URLs not to be followed, but this patch changes that.

> Which of the subtests in
> https://searchfox.org/mozilla-central/source/docshell/test/file_bug475636.
> sjs are we looking at here?

https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/docshell/test/file_bug475636.sjs#52,64

Comment 36

[Boris Zbarsky [:bzbarsky]]

> The test expects meta redirects to JS URLs not to be followed, but this patch changes that.

I see.  But only on ESR?  Does the patch not change behavior on tip, or does tip expect them to be followed?

Comment 37

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #36)
> > The test expects meta redirects to JS URLs not to be followed, but this patch changes that.
> 
> I see.  But only on ESR?  Does the patch not change behavior on tip, or does
> tip expect them to be followed?

Only on ESR from what I can tell. Beta and m-c are OK. I assume something else has changed in the meantime.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Beta and m-c are OK.

Right, but in which direction are they "OK"?  That is, why exactly does the test keep passing?

> I assume something else has changed in the meantime.

Sure.  But if we're backporting a security fix and seeing different behavior, we really need to figure out what exactly has changed and why to make sure we're not introducing other security issues...

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8926888 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

Fix a sec-high. Beta58+ & ESR52+.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

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

Ryan, the patch for ESR actually wasn't ready, and will probably cause a test to fail, and break some extensions.
I'm working right now figuring out the test.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8931457 [details] [diff] [review]
[esr] Make sure the appropriate triggeringPrincipal is set for a meta refresh

No worries, I probably won't try to uplift that for another week or so anyway (we tend to wait awhile on those to avoid making life more complicated for any 52.5.x dot-releases).

Setting the current patch to obsolete for now just in case, though :).

Comment 43

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #38)
> Right, but in which direction are they "OK"?  That is, why exactly does the
> test keep passing?
> > I assume something else has changed in the meantime.

I've been debugging the test, and it seems that it's actually the data: URI redirect that is failing.

It seems to be because of https://blog.mozilla.org/security/2017/10/04/treating-data-urls-unique-origins-firefox-57/
If I set the security.data_uri.unique_opaque_origin pref to false I get the same failure in m-c.

> Sure.  But if we're backporting a security fix and seeing different
> behavior, we really need to figure out what exactly has changed and why to
> make sure we're not introducing other security issues...

So, the difference here is that now when we get to LoadURI, principalToInherit will not be null.
I can fix the test by passing LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL to LoadURI - in which case principalToInherit will be set to a nsNullPrincipal instead.
I'm not sure yet if the difference principalToInherit == null vs principalToInherit == nsNullPrincipal matters.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> it seems that it's actually the data: URI redirect that is failing

That makes a lot more sense.  ;)

> I can fix the test by passing LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL to LoadURI - in which case 

Yes, that would be a good idea.  Have we checked whether trying to do a meta refresh to javascript: actually runs the script?  I'd somewhat hope not, and this is the only other case that would be affected by principal inheritance.  That said, it's not clear to me _why_ it doesn't, after your changes.

> I'm not sure yet if the difference principalToInherit == null vs principalToInherit == nsNullPrincipal matters.

The key here is that nsDocShell::ForceRefreshURI does loadInfo->SetPrincipalIsExplicit(true).

So on tip, we land in LoadURI, set principalToInherit to the triggering principal.  If this comes out null (the old state of things), then because principalIsExplicit is true we leave inheritPrincipal at the value we got from the loadinfo, which in this case is false.  Therefore we do not add the INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL flag.  Then in InternalLoad, aPrincipalToInherit is null, INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is not set, so we leave principalToInherit as null.  The upshot is that we don't set it on the loadinfo, nor do we set the SEC_FORCE_INHERIT_PRINCIPAL loadinfo flag.  Nor do we set any of the *_DATA_INHERITS loadinfo flags.  Then on the other end, nsScriptSecurityManager::GetChannelResultPrincipal ends up not inheriting anything, falling back on GetChannelURIPrincipal, which tries to create a codebase principal, fails to generate an origin (for a data: URI), and falls back to an nsNullPrincipal.  So the end result is the same as if you just inherit a nullprincipal.

On esr52... looks like the same thing actually.  I guess the most recent changes to this code predated 52, luckily.

Comment 45

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #44)
> > it seems that it's actually the data: URI redirect that is failing
> 
> That makes a lot more sense.  ;)
> 
> > I can fix the test by passing LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL to LoadURI - in which case 
> 
> Yes, that would be a good idea.  Have we checked whether trying to do a meta
> refresh to javascript: actually runs the script?  I'd somewhat hope not, and
> this is the only other case that would be affected by principal inheritance.
> That said, it's not clear to me _why_ it doesn't, after your changes.

Redirect to javascript URIs seem to be explicitly blocked by this check:
https://dxr.mozilla.org/mozilla-esr52/rev/f6216ea8b8fc4ff0c22f95e0aae6c4df2a5e6b5f/docshell/base/nsDocShell.cpp#7060,7063-7064

I've checked the test, and this is the branch we take.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> Redirect to javascript URIs seem to be explicitly blocked by this check:

Aha.  Yes, that would do it.

Comment 47

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

Attachment: [esr52] Make sure the appropriate triggeringPrincipal is set for a meta refresh

MozReview-Commit-ID: GgKVQwkvpFt

Comment 48

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

Try job for ESR patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71bfd5b487ae99d10c1c4ca9a7a9ca78da479ba0&selectedJob=148932812

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933522 [details] [diff] [review]
[esr52] Make sure the appropriate triggeringPrincipal is set for a meta refresh

>+  LoadURI(aURI, loadInfo, nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL, true);

This part should also land on m-c, I'd think.

r=me

Comment 50

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

Daniel, should we coordinate with Chrome on disclosing this?
I don't have access to the Chrome bug so I'm not sure on how we should proceed.

Comment 51

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca478b073fb789344a68a47af6340f2349e362b4

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5623e01e63a8

Please be sure to get that follow-up patch landed on m-c (and nominated for Beta uplift if necessary).

Comment 53

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

Thanks Ryan. Already filed it: bug 1416307.

Comment 54

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

Dan, what is the current status of this on Chrome's side?

Comment 55

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

Chrome fixed it in 63. Safari fixed it internally but not landed yet (They just released to beta). Edge is planning to release a patch on March.

Comment 56

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

Ok. We'll hold off on disclosure until everyone ships.

Comment 58

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  58.0a1 (2017.11.03) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 52.6.0esr, 58.0 and  59.0a1 on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.

Comment 59

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

All browsers fixed the issue. Thank you very much for the coordination :)