1067517 - Do not call contentPolicies with a nullptr as Node in txMozillaStylesheetCompiler.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb}]

We should not use
>  aProcessor->GetSourceContentModel()
when calling contentPolicies in
http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#514

This will always be a nullptr and hence does not provide any usefule information. Since we are going to store the requestingNode also in the loadInfo, we should fix that problem. Please also see:

https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c255
and the proposed patch
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c254

[Additional note]
Most likely we also have to call ::Init() here:
http://mxr.mozilla.org/mozilla-central/source/dom/xml/nsXMLPrettyPrinter.cpp#109
otherwise mDocument might be null when we try to query the NodePrincipal from it, see:

18:09:40     INFO -  nsCOMPtr<nsIDocument>::operator->() const [xpcom/glue/nsCOMPtr.h:828]
18:09:40     INFO -  txMozillaXSLTProcessor::ImportStylesheet(nsIDOMNode*) [dom/xslt/xslt/txMozillaXSLTProcessor.cpp:586]
18:09:40     INFO -  nsXMLPrettyPrinter::PrettyPrint(nsIDocument*, bool*) [dom/xml/nsXMLPrettyPrinter.cpp:115]
18:09:40     INFO -  nsXMLContentSink::MaybePrettyPrint() [dom/xml/nsXMLContentSink.cpp:213]
18:09:40     INFO -  nsXMLContentSink::DidBuildModel(bool) [dom/xml/nsXMLContentSink.cpp:308]
18:09:40     INFO -  nsParser::DidBuildModel(tag_nsresult) [parser/htmlparser/nsParser.cpp:908]
18:09:40     INFO -  nsParser::ResumeParse(bool, bool, bool) [parser/htmlparser/nsParser.cpp:1507]
18:09:40     INFO -  nsParser::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) [parser/htmlparser/nsParser.cpp:1878]
18:09:40     INFO -  nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) [uriloader/base/nsURILoader.cpp:323]
18:09:40     INFO -  nsStreamListenerTee::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) [netwerk/base/src/nsStreamListenerTee.cpp:54]
18:09:40     INFO -  mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) [netwerk/protocol/http/nsHttpChannel.cpp:5198]
18:09:40     INFO -  nsInputStreamPump::OnStateStop() [netwerk/base/src/nsInputStreamPump.cpp:722]
18:09:40     INFO -  nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) [netwerk/base/src/nsInputStreamPump.cpp:440]
18:09:40     INFO -  nsInputStreamReadyEvent::Run() [xpcom/io/nsStreamUtils.cpp:88]
18:09:40     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:823]
18:09:40     INFO -  NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:265]
18:09:40     INFO -  mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:140]
18:09:40     INFO -  MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:234]
18:09:40     INFO -  MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:509]
18:09:40     INFO -  nsBaseAppShell::Run() [widget/xpwidgets/nsBaseAppShell.cpp:166]
18:09:40     INFO -  nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:281]
18:09:40     INFO -  XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4109]
18:09:40     INFO -  XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4179]
18:09:40     INFO -  XRE_main [toolkit/xre/nsAppRunner.cpp:4394]
18:09:40     INFO -  do_main [browser/app/nsBrowserApp.cpp:282]

Comment 1

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

In txCompileObserver::startLoad, we can now use aReferrerPrincipal as the "triggering principal" and mLoaderDocument as the "context node".

And in TX_LoadSheet and txCompileObserver::loadURI we now have documents available that we can pass as context to nsIContentPolicy.

So this bug should be quite easy to fix.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1067517.patch

(In reply to Jonas Sicking (:sicking) from comment #1)
> In txCompileObserver::startLoad, we can now use aReferrerPrincipal as the
> "triggering principal" and mLoaderDocument as the "context node".
> 
> And in TX_LoadSheet and txCompileObserver::loadURI we now have documents
> available that we can pass as context to nsIContentPolicy.
> 
> So this bug should be quite easy to fix.

Indeed, here is a patch.

We have to wait till Bug 1083422 lands though, hence I will wait to set the review flag.

Comment 3

[Tanvi Vyas[:tanvi]]

Comment on attachment 8513922 [details] [diff] [review]
bug_1067517.patch

Since this is a content policy change, we should run this through try.  And set the review flag for Boris (once we land triggeringPrincipal).

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8513922 [details] [diff] [review]
bug_1067517.patch

Since we are getting close to landing Bug 1083422, I think it's time to review this one too. Jonas do you wanna do this?

Comment 5

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

Comment on attachment 8513922 [details] [diff] [review]
bug_1067517.patch

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

r=me with that fixed (or filed)

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ +462,5 @@
>      nsCOMPtr<nsIChannel> channel;
> +    nsresult rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),
> +                                                       aUri,
> +                                                       mLoaderDocument,
> +                                                       aReferrerPrincipal, // triggeringPrincipal

This is longer than 80 columns.

@@ +464,5 @@
> +                                                       aUri,
> +                                                       mLoaderDocument,
> +                                                       aReferrerPrincipal, // triggeringPrincipal
> +                                                       nsILoadInfo::SEC_NORMAL,
> +                                                       nsIContentPolicy::TYPE_STYLESHEET,

This is clearly a problem that existged before, but this should be TYPE_XSLT. Would be great if you could fix it here or in a followup.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1067517.patch

Yep, agreed. Incorporated suggestions from Jonas. Carrying over r+.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

I had this on try last night:
  https://tbpl.mozilla.org/?tree=Try&rev=dbc8f56eaee5

This is ready for checkin, but try seems closed right now. Setting checkin-needed flag - Thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/495cec2bc029

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/495cec2bc029