Closed Bug 1067517 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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]
Blocks: 1006868
Depends on: 1038756
Blocks: 1006881
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.
Depends on: 1076836
Depends on: 1083422
Attached patch bug_1067517.patch (obsolete) — Splinter Review
(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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
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).
Attachment #8513922 - Flags: feedback+
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?
Attachment #8513922 - Flags: review?(jonas)
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.
Attachment #8513922 - Flags: review?(jonas) → review+
Yep, agreed. Incorporated suggestions from Jonas. Carrying over r+.
Attachment #8513922 - Attachment is obsolete: true
Attachment #8521537 - Flags: review+
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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/495cec2bc029
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.