1382869 - CSP in <meta> tag of unrendered document leaks to rendered document.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Gerben]

Attachment: test.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170612122310

Steps to reproduce:

Add a strict Content Security Policy (e.g. "img-src 'none'") in a <meta> element in a newly created Document object (thus not window.document). Have or add an image in the rendered document (= window.document).

See attached file for details (test.html).


Actual results:

The image is not displayed. Interestingly, there is no error message in the console telling that the content security policy prohibited loading the image.


Expected results:

The image should show normally, since the CSP is not added to the document with the image, but to a different Document object. (for comparison, this does work fine in Chromium 59).

Comment 1

[Daniel Veditz [:dveditz]]

This is probably a side-effect of us hanging the CSP off the principal rather than the document. The new document rightly inherits the principals (and CSP, if any) of the original document. But it needs to get its own copy rather than a reference so that further CSP changes aren't reflected back. I would have thought that we would clone the principal rather than hold a reference, but if we were then I'm not sure how to explain these symptoms. However the described behavior is incorrect so we need to figure out a fix one way or another.

Be sure to add a test for the case that the original document has a CSP and the new document needs to inherit that so that you can't create a document as a way to escape CSP restrictions.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1382869_test_csp_leak_unredered_doc.patch

Here is an automated test that highlights the problem. Potentially we should extend that test with an iframe that already has a CSP attached and then calls createHTMLDocument(). But good enough for now.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Olli, it seems the document/principal is shared when calling document.implementation.createHTMLDocument() which causes us to attach the CSP from within the newly created document to the actual document that calls createHTMLDocument. It seems that problem is not easily fixable in my opinion - see stacktrace underneath - because it's decendant of the actual document. Unforutnately I don't know how createHTMLDocument works under the hood - any suggestions on how to fix or how to move forward? Happy to provide more debug information if needed.

0x00007fffe393cc02 in nsCSPContext::SetRequestContext (this=0x7fffb9036f60, aDOMDocument=0x7fffb6073370, aPrincipal=0x0)
    at /home/ckerschb/source/mc/dom/security/nsCSPContext.cpp:678
#1  0x00007fffe1600953 in mozilla::BasePrincipal::EnsureCSP (this=0x7fffb471c700, aDocument=0x7fffb6073370, aCSP=0x7fffffff9c80)
    at /home/ckerschb/source/mc/caps/BasePrincipal.cpp:210
#2  0x00007fffe32aea85 in mozilla::dom::HTMLMetaElement::BindToTree (this=0x7fffc29d2350, aDocument=0x7fffb6073000, aParent=0x7fffbf6bc270, aBindingParent=0x0, 
    aCompileEventHandlers=true) at /home/ckerschb/source/mc/dom/html/HTMLMetaElement.cpp:122
#3  0x00007fffe2065537 in nsINode::doInsertChildAt (this=0x7fffbf6bc270, aKid=0x7fffc29d2350, aIndex=0, aNotify=true, aChildArray=...)
    at /home/ckerschb/source/mc/dom/base/nsINode.cpp:1618
#4  0x00007fffe1ebfa8f in mozilla::dom::FragmentOrElement::InsertChildAt (this=0x7fffbf6bc270, aKid=0x7fffc29d2350, aIndex=0, aNotify=true)
    at /home/ckerschb/source/mc/dom/base/FragmentOrElement.cpp:1285
#5  0x00007fffe20681d3 in nsINode::ReplaceOrInsertBefore (this=0x7fffbf6bc270, aReplace=false, aNewChild=0x7fffc29d2350, aRefChild=0x0, aError=...)
    at /home/ckerschb/source/mc/dom/base/nsINode.cpp:2522
#6  0x00007fffe1ec6a58 in nsINode::InsertBefore (this=0x7fffbf6bc270, aNode=..., aChild=0x0, aError=...) at /home/ckerschb/source/mc/dom/base/nsINode.h:1814
#7  0x00007fffe1eb0a67 in mozilla::dom::Element::InsertAdjacent (this=0x7fffbf6bc270, aWhere=u"afterbegin", aNode=0x7fffc29d2350, aError=...)
    at /home/ckerschb/source/mc/dom/base/Element.cpp:3914
#8  0x00007fffe1eb0c04 in mozilla::dom::Element::InsertAdjacentElement (this=0x7fffbf6bc270, aWhere=u"afterbegin", aElement=..., aError=...)
    at /home/ckerschb/source/mc/dom/base/Element.cpp:3936
#9  0x00007fffe2bf7ec9 in mozilla::dom::ElementBinding::insertAdjacentElement (cx=0x7fffd7e22000, obj=..., self=0x7fffbf6bc270, args=...)
    at /home/ckerschb/source/mc-obj-ff-dbg/dom/bindings/ElementBinding.cpp:1314
#10 0x00007fffe2ec24f5 in mozilla::dom::GenericBindingMethod (cx=0x7fffd7e22000, argc=2, vp=0x7fffd58c1090) at /home/ckerschb/source/mc/dom/bindings/BindingUtils.cpp:3053
#11 0x00007fffe6e4274c in js::CallJSNative (cx=0x7fffd7e22000, native=0x7fffe2ec227f <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/source/mc/js/src/jscntxtinlines.h:293
#12 0x00007fffe6e1d5db in js::InternalCallOrConstruct (cx=0x7fffd7e22000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:469
#13 0x00007fffe6e1d954 in InternalCall (cx=0x7fffd7e22000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:514
#14 0x00007fffe6e1d992 in js::CallFromStack (cx=0x7fffd7e22000, args=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:520
#15 0x00007fffe6e2bacd in Interpret (cx=0x7fffd7e22000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:3064
#16 0x00007fffe6e1d149 in js::RunScript (cx=0x7fffd7e22000, state=...) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:409
#17 0x00007fffe6e1e79b in js::ExecuteKernel (cx=0x7fffd7e22000, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=0x7fffffffb5d0)
    at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:698
#18 0x00007fffe6e1ea84 in js::Execute (cx=0x7fffd7e22000, script=..., envChainArg=..., rval=0x7fffffffb5d0) at /home/ckerschb/source/mc/js/src/vm/Interpreter.cpp:731
#19 0x00007fffe73f1703 in ExecuteScript (cx=0x7fffd7e22000, scope=..., script=..., rval=0x7fffffffb5d0) at /home/ckerschb/source/mc/js/src/jsapi.cpp:4638
#20 0x00007fffe73f1945 in ExecuteScript (cx=0x7fffd7e22000, envChain=..., scriptArg=..., rval=0x7fffffffb5d0) at /home/ckerschb/source/mc/js/src/jsapi.cpp:4657
#21 0x00007fffe73f1b53 in JS_ExecuteScript (cx=0x7fffd7e22000, envChain=..., scriptArg=..., rval=...) at /home/ckerschb/source/mc/js/src/jsapi.cpp:4678
#22 0x00007fffe2079ed9 in nsJSUtils::ExecutionContext::CompileAndExec (this=0x7fffffffb590, aCompileOptions=..., aSrcBuf=..., aScript=...)
    at /home/ckerschb/source/mc/dom/base/nsJSUtils.cpp:265
#23 0x00007fffe3fc6f96 in mozilla::dom::ScriptLoader::EvaluateScript (this=0x7fffb4b6c560, aRequest=0x7fffc3247d00) at /home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:2116
#24 0x00007fffe3fc5494 in mozilla::dom::ScriptLoader::ProcessRequest (this=0x7fffb4b6c560, aRequest=0x7fffc3247d00) at /home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:1774
#25 0x00007fffe3fc40cc in mozilla::dom::ScriptLoader::ProcessScriptElement (this=0x7fffb4b6c560, aElement=0x7fffbde6dd90)
    at /home/ckerschb/source/mc/dom/script/ScriptLoader.cpp:1471
#26 0x00007fffe3fb9a63 in mozilla::dom::ScriptElement::MaybeProcessScript (this=0x7fffbde6dd90) at /home/ckerschb/source/mc/dom/script/ScriptElement.cpp:149
#27 0x00007fffe16886eb in nsIScriptElement::AttemptToExecute (this=0x7fffbde6dd90) at /home/ckerschb/source/mc-obj-ff-dbg/dist/include/nsIScriptElement.h:225
#28 0x00007fffe167fb43 in nsHtml5TreeOpExecutor::RunScript (this=0x7fffb3ef3800, aScriptElement=0x7fffbde6dd00) at /home/ckerschb/source/mc/parser/html/nsHtml5TreeOpExecutor.cpp:701
#29 0x00007fffe167f1ea in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7fffb3ef3800) at /home/ckerschb/source/mc/parser/html/nsHtml5TreeOpExecutor.cpp:502
#30 0x00007fffe1689bcb in nsHtml5ExecutorFlusher::Run (this=0x7fffbf4c92b0) at /home/ckerschb/source/mc/parser/html/nsHtml5StreamParser.cpp:128
#31 0x00007fffe034ec47 in nsThread::ProcessNextEvent (this=0x7fffdb74c210, aMayWait=false, aResult=0x7fffffffc187) at /home/ckerschb/source/mc/xpcom/threads/nsThread.cpp:1446
#32 0x00007fffe0354a0b in NS_ProcessNextEvent (aThread=0x7fffdb74c210, aMayWait=false) at /home/ckerschb/source/mc/xpcom/threads/nsThreadUtils.cpp:480
#33 0x00007fffe0bbbb0e in mozilla::ipc::MessagePump::Run (this=0x7fffdd1bb1c0, aDelegate=0x7fffdd1bd1c0) at /home/ckerschb/source/mc/ipc/glue/MessagePump.cpp:97
#34 0x00007fffe0b0b99c in MessageLoop::RunInternal (this=0x7fffdd1bd1c0) at /home/ckerschb/source/mc/ipc/chromium/src/base/message_loop.cc:326
#35 0x00007fffe0b0b920 in MessageLoop::RunHandler (this=0x7fffdd1bd1c0) at /home/ckerschb/source/mc/ipc/chr

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why are we processing meta CSP in data documents?  Why don't we just stop doing that?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Why are we processing meta CSP in data documents?  Why don't we just stop
> doing that?

I thought about that as well, I guess that's a very plausible solution. Is there an somewhat easy solution to enforce that?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Check !IsDataDocument() before applying meta CSP?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Check !IsDataDocument() before applying meta CSP?

Thanks. I'll get that ready. Smaug, I guess no need for your ni? anymore - clearing.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Check !IsDataDocument() before applying meta CSP?

Boris, doing a dxr search for '!IsDataDocument()' does not return any results. I looked through some code and I am not sure I understand what you mean, e.g. I looked at the code that introduced createHTMLDocument() [1], but there is no indication (as far as I can tell) that this document is treated somehow differently so we could query that information so we can use it within HTMLMetaElement.

Just to make sure we are on the same page, this is the code we want to update, right?

+++ b/dom/html/HTMLMetaElement.cpp
@@ -98,17 +98,17 @@ HTMLMetaElement::BindToTree(nsIDocument*
   if (aDocument &&
       AttrValueIs(kNameSpaceID_None, nsGkAtoms::name, nsGkAtoms::viewport, eIgnoreCase)) {
     nsAutoString content;
     rv = GetContent(content);
     NS_ENSURE_SUCCESS(rv, rv);
     nsContentUtils::ProcessViewportInfo(aDocument, content);
   }
 
-  if (CSPService::sCSPEnabled && aDocument &&
+  if (CSPService::sCSPEnabled && aDocument && !aDocument->IsLoadedAsData() &&
       AttrValueIs(kNameSpaceID_None, nsGkAtoms::httpEquiv, nsGkAtoms::headerCSP, eIgnoreCase)) {

Please note that IsLoadedAsData() is not what we are looking for here.

Thanks for your help!

[1] https://bugzilla.mozilla.org/attachment.cgi?id=471777&action=diff

Comment 9

[Boris Zbarsky [:bzbarsky]]

Er, IsLoadedAsData() is what you're looking for.  I just misremembered the name of the API for testing that boolean.

> there is no indication (as far as I can tell) that this document is treated somehow differently

createHTMLDocument calls NS_NewDOMDocument to create the document, and passes true for aLoadedAsData.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1382869_csp_leak_unredered_doc.patch

Boris, thanks for your help with this bug. I guess we can simply ignore the CSP in the case when setting it on a data document. We also simply ignore the meta CSP (without logging anything to the console) if the CSP is defined outside the head element. Agreed?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1382869_test_csp_leak_unredered_doc.patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8894196 [details] [diff] [review]
bug_1382869_csp_leak_unredered_doc.patch

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8894197 [details] [diff] [review]
bug_1382869_test_csp_leak_unredered_doc.patch

>+  doc.head.insertAdjacentElement('afterbegin', metaEl);

How about:

  doc.head.appendChild(metaEl);

for less confusion?

>+  <!-- Including SimpleTest.js so we can use waitForExplicitFinish !-->

You need it for is() as well.  Just take that comment out, please.

r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af89c989d39d
data document should ignore meta csp. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bedc4d36aeeb
Test data document should ignore meta csp. r=bz

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af89c989d39d
https://hg.mozilla.org/mozilla-central/rev/bedc4d36aeeb