1182546 - Use channel->asyncOpen2 in parser/htmlparser/nsExpatDriver.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_asyncopen2_nsexpatdriver.patch

Comment 2

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

Comment on attachment 8636235 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +138,5 @@
> +      {
> +        if (requestingContext) {
> +          nsCOMPtr<nsIDocument> doc = do_QueryInterface(requestingContext);
> +          MOZ_ASSERT(doc != nullptr,
> +                     "type_dtd requires requestingContext of type doc");

Something like MOZ_ASSERT(requestingContext->NodeType() == nsIDOMNode::DOCUMENT_TYPE) would be smaller/faster.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8636235 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

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

::: parser/htmlparser/nsExpatDriver.cpp
@@ +788,5 @@
>  
>    CopyUTF8toUTF16(absURL, aAbsURL);
>  
>    nsCOMPtr<nsIChannel> channel;
>    if (doc) {

We should do the same as in nsPluginHost, we could add something like:
NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
and eliminate that if-else branch, because a nullPrincipal is most likely not going to pass any security checks.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Within nsExpatDriver.cpp there was no call to CheckLoadURIWithPrincipal(), now that we are adding the flag SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS we are performing a CheckLoadURIWithPrincipal() within the securityManager [1] which fails.

One of the tests that is failing [2] uses the following arguments for CheckLoadURIWithPrincipal().
  loadingPrincipal: about:feeds
  aURI: resource://gre/res/dtd/htmlmathml-f.ent

Note sure how we can fix that problem, because we have to set one of the 5 securityFlags defined in LoadInfo.
Jonas, any suggestions?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#72
[2] browser/base/content/test/general/test_bug364677.html

Comment 5

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

If we don't want to do any security checks, then we should simply use the systemPrincipal as loadingPrincipal.

I think there is some other code somewhere that disables DTD loading almost everywhere. That might be enough, though we'd need to find where that code is. Peterv might know.

Another option is if we can configure things such that the resource://gre/res/dtd/htmlmathml-f.ent URL is loadable from web content. I think the chrome registry has the ability to tell web-accessible URLs from non-web-accessible ones. I don't know if we have something similar for resource URLs. I don't know who knows this part of the code. Jst maybe.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> If we don't want to do any security checks, then we should simply use the
> systemPrincipal as loadingPrincipal.

We could do that, which obviously works.

> I think there is some other code somewhere that disables DTD loading almost
> everywhere. That might be enough, though we'd need to find where that code
> is. Peterv might know.

Peter, do you happen to know where that code is?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_asyncopen2_nsexpatdriver.patch

This bug has been lying around for quite some time now. Let's keep that moving again. Rebased the patch. Carrying over r+ from Jonas and cancelling the needinfo for peterv.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8673405 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Johnny, probably you can help us out here: Can we use the Systemprincipal as the loadingPrincipal within nsExpatDriver? Please see also comment 5 from Jonas.

Comment 9

[Johnny Stenback (:jst)]

cc:ing bholley and bz who likely know more about this than I do.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Looking at this function, we do the load in two cases:

1)  aURLStr led to a chrome:// URI.  
  or
2)  We managed to map aFPIStr to a known local DTD url.

Here we're falling into the second case.

But this whole thing looks totally broken to me.  Consider this XML file:

  <!DOCTYPE window [
  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
  %brandDTD;
  ]>
  <window>&brandShorterName;</window>

When I load that from some random website, that website gets to sync-load that DTD and then can see what &brandShorterName; resolves to in it.  This seems totally broken to me.

I tracked this back a bit and this "allow all loads for chrome:// dtd files" fates back to revision 3.1 of nsExpatDriver.cpp, back in 2002 (patch by harishd, r=heikki, very old times).

It looks like we added the content policy call in bug 418119 but no CheckLoadURI call...

So I think what we should do is only treat the loading principal as system in the "known local DTD url" case: those DTDs are basically specified in various web standards.  In the other cases, we should be using the principal of the document, or the null principal if there is no document (assuming that can happen at all).  And we should probably replace that misleading isChrome check with a check for the UI_RESOURCE or LOCAL_RESOURCE flag, since it's really just checking that it's OK to syncload from that URI.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> (In reply to Jonas Sicking (:sicking) from comment #5)
> > I think there is some other code somewhere that disables DTD loading almost
> > everywhere. That might be enough, though we'd need to find where that code
> > is. Peterv might know.
> 
> Peter, do you happen to know where that code is?

Sorry, missed this needinfo. I don't really understand what code you're looking for. nsExpatDriver::OpenInputStreamFromExternalDTD is the code that loads external DTDs, what else do you need?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_asyncopen2_nsexpatdriver.patch

Boris, is this patch doing what you had in mind or are we missing something?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Peter Van der Beken [:peterv] from comment #11)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> > (In reply to Jonas Sicking (:sicking) from comment #5)
> > > I think there is some other code somewhere that disables DTD loading almost
> > > everywhere. That might be enough, though we'd need to find where that code
> > > is. Peterv might know.
> > 
> > Peter, do you happen to know where that code is?
> 
> Sorry, missed this needinfo. I don't really understand what code you're
> looking for. nsExpatDriver::OpenInputStreamFromExternalDTD is the code that
> loads external DTDs, what else do you need?

Sorry Peter, I just realized I accidentally cleared the needinfo flag earlier this afternoon. That was not my intention. I think Boris is having a clear vision of what we should do within this patch, so solely relying on the systemPrincipal as proposed in comment 5 is not an option anymore.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8673988 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

All the things that are UI_RESOURCE are also LOCAL_RESOURCE, so there is no point in checking both flags.  Pick one or the other, depending on what semantics we want to enforce here.  UI_RESOURCE is likely better, since it's more restrictive.

However returning NS_ERROR_FAILURE if not UI_RESOURCE is wrong, because the stuff that LookupCatalogData needs to get passed to it is not UI_RESOURCE; it's http:// URIs.  So what you want to do is if not UI_RESOURCE do the LookupCatalogData thing.

Apart from that this looks reasonable, I think.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_asyncopen2_nsexpatdriver.patch

Hey Boris, here is an updated version. Please note that I added 'nsILoadInfo::SEC_ALLOW_CHROME' as a securityflag within the else branch. Reason is, when running test [1] we are loading:
> channelURI: chrome://global/locale/global.dtd
> loadingPrincipal: about:feeds
> triggeringPrincipal: about:feeds
> contentPolicyType: 13

which would fail the CheckLoadURIWithPrincipal() check within the contentSecurityManager [2]. I suppose that's the right thing to do, but I would like you to confirm. Anything else we missed here?

[1] browser/base/content/test/general/test_bug364677.html
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#57

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8675936 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

So the new behavior is that if !isUIResource we try to GetLocalDTDURI and if that fails we still do the load, no?

That's wrong.  The whole point of the existing checks here is that if !isUIResource and !localURI then we should not do the load.  Please add tests as needed, since clearly we have poor test coverage here.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8675936 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Oh, and the ALLOW_CHROME bit is probably OK as long as that restricts to exposed chrome packages.  I can't tell from the documentation in nsILoadInfo.idl what the flag actually does, so can't comment intelligently on its use.  :(

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_asyncopen2_nsexpatdriver.patch

Boris, I added back that check:
> if (!localURI) {
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
so that the load is denied if !isUIResource and !localURI.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_nsexpatdriver_tests.patch

I am not sure if I am testing the right thing here, so please let me know if I am mistaken. My guess would have been that ALLOW_CHROME allows to pass CheckLoadURIWithPrincipal() but then the load gets denied because "chrome://branding/locale/brand.dtd" is not accessible to content. In other words, I don't see 'contentaccessible=yes' here [1].

The test currently fails because the current setup allows loading "chrome://branding/locale/brand.dtd". I am not sure how to proceed at this point. Am I missing something?

[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/locales/jar.mn#10

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8685164 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

r=me.  Here's hoping I didn't miss anything....

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8685166 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

> The test currently fails

Currently as in on tip right now, or currently as in with the AsyncOpen2 patch?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > The test currently fails
> 
> Currently as in on tip right now, or currently as in with the AsyncOpen2
> patch?

Sorry for not being precise. With 'currently' I meant: With the asyncopen2()-patch from this bug applied the test fails. In other words, we are allowing to access "chrome://branding/locale/brand.dtd" even though we shouldn't so that browserShortName translates into 'Nightly'.

Running the test without the asyncOpen2() patch from this bug applied, we get the same result, the test fails and we are allowing to access "chrome://branding/locale/brand.dtd" even though we shouldn't.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8685166 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

This test looks reasonable to me.  I don't understand why it fails.  Per IRC discussion, Christoph will try to figure that out.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #23)
> This test looks reasonable to me.  I don't understand why it fails.  Per IRC
> discussion, Christoph will try to figure that out.

As discussed on IRC the test is failing because %content/branding/ is indeed contentAccessible [1]. Only removing contentaccessible=yes from [1] is not doing the trick though. I don't know where that package lives and [2] confuses me even more :-(.

Anyway, you were right, we are creating the new channel in the 'else' branch where we pass 'SEC_ALLOW_CHROME' as a securityFlag. Here are the important values of the loadinfo:
> channelURI: chrome://branding/locale/brand.dtd
> loadingPrincipal: http://mochi.test:8888/tests/browser/base/content/test/general/bug_1182546.xml
> triggeringPrincipal: http://mochi.test:8888/tests/browser/base/content/test/general/bug_1182546.xml
> contentPolicyType: 13

Since the package is content accessible, the function CheckLoadURIWithPrincipal() returns true here [3] and allows the load.

You suggested we could file a bug to make %content/branding not accessible to content. Would you like that to happen within this bug? File a separate bug and make it block this one? Or would you like to rewrite the test to rely on a different URI?


[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/content/jar.mn#6
[2] http://mxr.mozilla.org/mozilla-central/search?string=%25content%2Fbranding%2F&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
[3] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#814

Comment 25

[Boris Zbarsky [:bzbarsky]]

For purposes of this bug, pick a different URI.  And file a separate bug, depending on this one, to move the DTD out of the content-exposed stuff.  Unless the Firefox folks are OK with pages reading all the stuff in that DTD, of course.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Boris, by now I had quite so many attempts to make that test work, but I can't seem to find any URI that is not accessible from content, see some of my attempts underneath:

%---test1---
<!DOCTYPE html [
  <!ENTITY % aboutDTD SYSTEM "chrome://global/locale/about.dtd">
  %aboutDTD;
]>
<window>&about.credits.beforeLink;</window>


%---test2----
<!DOCTYPE window [
  <!ENTITY % canvasDebuggerDTD SYSTEM "chrome://devtools/locale/canvasdebugger.dtd">
	%canvasDebuggerDTD;
]>
<window>&canvasDebuggerUI.recordSnapshot.tooltip;</window>


%---test3----
<!DOCTYPE window [
<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
  %brandDTD;
]>
<window>&brandShorterName;</window>


The good news is that the patch works correctly; if I remove the contentaccessible=yes from unofficial/content/jar.mn [1] then the *.dtd gets blocked correctly with the patch applied but loads without the patch applied.

Any suggestions on what URI we could use?


[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/content/jar.mn#6

Comment 27

[Boris Zbarsky [:bzbarsky]]

I have to admit that I don't understand why the contentaccessible annotation for content/branding affects a dtd in branding/locale...  Given that, I'm really nor sure how to tell which dtds are not contentaccessible.  Gijs, do you have any idea?

Comment 28

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #27)
> I have to admit that I don't understand why the contentaccessible annotation
> for content/branding affects a dtd in branding/locale...  Given that, I'm
> really nor sure how to tell which dtds are not contentaccessible.  Gijs, do
> you have any idea?

This is expected behaviour per https://developer.mozilla.org/en/docs/Chrome_Registration#contentaccessible , specifically:

> The contentaccessible flag applies only to content packages: it is not
> recognized for locale or skin registration. However, the matching locale
> and skin packages will also be exposed to content.


For the test, really the most stable thing to do would be to register your own bootstrapped manifest and remove it again when you're done. The tests for the chrome registry ( https://dxr.mozilla.org/mozilla-central/source/chrome/test ) can probably help with figuring out how to do this. Specifically, https://dxr.mozilla.org/mozilla-central/source/chrome/test/unit/head_crtestutils.js#9 when used from an xpcshell test should work. Because autoRegister persists for the remainder of the executable's lifetime, you probably want to use this method instead if you're using mochitests: https://dxr.mozilla.org/mozilla-central/source/xpcom/components/nsIComponentManager.idl#81 .

I realize this is more work, but I broke the tree the other day because we have tests in caps that rely on arbitrary details of how we package firefox (specifically, on a file called chrome://global/skin/passwordmgr/key.png or something actually existing), and Android is now fighting those same tests again as they move to "finally" remove a bunch of unused XUL/CSS from the fennec packaging. I'd really really really prefer not to exacerbate this problem.

If this is somehow completely impossible, if you look at the toolkit manifest the build system generates:

https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/bin/chrome/toolkit.manifest#5

It seems like a locale file from any of the (corresponding locale) packages without contentaccessible=yes should get the job done.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_nsexpatdriver_tests.patch

Gijs, thanks for your help. Registering a manifest seems complicated. However, I've tried and I am willing to invest a little more time if you can guide me. Please have a look at the other patch (which I am going to upload in a minute) before you review this patch. The testcase in this patch is the 'baseline' test which we could use in case we don't get the registration of the manifest working.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_nsexpatdriver_tests_2.patch

Gijs, so this testcase is at a very half baked state at this point, but at least the basic components should be there. I am trying to register a new manifest using
> SpecialPowers.Components.manager.addBootstrappedManifestLocation(MANIFEST_FILE);

which throws:

2 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/test_bug1182546.html | uncaught exception - Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass at http://mochi.test:8888/tests/browser/base/content/test/general/test_bug1182546.html:32
    simpletestOnerror@SimpleTest/SimpleTest.js:1519:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1506:1


But I suppose that's how the whole dance should somehow work, right? It's the first time I have to deal with packaging for Firefox, so any help is highly appreciated. Thanks!

Comment 31

[:Gijs (he/him)]

Comment on attachment 8688095 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests_2.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Created attachment 8688095 [details] [diff] [review]
> bug_1182546_nsexpatdriver_tests_2.patch
> 
> Gijs, so this testcase is at a very half baked state at this point, but at
> least the basic components should be there. I am trying to register a new
> manifest using
> > SpecialPowers.Components.manager.addBootstrappedManifestLocation(MANIFEST_FILE);
> 
> which throws:
> 
> 2 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/test_bug1182546.html | uncaught exception
> - Error: Permission denied for <http://mochi.test:8888> to create wrapper
> for object of class UnnamedClass at
> http://mochi.test:8888/tests/browser/base/content/test/general/
> test_bug1182546.html:32
>     simpletestOnerror@SimpleTest/SimpleTest.js:1519:11
>     OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1506:1
> 
> 
> But I suppose that's how the whole dance should somehow work, right? It's
> the first time I have to deal with packaging for Firefox, so any help is
> highly appreciated. Thanks!

Seems like you should use a browser mochitest instead of a plain one? Is there some reason that won't work?

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #31)
> Seems like you should use a browser mochitest instead of a plain one? Is
> there some reason that won't work?

Mainly because I wanted to simulate that a 'user' can't access a chrome package if it's not accessible to content. I suppose a browser chrome test would work.

Comment 33

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> (In reply to :Gijs Kruitbosch from comment #31)
> > Seems like you should use a browser mochitest instead of a plain one? Is
> > there some reason that won't work?
> 
> Mainly because I wanted to simulate that a 'user' can't access a chrome
> package if it's not accessible to content. I suppose a browser chrome test
> would work.

Right. You would add the manifest in the test, then load a page in-content that tests the accessibility of the locale file.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8688089 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

If it's going to speed things up here, considering this is a security issue, land this and deal with the "proper" test in a followup?

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8688095 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests_2.patch

(In reply to :Gijs Kruitbosch from comment #34)
> If it's going to speed things up here, considering this is a security issue,
> land this and deal with the "proper" test in a followup?

That sounds like a good idea to me - thanks!

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_nsexpatdriver_make_test_contentaccessible.patch

Boris, test [1] is failing because inlinesettings is not marked to be accessible from content which makes CheckLoadURIWithPrincipal bail out with an error not allowing the load [2].

[1] toolkit/mozapps/extensions/test/browser/browser_inlinesettings_custom.js
[2] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#856

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689247 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_test_contentaccessible.patch

This is a test-only addon?  r=me

Comment 38

[Carsten Book [:Tomcat]]

Hi, this got backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17518341&repo=mozilla-inbound

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182546_nsexpatdriver_make_branding_contentaccessible.patch

(In reply to Carsten Book [:Tomcat] from comment #38)
> Hi, this got backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=17518341&repo=mozilla-
> inbound

Gijs, at the moment it seems there is an inconsistency between browser/, b2g/ and mobile/ where branding is accessible from content for browser/ [1] but not for b2g/ and mobile/ which caused the patches of this bug to be backed out.

I agree with Boris that we should indeed figure out whether the .dtd needs to be accessible from content or not (See Comment 24, 25 and 26). For this bug however, I think it's fine that branding is accessible from content not only for browser/ but also for b2g/ and mobile/.

TRY looks good [2]. Let me know if you feel differently and we can discuss our options again.

[1] http://mxr.mozilla.org/mozilla-central/search?string=content%2Bbranding
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9baaf72709

Comment 40

[:Gijs (he/him)]

Comment on attachment 8689879 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_branding_contentaccessible.patch

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

rs=me

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c2bad72beab
https://hg.mozilla.org/mozilla-central/rev/5c551d0f4bfe
https://hg.mozilla.org/mozilla-central/rev/529d0520902c
https://hg.mozilla.org/mozilla-central/rev/792401624285