613278 - "ASSERTION: Want to fire mutation events, but it's not safe" when CSP is considering the load of a font (test_CSP_evalscript.html / test_CSP.html)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In mochitest-plain-1, there are 20 assertions of the form:

###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtre
e()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file /builds/slave/mozilla-central-linux-debug/build/content/base/src/nsContentUtils.cpp, line 3662

that seem related to CSP and loading of fonts.  This stack looks pretty scary to me, so marking security-sensitive.

They start happening between here:
1175 INFO TEST-END | /tests/content/base/test/test_CSP.html | finished in 1523ms
and here:
1176 INFO TEST-START | /tests/content/base/test/test_CSP_evalscript.html
and continue to here:
1177 INFO TEST-PASS | /tests/content/base/test/test_CSP_evalscript.html | EVAL SCRIPT BLOCKED: setTimeout(String)(setTimeout with a string was blocked)

The relevant stack is:

nsContentUtils::HasMutationListeners [content/base/src/nsContentUtils.cpp:3665]
nsINode::doRemoveChildAt [content/base/src/nsGenericElement.cpp:3661]
nsGenericElement::RemoveChildAt [content/base/src/nsGenericElement.cpp:3637]
nsContentUtils::SetNodeTextContent [content/base/src/nsContentUtils.cpp:4123]
nsGenericHTMLElement::SetInnerHTML [content/html/content/src/nsGenericHTMLElement.cpp:732]
nsIDOMNSHTMLElement_SetInnerHTML [dom_quickstubs.cpp:21471]
js::CallJSPropertyOpSetter [js/src/jscntxtinlines.h:738]
js::Shape::set [js/src/jsscopeinlines.h:275]
js_NativeSet [js/src/jsobj.cpp:4947]
js::Interpret [js/src/jsinterp.cpp:4411]
js::RunScript [js/src/jsinterp.cpp:657]
js::Invoke [js/src/jsinterp.cpp:737]
js::ExternalInvoke [js/src/jsinterp.cpp:862]
js::ExternalInvoke [js/src/jsinterp.h:955]
JS_CallFunctionValue [js/src/jsapi.cpp:4973]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1694]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:588]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95]
nsObserverList::NotifyObservers [xpcom/ds/nsObserverList.cpp:129]
nsObserverService::NotifyObservers [xpcom/ds/nsObserverService.cpp:185]
xptiInterfaceEntry::EnsureResolved [xpcom/reflect/xptinfo/src/xptiprivate.h:264]
CallMethodHelper::Invoke [js/src/xpconnect/src/xpcwrappednative.cpp:3042]
CallMethodHelper::Call [js/src/xpconnect/src/xpcwrappednative.cpp:2304]
XPCWrappedNative::CallMethod [js/src/xpconnect/src/xpcwrappednative.cpp:2268]
XPC_WN_CallMethod [js/src/xpconnect/src/xpcwrappednativejsops.cpp:1594]
js::CallJSNative [js/src/jscntxtinlines.h:684]
js::Interpret [js/src/jsinterp.cpp:4779]
js::RunScript [js/src/jsinterp.cpp:657]
js::Invoke [js/src/jsinterp.cpp:737]
js::ExternalInvoke [js/src/jsinterp.cpp:862]
js::ExternalInvoke [js/src/jsinterp.h:955]
JS_CallFunctionValue [js/src/jsapi.cpp:4973]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp:1694]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp:588]
PrepareAndDispatch [xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95]
CSPService::ShouldLoad [content/base/src/nsCSPService.cpp:134]
nsContentPolicy::CheckPolicy [content/base/src/nsContentPolicy.cpp:157]
nsContentPolicy::ShouldLoad [content/base/src/nsContentPolicy.cpp:218]
NS_CheckContentLoadPolicy [nsContentPolicyUtils.h:221]
nsFontFaceLoader::CheckLoadAllowed [layout/style/nsFontFaceLoader.cpp:190]
nsUserFontSet::StartLoad [layout/style/nsFontFaceLoader.cpp:260]
gfxUserFontSet::LoadNext [gfx/thebes/gfxUserFontSet.cpp:637]
gfxUserFontSet::FindFontEntry [gfx/thebes/gfxUserFontSet.cpp:192]
FindFontPatterns [gfx/thebes/gfxPangoFonts.cpp:1030]
gfxFcFontSet::SortPreferredFonts [gfx/thebes/gfxPangoFonts.cpp:1193]
gfxFcFontSet::gfxFcFontSet [gfx/thebes/gfxPangoFonts.cpp:938]
gfxPangoFontGroup::MakeFontSet [gfx/thebes/gfxPangoFonts.cpp:1728]
gfxPangoFontGroup::GetBaseFontSet [gfx/thebes/gfxPangoFonts.cpp:2194]
gfxPangoFontGroup::GetBaseFont [gfx/thebes/gfxPangoFonts.cpp:1666]
gfxPangoFontGroup::GetFontAt [gfx/thebes/gfxPangoFonts.cpp:1685]
nsThebesFontMetrics::GetMetrics [gfx/src/thebes/nsThebesFontMetrics.cpp:112]
...
ViewportFrame::Reflow [layout/generic/nsViewportFrame.cpp:293]

Comment 1

[Boris Zbarsky [:bzbarsky]]

Uh... contentSecurityPolicy.js does all sorts of nasty stuff (starting network loads, dispatching arbitrary observer notifications, etc) from inside shouldLoad.  All that stuff needs to be async or be removed ASAP.

Marking betaN+, but maybe this should be beta8+....

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

The specific problem in the CSP tests occur do_finish_test() is called from inside something called by a Content Policy's ShouldLoad method -- that's responding at a bad time.  That should be fixed by the code to make the finish calls asynchronous in bug 612391.  I could write a separate patch just for this if we need it sooner than that patch gets pushed.

bz: would you point out specific calls that need to be made async, and I'll address those as fast as I can?

Comment 3

[Boris Zbarsky [:bzbarsky]]

The notifyObservers stuff and the XHR stuff certainly needs to be async.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Either that, or the observers need to be carefully documented in terms of what they're allowed to do.  But unless there's a must-have need for the notifications to be sync, making them async is easier and safer.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

This seems to fix the assertion on my builds.  I factored out the code that needed to be made asynchronous and tossed it into a runnable dispatched on the main thread.  I also re-wrote the code to use Services.jsm for the IO service and thread manager.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch (really)

The previous attachment was a stale patch and needed a qrefresh.  This one should be up to date.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

In case you get a failure applying attachment 491919 [details] [diff] [review]: it is in my patch queue on top of one for bug 612391 (since fixing that bug provides more tests of report-uri), but this patch need not depend on 612391 and can be fairly applied to a tree without the other patch by ignoring the failed hunk.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The docs for the first argument of blockedURI don't really match reality (in particular, 'self' is not a URI in any way).

Why do you need the ""+d thing?  Why can't you just set .data to d?

You can pass a function directly as the first arg to dispatch(); any reason you have the extra vt object?

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Thanks for the rapid response, bz.

(In reply to comment #8)
> The docs for the first argument of blockedURI don't really match reality (in
> particular, 'self' is not a URI in any way).

Changed to "source" -- the CSP notion of an origin includes 'self' and 'none' keywords, and more or less can be URIs.  

> Why do you need the ""+d thing?  Why can't you just set .data to d?

I wanted to be absolutely sure d was a string, but I guess the setter probably takes care of that.

> You can pass a function directly as the first arg to dispatch(); any reason you
> have the extra vt object?

Good to know, and the only reason I used vt was because I didn't know I could pass in a function.  I was mirroring some other code I saw in m-c since I hadn't used the thread manager yet, and was basing my code off that and the nsIRunnable interface (saw that one of those is passed to dispatch() in nsIEventTarget).

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Changed to "source" -- the CSP notion of an origin includes 'self' and 'none'
> keywords, and more or less can be URIs.  

OK, so this should really document that the argument is a CSP origin or something, right?

> but I guess the setter probably takes care of that.

Yes.

> was basing my code off that and the nsIRunnable interface

For future reference:

  40 [scriptable, function, uuid(4a2abaf0-6886-11d3-9382-00104ba0fd40)]
  41 interface nsIRunnable : nsISupports

That "function" bit in the interface flags means you can pass a JS function as an instance of this interface, and that invoking the single method on the interface will call the function.  We generally try to use that for callback interfaces.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

(In reply to comment #10)
> OK, so this should really document that the argument is a CSP origin or
> something, right?

Yeah.  The field name used in the reports' transmitted JSON is "blocked-uri", and the field's contents are supposed to represent from where the objectionable content came. In the case of an external resource that's blocked and causes a policy violation, the resource's URI is transmitted in this field.  but in the case of an inline script or use of eval() that's blocked, the CSP Source 'self' is used to indicate that the script was on the document itself.

In the patch, I had used the variable name "blockedUri" to match the field in the violation report used for transmitting the variable's contents, but revisiting the naming of this JSON field might be appropriate since it's not always a URI.

Attach is an update of the patch with comments incorporated.  Should apply cleanly to m-c without the previous version's dependency.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492415 [details] [diff] [review]
proposed patch

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Sid, I assume that patch fixes bug 614077 too?

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

There was a bug in attachment 492415 [details] [diff] [review], the identifier blockedUri was undefined.  This patch fixes that (and only that).

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #13)
> Sid, I assume that patch fixes bug 614077 too?

Yep, just confirmed that this patch fixes bug 614077.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6a1c5a9da215