Closed Bug 1225882 Opened 10 years ago Closed 10 years ago

crash in nsContentSecurityManager::doContentSecurityCheck

Categories

(Core :: DOM: Security, defect)

44 Branch
Unspecified
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: RyanVM, Assigned: ckerschb)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
[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.
Blocks: 1191645
Flags: needinfo?(mozilla)
Keywords: regression
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
Flags: needinfo?(mozilla) → needinfo?(jonas)
Actually, here is the patch.
Attachment #8689121 - Flags: review?(jonas)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
If txMozillaStylesheetCompiler used to not do security checks, then that was certainly a serious security problem and not behavior that we should bring back.
Flags: needinfo?(jonas)
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.
Attachment #8689121 - Flags: review?(jonas) → review-
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.
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.
(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!
Attachment #8689121 - Attachment is obsolete: true
Attachment #8693944 - Flags: review?(jonas)
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>)
Attachment #8693944 - Flags: review?(jonas) → review+
Attachment #8693944 - Attachment is obsolete: true
Attachment #8693964 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Jonas, should we uplift this to Aurora44?
Flags: needinfo?(jonas)
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
Flags: needinfo?(jonas)
Attachment #8693964 - Flags: approval-mozilla-aurora?
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.
Attachment #8693964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: