1225882 - crash in nsContentSecurityManager::doContentSecurityCheck

Status: RESOLVED FIXED

Description

[Ryan VanderMeulen [:RyanVM]]

This bug was filed from the Socorro interface and is 
report bp-45c30230-494b-454e-a97a-b62782151118.
=============================================================

From bug 1215867 comment 3, STR are to do the following with e10s disabled:
> Here are the extensions I tested with (Alpheios reading tools):
> 
> https://addons.mozilla.org/firefox/downloads/latest/15085/platform:2/addon-
> 15085-latest.xpi
> https://addons.mozilla.org/firefox/downloads/latest/15086/addon-15086-latest.
> xpi
> 
> 
> You need both installed.  Navigate to any page. Enable Alpheis by clicking
> Alpheios on the Alpheios toolbar and then Toggle Alpheios On.  Navigate to
> any page and double-click on a word. Good build produces a pop-up, bad build
> doesn't and you see the error "Range.selectNode does not implement interface
> Node." in the browser console.  (A page with ancient greek text is not
> necessary to test, but is useful if you want to see the real functionality
> of the extension :) ) You can find more info on using these extensions here:
> https://vimeo.com/145095436 and
> http://alpheios.net/content/enabling-alpheios)

Reporter indicated that it was an insta-crash after performing the double-click step.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

[Tracking Requested - why for this release]: Crashes with some extensions

The relevant bit here is this part of the stack:

  nsContentSecurityManager::doContentSecurityCheck(nsIChannel*,
                                                   nsCOMPtr<nsIStreamListener>&)
  nsJARChannel::Open2(nsIInputStream**)
  nsSyncLoader::PushSyncStream(nsIStreamListener*)
...
  mozilla::dom::XSLTProcessorBinding::importStylesheet

So that's a call from JS into XSLTProcessor::importStylesheet, which does a sync load of that stylesheet somewhere under there.  In this case that's a sync load of a jar: channel.

If you look at nsJARChannel::Open2, bug 1143922 implemented it like so:

    nsCOMPtr<nsIStreamListener> listener;
    nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this, listener);
    NS_ENSURE_SUCCESS(rv, rv);
    return Open(aStream);

Note that this passes null (in a cleverly obfuscated way) to doContentSecurityCheck.  doContentSecurityCheck then calls DoCORSChecks (it's inlined in the breakpad crash report), which does:

  MOZ_RELEASE_ASSERT(aInAndOutListener, "can not perform CORS checks without a listener");

and we fail that release assert, of course.  Either that assert needs to go, or nsJARChannel::Open2 needs to change.

The release assert looks like it dates to bug 1191645, so this is only a problem on nightly and aurora so far.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

This problem traces back to Bug 1191645 [1]. Before we converted the syncLoadService to use asyncOpen2() we used to rely on the argument 'aLoaderPrincipal' to decide whether to set up a corsListener or not within LoadDocument. We removed the argument 'aLoaderPrincipal' and are passing securityFlags as an argument to LoadDocument instead of the principal.

Since txMozillaStylesheetCompiler used to pass a nullptr as 'aLoaderPrincipal' to LoadDocument we now need to pass SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS instead of SEC_REQUIRE_CORS_DATA_INHERITS as the securityflag within txMozillaStylesheetCompiler. Do you agree Jonas?

[1] http://hg.mozilla.org/mozilla-central/rev/5d5c5ddddff1

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Created attachment 8689121 [details] [diff] [review]
bug_1225882_wrong_securityflag_in_txMozillaStylesheetCompiler.patch

Actually, here is the patch.

Comment 4

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

If txMozillaStylesheetCompiler used to not do security checks, then that was certainly a serious security problem and not behavior that we should bring back.

Comment 5

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

Comment on attachment 8689121 [details] [diff] [review]
bug_1225882_wrong_securityflag_in_txMozillaStylesheetCompiler.patch

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

We should not allow reading XML files cross-origin. That would break the web's same-origin security model.

Comment 6

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

If I understand correctly, the situation here is that we have a chrome document trying to use a chrome url in the XSLT code.

I guess the simplest way to make this work would be to either

A) Let isSync in [1] be false if we're trying to do a CORS load
B) Change the security flags from CORS to ALLOW_CROSS_ORIGIN_DATA_IS_NULL if the loading principal is the
   system principal in the syncloader code [2].
C) Make nsContentSecurityManager only install a nsCORSListenerProxy if the loadingPrincipal is not the
   system principal. I.e. add an early bail somewhere in [3]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSyncLoadService.cpp#326
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsSyncLoadService.cpp#316
[3] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#112

I don't have much of an opinion which solution we use.

Comment 7

[Bridget Almas]

FWIW, I can confirm that is the situation: the extension which manifested the crash tries to load and run XSLT that is packaged with the extension.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Created attachment 8693944 [details] [diff] [review]
bug_1225882_force_asyncload_for_cors.patch

(In reply to Jonas Sicking (:sicking) from comment #6)
> I guess the simplest way to make this work would be to either
> 
> A) Let isSync in [1] be false if we're trying to do a CORS load

A sounds reasonable to me. Thanks Jonas!

Comment 9

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

Comment on attachment 8693944 [details] [diff] [review]
bug_1225882_force_asyncload_for_cors.patch

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

r=me with that fixed

::: dom/base/nsSyncLoadService.cpp
@@ +328,5 @@
>        (NS_SUCCEEDED(aURI->SchemeIs("resource", &isResource)) && isResource);
> +    // if the load needs to enforce CORS, then force the load to be async
> +    if (aSecurityFlags == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
> +        isSync = false;
> +    }

Don't use aSecurityFlags == SEC_REQUIRE_CORS_DATA_INHERITS since other flags might also be set. Use (aSecurityFlags & SEC_REQUIRE_CORS_DATA_INHERITS)

Also, rather than adding a separate if-statement, add

isSync = !(aSecurityFlags & SEC_REQUIRE_CORS_DATA_INHERITS) &&
         (<stuff that's already there>)

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Created attachment 8693964 [details] [diff] [review]
bug_1225882_force_asyncload_for_cors.patch

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c08ce11d24

Comment 12

[Ritu Kothari (:ritu)]

Tracked for 44 since it's a crash.

Comment 13

[Wes Kocher (:KWierso)]

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

Comment 14

[Ritu Kothari (:ritu)]

Jonas, should we uplift this to Aurora44?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8693964 [details] [diff] [review]
bug_1225882_force_asyncload_for_cors.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1191645 - Use channel->ascynOpen2 in dom/base/nsSyncLoadService.cpp

[User impact if declined]:
A sync load of XML files would require CORS and would crash the browser because CORS is not available when using open2().

[Describe test coverage new/current, TreeHerder]:
Using the addons described in comment 0.

[Risks and why]: 
low, because as we know it only affects addons that sync load XML files originating within txMozillaStylesheetCompiler.

[String/UUID change made/needed]:
no

Comment 16

[Ritu Kothari (:ritu)]

Comment on attachment 8693964 [details] [diff] [review]
bug_1225882_force_asyncload_for_cors.patch

I looked at the crash signature and this has not occurred since 12/02 build when the fix landed. That seems reason enough to uplift to Aurora44.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9942e657ff08

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9942e657ff08