Closed Bug 613278 Opened 9 years ago Closed 9 years ago

"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)

Categories

(Core :: Security, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected

People

(Reporter: dbaron, Assigned: geekboy)

References

Details

(Keywords: assertion, Whiteboard: [sg:critical?])

Attachments

(1 file, 3 obsolete files)

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]
Group: core-security
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+....
blocking2.0: --- → betaN+
Component: DOM: Core & HTML → Security
QA Contact: general → toolkit
Severity: normal → critical
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?
The notifyObservers stuff and the XHR stuff certainly needs to be async.
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.
Attached patch proposed patch (obsolete) — Splinter Review
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #491917 - Flags: review?(bzbarsky)
Attached patch proposed patch (really) (obsolete) — Splinter Review
The previous attachment was a stale patch and needed a qrefresh.  This one should be up to date.
Attachment #491917 - Attachment is obsolete: true
Attachment #491919 - Flags: review?(bzbarsky)
Attachment #491917 - Flags: review?(bzbarsky)
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.
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?
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).
> 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.
Attached patch proposed patch (obsolete) — Splinter Review
(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.
Attachment #491919 - Attachment is obsolete: true
Attachment #492415 - Flags: review?(bzbarsky)
Attachment #491919 - Flags: review?(bzbarsky)
Comment on attachment 492415 [details] [diff] [review]
proposed patch

r=me
Attachment #492415 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Whiteboard: [sg:critical?]
Sid, I assume that patch fixes bug 614077 too?
Attached patch proposed patchSplinter Review
There was a bug in attachment 492415 [details] [diff] [review], the identifier blockedUri was undefined.  This patch fixes that (and only that).
Attachment #492415 - Attachment is obsolete: true
Attachment #492794 - Flags: review+
(In reply to comment #13)
> Sid, I assume that patch fixes bug 614077 too?

Yep, just confirmed that this patch fixes bug 614077.
Duplicate of this bug: 614077
http://hg.mozilla.org/mozilla-central/rev/6a1c5a9da215
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Group: core-security
You need to log in before you can comment on or make changes to this bug.