Closed
Bug 1067517
Opened 10 years ago
Closed 10 years ago
Do not call contentPolicies with a nullptr as Node in txMozillaStylesheetCompiler.cpp
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ckerschb, Assigned: ckerschb)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
3.35 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
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]
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 2•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Yep, agreed. Incorporated suggestions from Jonas. Carrying over r+.
Attachment #8513922 -
Attachment is obsolete: true
Attachment #8521537 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
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
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/495cec2bc029
Keywords: checkin-needed
Target Milestone: --- → mozilla36
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/495cec2bc029
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•