1195162 - Use channel->ascynOpen2 dom/xbl/nsXBLService.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

A few notes:
* We can eliminate nsContentUtils::CheckSecurityBeforeLoad completely since there is no other callsite to this function.

* Lets move the same origin check with the bound document closer to the channel creation, so we are more explicit about security checks. (Moving them from LoadBindingDocumentInfo() into FetchBindingDocument()).

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8648565 [details] [diff] [review]
bug_1195162_asyncopen2_xblservice.patch

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

::: dom/xbl/nsXBLService.cpp
@@ +1052,5 @@
> +      // except if the stylesheet has the system principal.
> +      if (!(gAllowDataURIs && SchemeIs(aBindingURI, "data")) &&
> +          !SchemeIs(aBindingURI, "chrome")) {
> +        rv = aBoundDocument->NodePrincipal()->CheckMayLoad(aBindingURI,
> +                                                           true, false);

This check will be done by AsyncOpen2 since you use the SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS mode. So it's not needed here.

What's more problematic is that AsyncOpen2 will always do this check, so it's not helping that you're avoiding this check sometime.

I think we need to add an ALLOW_CHROME flag to nsILoadInfo. For CheckLoadURI checks that would set the appropriate flag to the scriptsecuritymanager. For CheckMayLoad checks, it'd avoid the checks if the loaded URI has a "chrome" scheme.

@@ +1058,5 @@
> +      }
> +
> +      // Finally check if this document is allowed to use XBL at all.
> +      NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL(),
> +                     NS_ERROR_XBL_BLOCKED);

You can move this check outside of the if-statement.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> > +      // Finally check if this document is allowed to use XBL at all.
> > +      NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL(),
> > +                     NS_ERROR_XBL_BLOCKED);
> 
> You can move this check outside of the if-statement.

We can, but then we would slightly change behavior. At the moment this check is only performed 'if (aOriginPrincipal)' is non null. Do we really want that?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

I still think
> +  // check if this document is allowed to use XBL at all.
> +  NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL(), NS_ERROR_XBL_BLOCKED);
belongs inside
> if (aOriginPrincipal) {

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, I meant: Move it outside the "if (!IsSystemOrChromeURLPrincipal(aOriginPrincipal))"

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8648883 [details] [diff] [review]
bug_1195162_asyncopen2_xblservice.patch

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

::: dom/xbl/nsXBLService.cpp
@@ +1035,5 @@
>    rv = NS_NewXBLContentSink(getter_AddRefs(xblSink), doc, aDocumentURI, nullptr);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // check if this document is allowed to use XBL at all.
> +  NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL(), NS_ERROR_XBL_BLOCKED);

So yeah, you probably need to wrap this in a "if (aOriginPrincipal)"

Actually, you probably even need to allow it if aOriginPrincipal is null or the system principal.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Sweet, that even allows us to remove:
* static bool SchemeIs(nsIURI* aURI, const char* aScheme)
* IsSystemOrChromeURLPrincipal(nsIPrincipal* aPrincipal)

Carrying over r+ from Jonas.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Moving SchemeIs from nsXBLService into contentSecuritymanager to make sure we always use the innermost uri for security checks when we allow chrome.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8648952 [details] [diff] [review]
bug_1195162_asyncopen2_xblservice.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +52,5 @@
>      return NS_OK;
>    }
>  
> +  if (aLoadInfo->GetAllowChrome() && SchemeIs(aURI, "chrome")) {
> +    return NS_OK;

Actually, I just realized that we should do a CheckLoadURIWithPrincipal check here. The reason is that ALLOW_CHROME shouldn't actually allow *all* chrome:// URLs.

See http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#807

Sorry, didn't think about this until just now. This is probably a bug in the existing code too...

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xblservice2.patch

(In reply to Jonas Sicking (:sicking) from comment #9)
> Comment on attachment 8648952 [details] [diff] [review]
> bug_1195162_asyncopen2_xblservice.patch
> 
> Review of attachment 8648952 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/nsContentSecurityManager.cpp
> @@ +52,5 @@
> >      return NS_OK;
> >    }
> >  
> > +  if (aLoadInfo->GetAllowChrome() && SchemeIs(aURI, "chrome")) {
> > +    return NS_OK;
> 
> Actually, I just realized that we should do a CheckLoadURIWithPrincipal
> check here. The reason is that ALLOW_CHROME shouldn't actually allow *all*
> chrome:// URLs.
> a
> See
> http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.
> cpp#807
> 
> Sorry, didn't think about this until just now. This is probably a bug in the
> existing code too...

No worries, thanks for catching the problem. I will qfold that patch into the other once reviewed.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Just qfolded those two patches. Carrying over r+ from Jonas.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xbl_update.patch

When running dom/html/test/test_bug613722.html I realized that we should use the triggeringPrincipal intead of the loadingPrincipal for checkMayLoad if SEC_ALLOW_CHROME is set, because the loadingPrincipal would be the principal of aBoundDocument which is *not* the principal that started/triggered the load. Here are the important values within DoSOP() check in the contentSecurityManager:

> loadingPrincipal: http://mochi.test:8888/tests/dom/html/test/test_bug613722.html
> triggeringPrincipal: systemPrincipal
> aURI: chrome://mozapps/content/plugins/pluginProblem.xml

Also, we should only check aBoundDocument->AllowXULXBL() in case the principal (triggeringPrincipal) is not chrome:// or system:// like the initial implementation does.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650074 [details] [diff] [review]
xbl_update.patch

As discussed in our meeting, let me gather some more information first and then I'll reflag you for this patch.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: tmp_xbl.patch

Jonas, looks like a big change, but in fact it isn't. Basically I am only moving the checks for particular security-flags to the callsite so we can reuse DoCheckLoadURIChecks from within DoSOPChecks whenever SEC_ALLOW_CHROME is set.

The real updates are:
* whitelisting content/mozapps, so it's accessible for content code, and
* denying 'data:'; please note that there is a pref gAllowDataURIs which is otherwise unused and there is also a test that data: is denied: dom/xbl/test/test_bug379959.html

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8650218 [details] [diff] [review]
tmp_xbl.patch

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

Looks great. I like this setup for the SOP/CheckLoadURI checks much better.

::: dom/xbl/nsXBLService.cpp
@@ +1037,5 @@
> +    }
> +
> +    if (!gAllowDataURIs && SchemeIs(aDocumentURI, "data")) {
> +      return NS_ERROR_XBL_BLOCKED;
> +    }

This seems really silly. gAllowDataURIs is set to true by default and is only set to false in one test. I.e. it's a feature that only appears to exist in order to test the feature itself.

We should just kill it and make the security checks here more consistent.

Feel free to do as a followup. But basically we should kill everything related to the "layout.debug.enable_data_xbl" pref.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Just qfolded the innerdiff into the main patch. Carrying over r+ from Jonas.

(In reply to Jonas Sicking (:sicking) from comment #15)
> tmp_xbl.patch

> This seems really silly. gAllowDataURIs is set to true by default and is
> only set to false in one test. I.e. it's a feature that only appears to
> exist in order to test the feature itself.
> 
> We should just kill it and make the security checks here more consistent.

I agree, let's kill that, but we should do it all in this patch.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_remove_enable_data_xbl.patch

Removed the pref and also updated the test to be more precise and robust.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650593 [details] [diff] [review]
bug_1195162_remove_enable_data_xbl.patch

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

::: dom/xbl/test/file_bug379959_cross.html
@@ +18,5 @@
>  onload = function() {
> +  // same origin should be allowed
> +  var nodes1 = SpecialPowers.wrap(document).getAnonymousNodes(document.getElementById('div1'));
> +  parent.postMessage({test: "sameOriginIsAllowed",
> +  	                  result: nodes1 ? nodes1.length : 0,

arr, damn tabs, I will fix that up before landing.

@@ +24,5 @@
> +
> +  // cross origin should be blocked
> +  var nodes2 = SpecialPowers.wrap(document).getAnonymousNodes(document.getElementById('div2'));
> +  parent.postMessage({test: "crossOriginIsBlocked",
> +  	                  result: nodes2 ? nodes2.length : 0,

same here

::: dom/xbl/test/file_bug379959_data.html
@@ +12,5 @@
>  <script>
>  onload = function() {
>    nodes = SpecialPowers.wrap(document).getAnonymousNodes(document.getElementById('d'));
> +  parent.postMessage({test: "dataIsBlocked",
> +  	                  result: nodes ? nodes.length : 0,

and here

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8650593 [details] [diff] [review]
bug_1195162_remove_enable_data_xbl.patch

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

r=me with that fixed.

::: dom/xbl/nsXBLService.cpp
@@ +1031,5 @@
>        // check if this document is allowed to use XBL at all.
>        NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL(), NS_ERROR_XBL_BLOCKED);
>      }
>  
> +    if (SchemeIs(aDocumentURI, "data")) {

This is the wrong way around. gAllowDataURIs was always true, so you should remove this whole if-statement since we never entered it.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Jonas, there is one other test failing, in particular this line:
http://mxr.mozilla.org/mozilla-central/source/dom/xbl/test/test_bug591198.html?force=1#35

Here are the important values:
> principal: http://noxul.example.com/tests/dom/xbl/test/file_bug591198_inner.html
> uri: http://noxul.example.com/tests/dom/xbl/test/file_bug591198_xbl.xml
> [2556] WARNING: NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL()) failed:

Please note that in all other test cases withing this test the principal is the SystemPrincipal, so the aBoundDocument->AllowXULXBL() check is bypassed (see patch). Since you wrote that test, can you guess what might go wrong here?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > [2556] WARNING: NS_ENSURE_TRUE(aBoundDocument->AllowXULXBL()) failed:

It sounds like we are bailing early due to XBL not being allowed in the target document? If so that seems to be what the test on line 35 is expecting. I.e. the tests on line 35-37 are verifying that the binding does *not* get attached and fails if the binding did get attached.

What exact test failure are you getting?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

>  iframe.src = "file_bug591198_inner.html";
>  let res = (yield);

>>>>>> res.widths[0]: 44
>>>>>> res.widths[1]: 151
>>>>>> res.widths[2]: 44

>  is(res.widths[0], res.widths[2], "binding was rendered 1");
>  isnot(res.widths[0], res.widths[1], "binding was rendered 2");
>  is(res.anonChildCount, 2, "correct number of anon children");
> INFO TEST-PASS | dom/xbl/test/test_bug591198.html | binding was rendered 1 
> INFO TEST-PASS | dom/xbl/test/test_bug591198.html | binding was rendered 2 
> INFO TEST-PASS | dom/xbl/test/test_bug591198.html | correct number of anon children 

>  iframe.src = "http://noxul.example.com/tests/dom/xbl/test/file_bug591198_inner.html";
>  res = (yield);

>>>>>> res.widths[0]: 0
>>>>>> res.widths[1]: 151
>>>>>> res.widths[2]: 44

>  is(res.widths[0], res.widths[1], "binding was not rendered 3");

>>>>>>> principal: http://noxul.example.com/tests/dom/xbl/test/file_bug591198_inner.html
>>>>>>> uri: http://noxul.example.com/tests/dom/xbl/test/file_bug591198_xbl.xml

> TEST-UNEXPECTED-FAIL | dom/xbl/test/test_bug591198.html | binding was not rendered 3 - got +0, expected 151

>  isnot(res.widths[0], res.widths[2], "binding was not rendered 4");
>  is("anonChildCount" in res, false, "no anon children");

> INFO TEST-PASS | dom/xbl/test/test_bug591198.html | binding was not rendered 4 
> INFO TEST-PASS | dom/xbl/test/test_bug591198.html | no anon children

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It appears that we are not attaching the binding, which is the desired behavior. However it appears that rendering is getting blocked, which is not desired behavior.

The problem is likely that we get much father down in LoadBindingDocumentInfo. I.e. we used to abort pretty early, however now we do a bunch of stuff before we call FetchBindingDocument. The fact that we ignore any errors returned by FetchBindingDocument probably isn't helping either.

Some of the stuff that we are doing is things that we should not do if the document doesn't have permission to use XBL. I.e. you've moved the security checks down below some of the stuff that the security checks are intended to protect.

I think all you need to do to fix this is to move the AllowXULXBL() check back to where it used to be.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xbl_ensure_success_check.patch

(In reply to Jonas Sicking (:sicking) from comment #23)
> It appears that we are not attaching the binding, which is the desired
> behavior. However it appears that rendering is getting blocked, which is not
> desired behavior.
> 
> The problem is likely that we get much father down in
> LoadBindingDocumentInfo. I.e. we used to abort pretty early, however now we
> do a bunch of stuff before we call FetchBindingDocument. The fact that we
> ignore any errors returned by FetchBindingDocument probably isn't helping
> either.

That is exactly what's missing, we have to report error back up the callstack, arrrrh. Should have seen that earlier, especially since we are always discussing that we are probably going to run into those problems where we don't report the error back up the callstack!

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8650751 [details] [diff] [review]
xbl_ensure_success_check.patch

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

I think you should also move up the AllowXULXBL check up to the beginning of LoadBindingDocumentInfo. That check needs to be before the other things that that function does which are not related to network traffic.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xbl_tmp.patch

(In reply to Jonas Sicking (:sicking) from comment #25)
> Comment on attachment 8650751 [details] [diff] [review]
> xbl_ensure_success_check.patch
> 
> Review of attachment 8650751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you should also move up the AllowXULXBL check up to the beginning of
> LoadBindingDocumentInfo. That check needs to be before the other things that
> that function does which are not related to network traffic.

Sure, let's do that. So with this patch I am restoring the original code here:

> if (aOriginPrincipal) {
>    // if there is an originPrincipal we should also have aBoundDocument
>    NS_ASSERTION(aBoundDocument, "can not create a channel without aBoundDocument");

and moving the aBoundDocument->AllowXULXBL() check to the beginning of LoadBindingDocumentInfo().

JFYI, I am still keeping the NS_ENSURE_SUCCESS here:

> rv = FetchBindingDocument(aBoundElement, aBoundDocument, documentURI,
>                           aBindingURI, aOriginPrincipal, aForceSyncLoad,
>                           getter_AddRefs(document));
> NS_ENSURE_SUCCESS(rv, rv);

even though it's not in this patch.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8650760 [details] [diff] [review]
xbl_tmp.patch

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

::: dom/xbl/nsXBLService.cpp
@@ +854,5 @@
>                    "If we're doing a security check, we better have a document!");
>  
>    *aResult = nullptr;
> +  NS_ENSURE_TRUE(!aBoundDocument || aBoundDocument->AllowXULXBL(),
> +                 NS_ERROR_XBL_BLOCKED);

Only do this if aOriginPrincipal is non-null and not the system principal, right?

@@ +1020,5 @@
>    nsCOMPtr<nsIChannel> channel;
>  
>    if (aOriginPrincipal) {
>      // if there is an originPrincipal we should also have aBoundDocument
> +    NS_ASSERTION(aBoundDocument, "can not create a channel without aBoundDocument");

MOZ_ASSERT

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xbl_tmp.patch

(In reply to Jonas Sicking (:sicking) from comment #27)
> > +  NS_ENSURE_TRUE(!aBoundDocument || aBoundDocument->AllowXULXBL(),
> > +                 NS_ERROR_XBL_BLOCKED);
> 
> Only do this if aOriginPrincipal is non-null and not the system principal,
> right?

It seems there is a strong correlation between aBoundDocument being present and aOriginPrincipal. All tests passed even without that check, but I think to be on the safe side we should reproduce the original structure.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Qfolded and rebased. Carrying over r+ from Jonas.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_remove_enable_data_xbl.patch

Rebased on top of the other patch. Carrying over r+ from Jonas.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xbl_debug.patch

Jonas, there are still some tests failing:

(1) We were not using TYPE_XBL when creating the channel. There are two tests that make sure that contentPolicis are called for TYPE_XBL: test_bug375314.html and test_simplecontentpolicy.html. Those tests are great. We just have to update the contentPolicyType for the channel.

(2) Test test_bug840098.html accesses 'chrome://xbl-marquee/content/xbl-marquee.xml', so I suppose we have to make that accessible to content as well, right?

(3) In order to make test_bug613722.html work, we made 'content/mozapps' accessible from content. Test test_bug292789.html relies that 'content/mozapps' is not accessible from content. But since that test does not care about 'content/mozapps' in particular, but rather chose that at random because it was *not* accessible from content, I just replaced it with some other .png that is not accessible from content.

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8651314 [details] [diff] [review]
xbl_debug.patch

Got an email r+ from Jonas for that patch.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Just qfolded the r+ed interdiff into the main patch. Carrying over r+ from Jonas.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 36

[Benjamin Smedberg]

Christoph pinged me on IRC about test_bug292789.html. I don't understand why we want to make chrome://mozapps contentaccessible. There is a pretty significant risk associated with that and I'd like to avoid it if at all possible.

It's not clear what problem we're solving by making it contentaccessible in any case.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> Christoph pinged me on IRC about test_bug292789.html. I don't understand why
> we want to make chrome://mozapps contentaccessible. There is a pretty
> significant risk associated with that and I'd like to avoid it if at all
> possible.

Jonas, I am also not convinced we should make mozapps accessible to content. But even if we do, I would like to re-evaluate the only failing testcase at the moment, which is:
> dom/html/test/test_bug613722.html

The problem is that CheckLoadURIWithPrincipal() fails using the following arguments:

>  channelURI: chrome://mozapps/content/plugins/pluginProblem.xml
>  loadingPrincipal: http://mochi.test:8888/tests/dom/html/test/test_bug613722.html
>  triggeringPrincipal: SystemPrincipal
>  contentPolicyType: 9

When looking at the initial implementation (and especially the comment) we did *not* call checkMayLoad() in this very case of loading a stylesheet where the 'triggeringPrincipal' is the systemPrincipal. See also stacktrace underneath:

> -    if (!IsSystemOrChromeURLPrincipal(aOriginPrincipal)) {
> -      // Also make sure that we're same-origin with the bound document
> -      // except if the stylesheet has the system principal.
> -      if (!(gAllowDataURIs && SchemeIs(aBindingURI, "data")) &&
> -          !SchemeIs(aBindingURI, "chrome")) {
> -        rv = aBoundDocument->NodePrincipal()->CheckMayLoad(aBindingURI,
> -                                                           true, false);
> -        NS_ENSURE_SUCCESS(rv, NS_ERROR_XBL_BLOCKED);
> -      }

I am sure we are doing the right thing in our patch and consult DoCheckLoadURIChecks() in case the SEC_ALLOW_CHROME flag is set in combination with SEC_REQUIRE_SAME_ORIGIN_*. I also believe we should *not* really update that testcase, but I don't know how to make it work. Maybe we should add an additional flag on the loadInfo to indicate that DoCheckLoadURIChecks() should only be enforced on the triggeringPrincipal exactly for this reason. I don't know of any other approaches to make this testcase work without making mozapps/ accessible to content.

Jonas, what do you think?

#01: nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) (/home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:303)
#02: nsBaseChannel::Open2(nsIInputStream**) (/home/ckerschb/moz/mc/netwerk/base/nsBaseChannel.cpp:621)
#03: non-virtual thunk to nsBaseChannel::Open2(nsIInputStream**) (/home/ckerschb/moz/mc-obj-dbg/netwerk/base/Unified_cpp_netwerk_base1.cpp:624)
#04: nsXBLService::FetchBindingDocument(nsIContent*, nsIDocument*, nsIURI*, nsIURI*, nsIPrincipal*, bool, nsIDocument**) (/home/ckerschb/moz/mc/dom/xbl/nsXBLService.cpp:1090)
#05: nsXBLService::LoadBindingDocumentInfo(nsIContent*, nsIDocument*, nsIURI*, nsIPrincipal*, bool, nsXBLDocumentInfo**) (/home/ckerschb/moz/mc/dom/xbl/nsXBLService.cpp:949)
#06: nsXBLService::GetBinding(nsIContent*, nsIURI*, bool, nsIPrincipal*, bool*, nsXBLBinding**, nsTArray<nsIURI*>&) (/home/ckerschb/moz/mc/dom/xbl/nsXBLService.cpp:725)
#07: nsXBLService::GetBinding(nsIContent*, nsIURI*, bool, nsIPrincipal*, bool*, nsXBLBinding**) (/home/ckerschb/moz/mc/dom/xbl/nsXBLService.cpp:651)
#08: nsXBLService::LoadBindings(nsIContent*, nsIURI*, nsIPrincipal*, nsXBLBinding**, bool*) (/home/ckerschb/moz/mc/dom/xbl/nsXBLService.cpp:453)
#09: nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsIAtom*, int, bool, nsStyleContext*, unsigned int, nsTArray<nsIAnonymousContentCreator::ContentInfo>*, nsCSSFrameConstructor::FrameConstructionItemList&) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:5512)
#10: nsCSSFrameConstructor::DoAddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, nsStyleContext*, bool, nsContainerFrame*, nsTArray<nsIAnonymousContentCreator::ContentInfo>*, nsCSSFrameConstructor::FrameConstructionItemList&) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:5433)
#11: nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:5448)
#12: nsCSSFrameConstructor::CreateAnonymousFrames(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, PendingBinding*, nsFrameItems&) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:4020)
#13: nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, nsIAtom*, bool, nsContainerFrame*&) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:4429)
#14: nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:2876)
#15: nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:2417)
#16: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:7498)
#17: nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (/home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:7383)
#18: PresShell::Initialize(int, int) (/home/ckerschb/moz/mc/layout/base/nsPresShell.cpp:1659)
#19: nsContentSink::StartLayout(bool) (/home/ckerschb/moz/mc/dom/base/nsContentSink.cpp:1239)
#20: nsHtml5TreeOpExecutor::StartLayout() (/home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:612)
#21: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) (/home/ckerschb/moz/mc/parser/html/nsHtml5TreeOperation.cpp:824)
#22: nsHtml5TreeOpExecutor::RunFlushLoop() (/home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:447)
#23: nsHtml5ExecutorFlusher::Run() (/home/ckerschb/moz/mc/parser/html/nsHtml5StreamParser.cpp:127)
#24: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:865)
#25: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:277)
#26: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95)
#27: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235)
#28: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228)
#29: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201)
#30: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156)
#31: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:281)
#32: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4292)
#33: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4389)
#34: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4478)
#35: do_main(int, char**, nsIFile*) (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:212)
#36: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:399)
#37: __libc_start_main (/build/buildd/eglibc-2.19/csu/libc-start.c:321)
#38: _start (/home/ckerschb/moz/mc-obj-dbg/dist/bin/firefox)

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> Christoph pinged me on IRC about test_bug292789.html. I don't understand why
> we want to make chrome://mozapps contentaccessible. There is a pretty
> significant risk associated with that and I'd like to avoid it if at all
> possible.

Is there? What are the risks?

I thought we only blocked access to chrome:// URLs in order to prevent a webpage to detect which addons the user has installed. Which for built-in URLs clearly isn't an issue.

> It's not clear what problem we're solving by making it contentaccessible in
> any case.

Just to fix tests. If we can fix the test in other ways then that's definitely fine with me.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Benjamin, in particular, we made mozapps accessible to content in order to make the following test work:

https://mxr.mozilla.org/mozilla-central/source/dom/html/test/test_bug613722.html?force=1

If you can think of a way to make that test work without exposing mozapps to content, please let us know.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #39)
> Benjamin, in particular, we made mozapps accessible to content in order to
> make the following test work:
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/html/test/test_bug613722.
> html?force=1
> 
> If you can think of a way to make that test work without exposing mozapps to
> content, please let us know.

Benjamin, if we decide to make mozapps accessible to content, we would have to update that testcase:
http://mxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html?force=1

In particular, we would need alternative chrome urls for line 57 and 58.
> 57 var img_mozapps = "chrome://mozapps/skin/passwordmgr/key.png";
> 58 var res_mozapps = "resource://gre/chrome/toolkit/skin/classic/mozapps/passwordmgr/key.png";

Can you suggest any other chrome urls that are not accessible from content?

Comment 41

[Benjamin Smedberg]

I don't understand what chrome://mozapps has to do with that test. I don't see any references to mozapps in that test. But also, if this is just about making a test work, making mozapps publicly-accessible is the wrong way to go.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> I don't understand what chrome://mozapps has to do with that test. I don't
> see any references to mozapps in that test. But also, if this is just about
> making a test work, making mozapps publicly-accessible is the wrong way to
> go.

This test [1] relies on the fact that mozapps is not publicly accessible for content. We are about to make mozapps publicly accessible, hence we have to update that test and I was wondering if you could suggest any other chrome url which I could use to make the test work again. I don't think the test is relying on mozapps in particular, as far as I understand. The test just wants to make sure that we block chrome:// access to *not* publicly accessible urls.

[1] http://mxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/test_bug292789.html?force=1

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Making changes in order to get tests to pass is usually a good thing. I.e. we don't want to break the feature that the test is testing, no? :)

See comment 12 for why the test is failing. The test is trying to load chrome://mozapps/content/plugins/pluginProblem.xml, but the load gets blocked because the package isn't available to content.

Previously we would always allow an XBL file which was linked to by a chrome stylesheet to link to any chrome:// package, even if the package isn't exposed to content. Now we are simplifying and homogenizing security checks which means that we now block access to chrome:// URLs in unexposed packages no matter who linked to the resource.

Comment 44

[Benjamin Smedberg]

I think we're talking past eachother. There are many things in mozapps which content should not be able to link to. Please do not make mozapps publicly accessible. Instead we should be moving pluginProblem to a different location/package which is publicly accessible.

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that there are two tests involved. 

test_bug613722.html fails because we are blocking access to chrome://mozapps/content/plugins/pluginProblem.xml.

test_bug292789.html is failing once we fix test_bug613722.html by making mozapps available to content since the test is explicitly wanting to test that a package which isn't exposed can't be loaded, and happened to use mozapps as an example of an unexposed package.


Also, I still don't understand the risks of exposing packages to content. Obviously we shouldn't do that if the risks are bad, but I can't judge that without understanding what the risks are.

Comment 46

[Benjamin Smedberg]

Exposing a package to content means that content can link to or load chrome URIs in various contexts. Since mozapps is used for pretty powerful stuff (including app update and the addon manager), allowing chrome to link to these URIs is a large risk surface that I know we don't want to audit.

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_asyncopen2_xblservice.patch

Rebased the xblservice. Carrying over r+ from Jonas.

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_remove_enable_data_xbl.patch

Rebased; Carrying over r+ from Jonas.

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_pluginproblem.patch

Moving mozapps/plugins into pluginproblem/ and making it accessible to content.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8658401 [details] [diff] [review]
bug_1195162_pluginproblem.patch

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

This looks fine to me, but I don't know the chrome manifest stuff well enough to review, so need bsmedberg's review here.

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_pluginproblem.patch

Benjamin, after our IRC conversation I updated a punch of things in the patch. E.g., use pluginproblem  throughout the patch consistently to avoid semaatic confusion between pluginproblem and plugins. Hopefully I got it all right. I am not an export on this part of the code, so any guidance is highly appreciated. Thanks!

Comment 52

[Benjamin Smedberg]

Comment on attachment 8660033 [details] [diff] [review]
bug_1195162_pluginproblem.patch

>diff --git a/mobile/android/locales/jar.mn b/mobile/android/locales/jar.mn

>-# plugins
>-  locale/@AB_CD@/browser/overrides/plugins/plugins.dtd             (%chrome/mozapps/plugins/plugins.dtd)
>+# pluginproblem
>+  locale/@AB_CD@/browser/overrides/pluginproblem/pluginproblem.dtd (%chrome/pluginproblem/pluginproblem.dtd)

This packaging is correct and shouldn't be changed. What you need to change is this line below:

% override chrome://mozapps/locale/plugins/plugins.dtd chrome://browser/locale/overrides/plugins/plugins.dtd

Since now android needs to override chrome://pluginproblem/locale/ instead of chrome://mozapps/locale/...

>diff --git a/toolkit/mozapps/extensions/content/pluginPrefs.xul b/toolkit/mozapps/extensions/content/pluginPrefs.xul
>--- a/toolkit/mozapps/extensions/content/pluginPrefs.xul
>+++ b/toolkit/mozapps/extensions/content/pluginPrefs.xul
>@@ -1,15 +1,15 @@
> <?xml version="1.0"?>
> 
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this file,
>    - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> 
>-<!DOCTYPE window SYSTEM "chrome://mozapps/locale/plugins/plugins.dtd">
>+<!DOCTYPE window SYSTEM "chrome://pluginproblem/pluginproblem.dtd">

Needs to be chrome://pluginproblems/locale/pluginproblem.dtd

>diff --git a/toolkit/mozapps/plugins/content/pluginFinderBinding.css b/toolkit/pluginproblem/content/pluginFinderBinding.css

> object:-moz-type-unsupported-platform {
>     display: inline-block;
>     overflow: hidden;
>-    -moz-binding: url('chrome://mozapps/content/plugins/pluginProblem.xml#pluginProblem') !important;
>+    -moz-binding: url('chrome://pluginproblem/pluginProblem.xml#pluginProblem') !important;
> }

chrome://pluginproblem/content/pluginProblem.xml...

>diff --git a/toolkit/mozapps/plugins/content/pluginProblem.xml b/toolkit/pluginproblem/content/pluginProblem.xml

>-  <!ENTITY % pluginsDTD SYSTEM "chrome://mozapps/locale/plugins/plugins.dtd">
>+  <!ENTITY % pluginproblemDTD SYSTEM "chrome://pluginproblem/pluginproblem.dtd">

chrome://pluginproblem/locale/pluginproblem.dtd here and similar below. Lots more not having /content or /locale, so just fix by going through the patch.

>--- a/toolkit/mozapps/plugins/jar.mn
>+++ b/toolkit/pluginproblem/jar.mn

> toolkit.jar:
>-% content mozapps %content/mozapps/
>-  content/mozapps/plugins/pluginProblem.xml                     (content/pluginProblem.xml)
>-  content/mozapps/plugins/pluginProblemContent.css              (content/pluginProblemContent.css)
>-  content/mozapps/plugins/pluginProblemBinding.css              (content/pluginProblemBinding.css)
>-  content/mozapps/plugins/pluginFinderBinding.css               (content/pluginFinderBinding.css)
>+% pluginproblem %pluginproblem/ contentaccessible=yes

Does this even build? This is the chrome registration instruction and needs to be "content pluginproblem %pluginproblem/ contentaccessible=yes"

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_pluginproblem.patch

Thanks Benjamin for your help and feedback.

Comment 54

[Benjamin Smedberg]

Comment on attachment 8660068 [details] [diff] [review]
bug_1195162_pluginproblem.patch

LGTM: please test carefully. You can use http://benjamin.smedbergs.us/tests/ctptests/ to test whether this binding is still working correctly.

Comment 55

[Christoph Kerschbaumer [:ckerschb]]

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

Benjamin, it looks like android is not working properly - did we miss something?

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #55)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=edcc44141138
> 
> Benjamin, it looks like android is not working properly - did we miss
> something?

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

Attachment: plugingproblem_android.patch

Benjamin, before pushing to TRY again, is this the right change?

Comment 58

[Christoph Kerschbaumer [:ckerschb]]

Attachment: plubingproblem_android.patch

Benjamin, thanks for you guidance with this patch. Hopefully this is what you meant on IRC to get android fixed.

Once r+ed I am going to qfold this patch into the other one.

Comment 59

[Benjamin Smedberg]

Attachment: 1195162-android.patch

You want to fix the android-specific packaging, not the main packaging which was already correct.

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_pluginproblem.patch

Qfolded Benhmain's fix into the main pluginproblem patch. Carrying over r+.

Comment 61

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195162_pluginproblem.patch

Discussed with bsmedberg over IRC, it should be:
> (%chrome/pluginproblem/pluginproblem.dtd)
instead of
> (%chrome/mozapps/pluginproblem/pluginproblem.dtd)

Only made that change, carrying over r+.

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf37257f81d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e1f5749c09
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ba84502c36

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf37257f81d2
https://hg.mozilla.org/mozilla-central/rev/58e1f5749c09
https://hg.mozilla.org/mozilla-central/rev/92ba84502c36

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jorge, this changes our security around XBL in a way that we are seeing affecting addons.

That patch is part of an effort to harmonize and simplify our security checks.

It used to be that *all* chrome:// URLs were accessible to content as long as it was loaded as XBL, and as long as the link to it came from a chrome:// stylesheet.

This meant that we used different policies for chrome:// URLs depending on if a resource was loaded as a script, as an XBL or as a stylesheet.

The goal is that any content that can be loaded into a webpage needs to have a contentaccessible=yes, no matter how it is loaded. And so any content which does *not* have contentaccessible=yes is sure to *not* be loaded into a webpage.

The result is that some addons will have to add contentaccessible=yes to their manifests that contain the XBL file that they want to use in content.

Comment 65

[Jorge Villalobos [:jorgev] (he/him)]

Okay, I'll make sure to add it to our compat post.

Comment 66

[[e-mail]]

Comment on attachment 8658398 [details] [diff] [review]
bug_1195162_asyncopen2_xblservice.patch

>-    if (!IsSystemOrChromeURLPrincipal(aOriginPrincipal)) {
>+  if (aOriginPrincipal && !nsContentUtils::IsSystemPrincipal(aOriginPrincipal)) {
This change caused bug 1211708. This is because themes are allowed to specify bindings, but they don't have the system principal.