Resource Timing API leaks URL after subframe navigation again

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: s.h.h.n.j.k, Assigned: valentin.gosu)

Tracking

({sec-high})

unspecified
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox-esr5258+ verified, firefox56 wontfix, firefox57 wontfix, firefox58+ verified, firefox59+ verified)

Details

(Whiteboard: [necko-triaged][post-critsmash-triage])

Attachments

(3 attachments, 6 obsolete attachments)

13.38 KB, patch
bzbarsky
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
18.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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 :)
Keywords: sec-high
Group: firefox-core-security → core-security
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
MozReview-Commit-ID: 4Kn6P4hnx74
Attachment #8925512 - Flags: review?(bzbarsky)
Assignee: nobody → valentin.gosu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [necko-triaged]
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....
Attachment #8925512 - Flags: review?(bzbarsky) → review-
Oh, and we desperately need tests here.  :(
(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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
AFAICT, all supported branches are affected.
Group: core-security → network-core-security
MozReview-Commit-ID: 4Kn6P4hnx74
Attachment #8926361 - Flags: review?(bzbarsky)
Attachment #8925512 - Attachment is obsolete: true
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.
Attachment #8926361 - Flags: review?(bzbarsky) → review-
MozReview-Commit-ID: 4Kn6P4hnx74
Attachment #8926440 - Flags: review?(bzbarsky)
Attachment #8926361 - Attachment is obsolete: true
Posted patch bug1414425-tests.patch (obsolete) — Splinter Review
Attachment #8926441 - Flags: feedback?(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...
Flags: needinfo?(valentin.gosu)
(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"?
Flags: needinfo?(valentin.gosu)
Won't fixing for 57. This is too late.
MozReview-Commit-ID: 4Kn6P4hnx74
Attachment #8926888 - Flags: review?(bzbarsky)
Attachment #8926440 - Attachment is obsolete: true
Attachment #8926440 - Flags: review?(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 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.
Attachment #8926888 - Flags: review?(bzbarsky) → review+
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?
Attachment #8926441 - Flags: feedback?(bzbarsky) → feedback-
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.
Attachment #8926888 - Flags: sec-approval?
Blocks: 1416307
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.
Whiteboard: [necko-triaged] → [necko-triaged[checkin on 11/28]
Attachment #8926888 - Flags: sec-approval? → sec-approval+
Priority: -- → P1
Whiteboard: [necko-triaged[checkin on 11/28] → [necko-triaged][checkin on 11/28]
MozReview-Commit-ID: 4Kn6P4hnx74
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.
Attachment #8931457 - Flags: approval-mozilla-esr52?
(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?
Flags: needinfo?(bzbarsky)
Posted patch bug1414425-tests.patch (obsolete) — Splinter Review
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.
Attachment #8931991 - Flags: review?(bzbarsky)
Attachment #8926441 - Attachment is obsolete: true
(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?
Can you please attach the non-working version with postMessage?
Flags: needinfo?(bzbarsky) → needinfo?(valentin.gosu)
I don't know what I was doing differently the first time, or if I needed to clobber my build, but it works now.
Attachment #8932225 - Flags: review?(bzbarsky)
Attachment #8931991 - Attachment is obsolete: true
Attachment #8931991 - Flags: review?(bzbarsky)
https://hg.mozilla.org/integration/mozilla-inbound/rev/229546747cbf04cf71a235275d7c2eb2520a298b
Flags: in-testsuite?
Whiteboard: [necko-triaged][checkin on 11/28] → [necko-triaged]
https://hg.mozilla.org/mozilla-central/rev/229546747cbf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
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.
Flags: needinfo?(valentin.gosu)
Attachment #8926888 - Flags: approval-mozilla-beta?
> Do you think we should do this?

For backporting to ESR, please make the argument optional, yes.
(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 ?
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky) → needinfo?(valentin.gosu)
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.
Attachment #8932225 - Flags: review?(bzbarsky) → review+
(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
> 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?
(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.
Flags: needinfo?(valentin.gosu)
> 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...
Group: network-core-security → core-security-release
Comment on attachment 8926888 [details] [diff] [review]
Make sure the appropriate triggeringPrincipal is set for a meta refresh

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8926888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8931457 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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.
Flags: needinfo?(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 :).
Attachment #8931457 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(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.
> 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.
(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.
> Redirect to javascript URIs seem to be explicitly blocked by this check:

Aha.  Yes, that would do it.
MozReview-Commit-ID: GgKVQwkvpFt
Attachment #8933522 - Flags: review?(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
Attachment #8933522 - Flags: review?(bzbarsky) → review+
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.
Flags: needinfo?(dveditz)
Depends on: 1422518
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
Whiteboard: [necko-triaged] → [Embargo until Chrome also fixed][necko-triaged]
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).
Flags: needinfo?(valentin.gosu)
Thanks Ryan. Already filed it: bug 1416307.
Flags: needinfo?(valentin.gosu)
Dan, what is the current status of this on Chrome's side?
Flags: needinfo?(dveditz)
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.
Ok. We'll hold off on disclosure until everyone ships.
Flags: needinfo?(dveditz)
Whiteboard: [Embargo until Chrome also fixed][necko-triaged] → [Embargo until Chrome and Edge also fixed][necko-triaged]
Flags: qe-verify+
Whiteboard: [Embargo until Chrome and Edge also fixed][necko-triaged] → [Embargo until Chrome and Edge also fixed][necko-triaged][post-critsmash-triage]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
All browsers fixed the issue. Thank you very much for the coordination :)
Whiteboard: [Embargo until Chrome and Edge also fixed][necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.