1300671 - Set firstPartyDomain for about: pages

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 1260931 we added an origin attribute called firstPartyDomain.
But right now we don't have firstPartyDomain set for data://, about: URLs, because TLDService doesn't parse these URLs.

So this bug we try to decide what we should do for those URLs.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

For data:// and some about: pages, they use NullPrinicipal, which will be unique already.
Should we also add another isolation for them?

Also should we isolate chrome:// pages? Because this uses SystemPrincipal, which shouldn't have OA.

Comment 2

[Georg Koppen]

The important question, from our point of view, to ask is whether those URLs could be used for cross-domain tracking somehow. If so, we'd need to isolate them to the first party domain. If not this does not seem so important. However, having them isolated as well might be smart in case their functioning gets changed in the future in a way that it would be suddenly problematic. This might be hard to conceive for all schemes involved but for some at least it might be a good defense in depth.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

If data: or about: url pages contain iframes pointing to some http/https site, those sites could of course do some tracking.
So I do think we need to set the firstPartyDomain to some unique value, and such value can be get from the principal itself.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #3)
> If data: or about: url pages contain iframes pointing to some http/https
> site, those sites could of course do some tracking.
> So I do think we need to set the firstPartyDomain to some unique value, and
> such value can be get from the principal itself.

Hi, smaug
For tor, they use --NoFirstPartyHost-- for those about: pages, (I think they didn't have firstParty isolated on data URIs),

https://gitweb.torproject.org/tor-browser.git/tree/netwerk/base/mozIThirdPartyUtil.idl?h=tor-browser-45.3.0esr-6.5-1#n245

Do you suggest we use similar behavior, for example, a fixed string like 'NoFirstPartyHost'?
Or we should have a unique uuid for the firstPartyDomain every time?

Also, so far I only know null-principal is used for sandbox iframe, can you give me some other examples that null-principal is used? I am trying to have a test for these.

Thanks

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

hmm, this gets a bit complicated if they want to opt-out in some cases.
I guess we need some service or pref to have urls which can be opted out.

But for data documents I'd expect normal firstPartyDomain handling. Is there any reason to not have that?

Hmm, how should blob URIs work? I guess they should also get firstPartyDomain when load top level? Hmm, or no, they would inherit firstPartyDomain from opener, same way data-urls would do normally. But if data: url is loaded by any opener, then it should get unique firstPartyDomain.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

When fixing this, the assertion removed in bug 1315602 should be added back in some form, IMO.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

smaug doesn't accept r? for now, ni? him when he has time on this.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8842445 [details] [diff] [review]
Patch

ok, so this is for about: urls only.


>+ * For loading the initial about:blank in e10s mode, it will be loaded with
>+ * NullPrincipal.
>+ */
>+add_task(function* test_remote_window_open_aboutBlank() {
>+  let win = yield BrowserTestUtils.openNewBrowserWindow({ remote: true });
>+  let browser = win.gBrowser.selectedBrowser;
>+
>+  Assert.ok(browser.isRemoteBrowser, "should be a remote browser");
>+
>+  yield ContentTask.spawn(browser, {}, function* () {
>+    info("origin " + content.document.nodePrincipal.origin);
>+    info("principal " + content.document.nodePrincipal);
>+
>+    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
>+                 "", "about:blank shouldn't have firstPartyDomain set");
I don't understand this. Why not? NullPrincipal documents may still have iframes and so, and those iframes may load any pages.
Should those pages be limited to some fpd


>+/**
>+ * Check if data: URI inherits NullPrincipal from about:blank correctly.
>+ */
>+add_task(function* test_remote_window_open_data_uri() {
>+
>+  let win = yield BrowserTestUtils.openNewBrowserWindow({ remote: true });
>+  let browser = win.gBrowser.selectedBrowser;
>+  let mm = browser.messageManager;
>+  mm.loadFrameScript("data:,(" + frame_script.toString() + ")();", true);
>+
>+  yield BrowserTestUtils.browserLoaded(browser, false, function(url) {
>+    return url == "data:text/plain,hello";
>+  });
>+
>+  yield ContentTask.spawn(browser, {}, function* () {
>+    info("origin " + content.document.nodePrincipal.origin);
>+    info("principal " + content.document.nodePrincipal);
>+
>+    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
>+                 "", "data: URI shouldn't have firstPartyDomain set");
So I don't under why not. Same applies here as about:blank loaded with null principal

>+++ b/caps/nsScriptSecurityManager.cpp
>@@ -1161,16 +1161,18 @@ nsScriptSecurityManager::
>   NS_ENSURE_STATE(aLoadContext);
>   OriginAttributes docShellAttrs;
>   bool result = aLoadContext->GetOriginAttributes(docShellAttrs);;
>   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
> 
>   OriginAttributes attrs;
>   attrs.Inherit(docShellAttrs);
> 
>+  attrs.SetFirstPartyDomain(true, aURI);
>+
I'm having great trouble to understand this change. 
Why is true always passed to SetFirstPartyDomain, even if aLoadContext might not be the top level content docshell?

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v2

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #9)
> I don't understand this. Why not? NullPrincipal documents may still have
> iframes and so, and those iframes may load any pages.
> Should those pages be limited to some fpd
> 
Yes, you're right.
I've set firstPartyDomain for NullPrincipal in my v2 patch.

> >+++ b/caps/nsScriptSecurityManager.cpp
> >@@ -1161,16 +1161,18 @@ nsScriptSecurityManager::
> >   NS_ENSURE_STATE(aLoadContext);
> >   OriginAttributes docShellAttrs;
> >   bool result = aLoadContext->GetOriginAttributes(docShellAttrs);;
> >   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
> > 
> >   OriginAttributes attrs;
> >   attrs.Inherit(docShellAttrs);
> > 
> >+  attrs.SetFirstPartyDomain(true, aURI);
> >+
> I'm having great trouble to understand this change. 
> Why is true always passed to SetFirstPartyDomain, even if aLoadContext might
> not be the top level content docshell?
After double checking, this change is not neccesary and can be removed.

Thanks

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v2.

fixed eslint error

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843914 [details] [diff] [review]
Patch v2.

I don't understand why null principal is handled specially.
We do call the existing OA.SetFirstPartyDomain in couple of places, the one in DoURILoad being perhaps the most interesting one. There we set OA of the LoadInfo, but inheriting from docshell, not from any principal. So OA's fpd ends up being empty string (in top level page). Should we perhaps try to inherit fdp from a principal if available, and otherwise use URI + isTopLevel?


It is unclear to me why only one place where we create null principal needs to be updated.
Did you go through them all?

This is tricky stuff. Have spent hours going through the relevant code.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #13)
> Comment on attachment 8843914 [details] [diff] [review]
> Patch v2.
> 
> I don't understand why null principal is handled specially.
Perhaps I misunderstood your comment 9?
You said data: could have iframes, which could be used to do identity tracking.
So since the iframe will inherit origin attributes from the data document, so I update the FirstPartyDomain of the NullPrincipal accordingly.

> We do call the existing OA.SetFirstPartyDomain in couple of places, the one
> in DoURILoad being perhaps the most interesting one. There we set OA of the
> LoadInfo, but inheriting from docshell, not from any principal.
But the OA of the docshell inherits from parent document.

> So OA's fpd
> ends up being empty string (in top level page). 
There're two OAs here. One is the docshell, the other is the document.
We set OA on the LoadInfo, this OA will be the OA of document, (which will NOT be empty string)

The top-level docshell's OA is empty string, but it's not related to LoadInfo.

> Should we perhaps try to
> inherit fdp from a principal if available, and otherwise use URI +
> isTopLevel?
> 
Let me explain in more detail, 

I found when we load about:blank in a remote tab, TabChild will pass nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL from
http://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#1168

which will create principalToInherit will NullPrincipal in
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1512

This (NullPrincipal) principal will become the principalToInherit of the loadInfo in 
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10969

Later when we create the principal for the document(about:blank)
It will call nsScriptSecurityManager::GetChannelResultPrincipal, and return the loadInfo->PrincipalToInherit() as the principal.
http://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#311

So the about:blank will be created with NullPrincipal with FPD set to NULL_PRINCIPAL_FIRST_PARTY_DOMAIN
And data: URIs will inherit the principal and FPD from about:blank.

The princpal is actually inherited from the 'principalToInherit' of loadInfo, is this what you meant by 
"Should we perhaps try to inherit fdp from a principal if available?"

> 
> It is unclear to me why only one place where we create null principal needs
> to be updated.
> Did you go through them all?
> 
Most places are already using nsNullPrincipal::CreateWithInheritedAttributes, which will inherit the OA from the principal or docshell.
This bug is trying to fix the case if the top-level document uses NullPrincipal.

Are you asking if there are other places that about: pages will use NullPrincipal but I didn't find out?

Or are you asking general NullPrincipal should have correct origin attributes, regardless of about: pages?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Yoshi Huang[:allstars.chh] from comment #14)
> 
> > So OA's fpd
> > ends up being empty string (in top level page). 
> There're two OAs here. One is the docshell, the other is the document.
Yes

> We set OA on the LoadInfo, this OA will be the OA of document, (which will
> NOT be empty string)
The OA of LoadInfo has empty string. The principal LoadInfo has doesn't have empty string as fpd, if I read the 
code right.


> Most places are already using
> nsNullPrincipal::CreateWithInheritedAttributes, which will inherit the OA
> from the principal or docshell.
But top level docshell's OA will not have fpd set.


> Are you asking if there are other places that about: pages will use
> NullPrincipal but I didn't find out?
I'm asking if there are cases null principal is created for top level docshell other than the place you updated in the patch.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v3

- Make LoadInfo have the same origin attributes with principalToInherit
- add test for javascript: URI, another case that will have null principal in top-level.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v3

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v3.1

updated commit message.

Hi smaug,

In this patch, I've set FirstPartyDomain on data: URI, about: URI, and javascript: URI.

(In reply to Olli Pettay [:smaug] from comment #15)
> I'm asking if there are cases null principal is created for top level
> docshell other than the place you updated in the patch.

There are two other places will use NullPrincipal as top-level 
1. a sandbox iframe calls window.open, however it should inherit origin attributes from the opener. (But this shouldn't be a top-level _docshell_ load, right?, it is a top-level _document_ load)

2. converter-child.js will call loadInfo.resetPrincipalsToNullPrincipal() and hence reset the principal to NullPrincipal, however in this case it still preserve its own origin attributes. (This mostly happen in tests, like web-platform-tests)

Thanks

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845805 [details] [diff] [review]
Patch v3.1

>+function frame_script() {
>+  content.document.body.innerHTML = `
>+    <a href="data:text/plain,hello" id="test">hello</a>
>+  `;
>+
>+  let element = content.document.getElementById("test");
>+  element.click();
>+}
>+
>+/**
>+ * Check if data: URI inherits NullPrincipal from about:blank correctly.
>+ */
How do you actually test that? I think you need to somehow compare the whole origin, not just firstPartyDomain, since we 
reuse the same firstPartyDomain value for null principals.



>+add_task(function* test_remote_window_open_js_uri() {
>+  let win = yield BrowserTestUtils.openNewBrowserWindow({ remote: true });
>+  let browser = win.gBrowser.selectedBrowser;
>+
>+  Assert.ok(browser.isRemoteBrowser, "should be a remote browser");
>+
>+  browser.loadURI(`javascript:alert("hi");`);
(I wouldn't use alert() but just some 1; or so. alert() spins event loop which might cause some issues)


>+  // If inherit is true, which means loadInfo will have SEC_FORCE_INHERIT_PRINCIPAL
>+  // set, so later when we create principal of the document from
>+  // nsScriptSecurityManager::GetChannelResultPrincipal, we will use
>+  // principalToInherit of the loadInfo as the document principal.
>+  // Therefore we use the origin attributes from aPrincipalToInherit.
>+  //
>+  // Otherwise we just use the origin attributes from docshell.
>+  if (inherit) {
>+    MOZ_ASSERT(aPrincipalToInherit, "We should have aPrincipalToInherit here.");
>+    attrs = aPrincipalToInherit->OriginAttributesRef();
Please MOZ_IF_ASSERT here that if fpd is not enabled, attrs == GetOriginAttributes();
This will break once data: won't do the inheritance though.


I'm not still 100% convinced we should reuse the same fpd value for different data or about etc urls. NullPrincipal created for top level could just use its url as fpd, I think. It would be unique.
What is the reason to reuse the same fpd?
But I guess we can take this and change the behavior as needed later.

Do we have tests for the case when example.com opens a data url window or about:blank?

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #19)
> >+/**
> >+ * Check if data: URI inherits NullPrincipal from about:blank correctly.
> >+ */
> How do you actually test that? I think you need to somehow compare the whole
> origin, not just firstPartyDomain, since we 
> reuse the same firstPartyDomain value for null principals.
Sorry, but I don't understand this part.
on the sentence 'compare the whole origin'?
its origin is moznull-principal:{uuid}^firstPartyDomain={another uuid}, 
what value should I compare to, since it's different every time?

  
> I'm not still 100% convinced we should reuse the same fpd value for
> different data or about etc urls. NullPrincipal created for top level could
> just use its url as fpd, I think. It would be unique.
> What is the reason to reuse the same fpd?
> But I guess we can take this and change the behavior as needed later.
>
So far I think the same fpd is enough, but I've filed bug 1346713 as followup.

> Do we have tests for the case when example.com opens a data url window or
> about:blank?
In my patch, test_remote_window_open_data_uri tests about:blank opens a data url.

And we have another test for calls window.open().
http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/browser_firstPartyIsolation.js#152

http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/window.html
This test will test does window opened by window.open inherit firstPartyDomain from the opener.

Are these enough? I guess you're asking if window.open() or window.open("data:..."), the window should inherit firstPartyDomain from opener, not using NULL_PRINCIPAL_FIRST_PARTY_DOMAIN introduced in this patch, right?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Yoshi Huang[:allstars.chh] from comment #20)
> (In reply to Olli Pettay [:smaug] from comment #19)
> > >+/**
> > >+ * Check if data: URI inherits NullPrincipal from about:blank correctly.
> > >+ */
> > How do you actually test that? I think you need to somehow compare the whole
> > origin, not just firstPartyDomain, since we 
> > reuse the same firstPartyDomain value for null principals.
> Sorry, but I don't understand this part.
> on the sentence 'compare the whole origin'?
> its origin is moznull-principal:{uuid}^firstPartyDomain={another uuid}, 
> what value should I compare to, since it's different every time?
Ok, so we aren't inheriting the principal then. If we were, then we would be using the same principal and origin would be the same. So at least the comment about the test is wrong or not clear enough

 
 
> Are these enough? I guess you're asking if window.open() or
> window.open("data:..."), the window should inherit firstPartyDomain from
> opener, not using NULL_PRINCIPAL_FIRST_PARTY_DOMAIN introduced in this
> patch, right?
right.

(FWIW, data: url handling is about to change of course to not inherit, and always use null principal)

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #21)
> (In reply to Yoshi Huang[:allstars.chh] from comment #20)
> > (In reply to Olli Pettay [:smaug] from comment #19)
> > > >+/**
> > > >+ * Check if data: URI inherits NullPrincipal from about:blank correctly.
> > > >+ */
> > > How do you actually test that? I think you need to somehow compare the whole
> > > origin, not just firstPartyDomain, since we 
> > > reuse the same firstPartyDomain value for null principals.
> > Sorry, but I don't understand this part.
> > on the sentence 'compare the whole origin'?
> > its origin is moznull-principal:{uuid}^firstPartyDomain={another uuid}, 
> > what value should I compare to, since it's different every time?
> Ok, so we aren't inheriting the principal then. If we were, then we would be
> using the same principal and origin would be the same. So at least the
> comment about the test is wrong or not clear enough
>
Okay, I'll update comments to "Check if data: URI inherits firstPartyDomain from about:blank correctly"

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v4

addressed smaug's comments.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca31554d93a2
set firstPartyDomain on about: pages. r=smaug

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca31554d93a2