Jonas, I am not a 100% sure, but I think it's safe to also remove DoURLLoadSecurityCheck, because the only two consumers of that method: * nsPluginHost::GetURLWithHeaders, as well as * nsPluginHost::PostURL end up calling nsPluginHost::NewPluginURLStream() which then creates the channel and calls asyncOpen2() on it. Or am I missing something?

Comment on attachment 8636388 [details] [diff] [review] bug_1182543_asyncopen2_nspluginhost.patch Review of attachment 8636388 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed. ::: dom/plugins/base/nsPluginHost.cpp @@ +3236,5 @@ > } > + > + > + nsSecurityFlags securityFlags = nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS > + | nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; I'd say inline this expression at the two places where it's used. It'll make it easier to look at the channel creation code and get an understanding of what security policy is used. @@ -3275,5 @@ > } > else { > // in this else branch we really don't know where the load is coming > // from and in fact should use something better than just using > // a nullPrincipal as the loadingPrincipal. Given that nsPluginHost::DoURLLoadSecurityCheck returns a failure if there's no requestingNode, should we simply bail if we get here? AsyncOpen2 will almost always fail if the loadingPrincipal is a nullprincipal anyway.

(In reply to Jonas Sicking (:sicking) from comment #2) > > else { > > // in this else branch we really don't know where the load is coming > > // from and in fact should use something better than just using > > // a nullPrincipal as the loadingPrincipal. > > Given that nsPluginHost::DoURLLoadSecurityCheck returns a failure if there's > no requestingNode, should we simply bail if we get here? AsyncOpen2 will > almost always fail if the loadingPrincipal is a nullprincipal anyway. That sounds reasonable, lets make sure that everything is fine with the last 3 callsite changes before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=878e96b8eb1e Carrying over r+ from Jonas.

Oh, one thing that I just realized is that the old nsPluginHost::DoURLLoadSecurityCheck function returns an error if the element isn't "in the document. I.e. if IsInDoc() returns false (which in turn causes GetDocument to return null). Can you make sure to add back that check?

(In reply to Jonas Sicking (:sicking) from comment #5) > Oh, one thing that I just realized is that the old > nsPluginHost::DoURLLoadSecurityCheck function returns an error if the > element isn't "in the document. I.e. if IsInDoc() returns false (which in > turn causes GetDocument to return null). > > Can you make sure to add back that check? Yes, I will make sure to add it back. Also have do run failing tests locally and debug them. Canceling the try run (from Comment 4) for now till we got those issues resolved locally.

Added back the check for IsInUncomposedDoc() which is the newer version of IsInDoc(): > NS_ENSURE_TRUE(requestingNode->IsInUncomposedDoc(), NS_ERROR_FAILURE); Also returning false in case there is no doc at all. Carrying over r+ from Jonas.

Jonas, I am afraid we can't remove the DoURLLoadSecurityCheck() from nsPluginHost.cpp. Reason is best explained by examining test_bug813906.html where CheckLoadURIWithPrincipal() [1] is called with the following arguments: > doc->NodePrincipal(): http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_bug813906.html > targetURL: chrome://browser/content/browser.xul If we remove DoURLLoadSecurityCheck() then the load is allowed because there is no call do NewPluginURLStream() [2] which would create a channel and would perform CheckLoadURIWithPrincipal() within asyncOpen2(). That might become a problem at other callsites as well. [1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#3427 [2] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#532

As discussed over IRC, probably it makes the most sense if we perform security checks in case there is no streamListener and in case there is one then ::NewPluginURLStream() is called which creates a new channel and performs the security checks within asyncOpen2() of that channel. Agreed?

Comment on attachment 8637527 [details] [diff] [review] bug_1182543_asyncopen2_nspluginhost.patch Review of attachment 8637527 [details] [diff] [review]: ----------------------------------------------------------------- I think we need to check with bz about what security checks DoLinkClick will do. ::: dom/plugins/base/nsPluginHost.cpp @@ +518,5 @@ > + nsresult rv = NS_OK; > + if (!streamListener) { > + rv = DoURLLoadSecurityCheck(pluginInst, url); > + NS_ENSURE_SUCCESS(rv, rv); > + } I think you can remove this one completely. If we run the "if (target)" codepath, then we'll end up calling OnLinkClick on the docshell while passing the plugin element as aContent. So the docshell will do the necessary security checks. What's strange is that the docshell code doesn't seem to call CheckLoadURI/CheckLoadURIWithPrincipal anywhere. I'm not sure if that's something that callers of the docshell code is expected to do, or if there's something else going on here. @@ +570,5 @@ > + // securityManager of AsyncOpen2(). > + if (!streamListener) { > + rv = DoURLLoadSecurityCheck(instance, url); > + NS_ENSURE_SUCCESS(rv, rv); > + } The same appears true here too.

(In reply to Jonas Sicking (:sicking) from comment #10) > ::: dom/plugins/base/nsPluginHost.cpp > @@ +518,5 @@ > > + nsresult rv = NS_OK; > > + if (!streamListener) { > > + rv = DoURLLoadSecurityCheck(pluginInst, url); > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > I think you can remove this one completely. If we run the "if (target)" > codepath, then we'll end up calling OnLinkClick on the docshell while > passing the plugin element as aContent. So the docshell will do the > necessary security checks. I am not sure we can remove this check. Looking at the stacktrace when running test_bug813906.html it doesn't seem that any security checks are happening between GetURLWithHeaders() and OnLinkClick(), but maybe I am missing something! #01: nsDocShell::OnLinkClick(nsIContent*, nsIURI*, char16_t const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:13284) #02: non-virtual thunk to nsDocShell::OnLinkClick(nsIContent*, nsIURI*, char16_t const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool) (/home/ckerschb/moz/mc-obj-dbg/docshell/base/Unified_cpp_docshell_base0.cpp:13324) #03: nsPluginInstanceOwner::GetURL(char const*, char const*, nsIInputStream*, void*, unsigned int) (/home/ckerschb/moz/mc/dom/plugins/base/nsPluginInstanceOwner.cpp:544) #04: nsPluginHost::GetURLWithHeaders(nsNPAPIPluginInstance*, char const*, char const*, nsNPAPIPluginStreamListener*, char const*, char const*, bool, unsigned int, char const*) (/home/ckerschb/moz/mc/dom/plugins/base/nsPluginHost.cpp:535) #05: nsPluginHost::GetURL(nsISupports*, char const*, char const*, nsNPAPIPluginStreamListener*, char const*, char const*, bool) (/home/ckerschb/moz/mc/dom/plugins/base/nsPluginHost.cpp:502) #06: ...

Jonas, as discussed over IRC I moved DoURLLoadSecurityCheck() inside 'if (target)' and filed a follow up to let docshell do the security checks.

As discussed over IRC, I moved the security checks into ::GetURL() which beautified things checks quite a bit.

Comment on attachment 8642640 [details] [diff] [review] bug_1182543_asyncopen2_nspluginhost_instanceowner.patch Review of attachment 8642640 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginInstanceOwner.cpp @@ +514,5 @@ > NS_ENSURE_TRUE(lh, NS_ERROR_FAILURE); > > + if ((0 == PL_strcmp(aTarget, "newwindow")) || > + (0 == PL_strcmp(aTarget, "_new"))) { > + aTarget = "_blank"; Don't assign to 'a' prefixed variables. The intent of the prefix is to indicate that this is a passed in argument. Create an |nsCString target| temporary and use that. @@ +534,5 @@ > + nsCOMPtr<nsIScriptSecurityManager> secMan( > + do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv)); > + NS_ENSURE_TRUE(secMan, NS_ERROR_FAILURE); > + > + rv = secMan->CheckLoadURIWithPrincipal(doc->NodePrincipal(), uri, Nit: Use 'content->NodePrincipal()' here since 'content' will be the loadingNode. It'll return the same principal as the principal of the document, so either is correct. But using 'content' makes things a bit more specific.

(In reply to Jonas Sicking (:sicking) from comment #14) > ::: dom/plugins/base/nsPluginInstanceOwner.cpp > @@ +514,5 @@ > > NS_ENSURE_TRUE(lh, NS_ERROR_FAILURE); > > > > + if ((0 == PL_strcmp(aTarget, "newwindow")) || > > + (0 == PL_strcmp(aTarget, "_new"))) { > > + aTarget = "_blank"; > > Don't assign to 'a' prefixed variables. The intent of the prefix is to > indicate that this is a passed in argument. Create an |nsCString target| > temporary and use that. In fact, we can directly assign to 'nsAutoString unitarget', no need for another temporray.

Comment on attachment 8642681 [details] [diff] [review] bug_1182543_asyncopen2_nspluginhost_instanceowner.patch Review of attachment 8642681 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginInstanceOwner.cpp @@ +537,5 @@ > + NS_ENSURE_TRUE(secMan, NS_ERROR_FAILURE); > + > + rv = secMan->CheckLoadURIWithPrincipal(content->NodePrincipal(), uri, > + nsIScriptSecurityManager::STANDARD); > + NS_ENSURE_SUCCESS(rv, rv); Jonas, apparently there are other callsites to this function besides nsPluginHost::GetURLWithHeaders() and nsPluginHost::PostURL(). See stack underneath. Test test_pluginstream_newstream.html for example uses the uri: 'file:///tmp/testframe-6' to be passed to CheckLoadURIWithPrincipal which causes the securityManager to return false. What's the best way to handle? Just whitelisting 'file:' seems not look a good plan to me. The alternative would be to move the securityChecks back, which is also not that crisp. I am sure you have a plan, right? /home/ckerschb/moz/mc/dom/plugins/base/nsPluginInstanceOwner.cpp:549 #01: nsPluginInstanceOwner::GetURL(char const*, char const*, nsIInputStream*, void*, unsigned int) (/home/ckerschb/moz/mc/dom/plugins/base/nsPluginInstanceOwner.cpp:549) #02: nsPluginStreamToFile::Write(char const*, unsigned int, unsigned int*) (/home/ckerschb/moz/mc/dom/plugins/base/nsNPAPIPluginStreamListener.cpp:95) #03: mozilla::plugins::parent::_write(_NPP*, _NPStream*, int, void*) (/home/ckerschb/moz/mc/dom/plugins/base/nsNPAPIPlugin.cpp:1028) #04: mozilla::plugins::PluginStreamParent::AnswerNPN_Write(nsCString const&, int*) (/home/ckerschb/moz/mc/dom/plugins/ipc/PluginStreamParent.cpp:43) #05: mozilla::plugins::PPluginStreamParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PPluginStreamParent.cpp:230) #06: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PPluginModuleParent.cpp:1407) #07: mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const&, unsigned long) (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1450) #08: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1300) #09: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1275) #10: void DispatchToMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), Tuple0 const&) (/home/ckerschb/moz/mc/ipc/chromium/src/base/tuple.h:388) #11: RunnableMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/task.h:311) #12: mozilla::ipc::MessageChannel::RefCountedTask::Run() (/home/ckerschb/moz/mc-obj-dbg/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:459) #13: mozilla::ipc::MessageChannel::DequeueTask::Run() (/home/ckerschb/moz/mc-obj-dbg/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:476) #14: MessageLoop::RunTask(Task*) (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:365) #15: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:375) #16: MessageLoop::DoWork() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:459) #17: mozilla::ipc::DoWorkRunnable::Run() (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:221) #18: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:868) #19: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:277) #20: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95) #21: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235) #22: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228) #23: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201) #24: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165) #25: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:280) #26: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4288) #27: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4385) #28: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4474) #29: do_main(int, char**, nsIFile*) (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:212) #30: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:399) #31: __libc_start_main (/build/buildd/eglibc-2.19/csu/libc-start.c:321) #32: _start (/home/ckerschb/moz/mc-obj-dbg/dist/bin/firefox) #33: ??? (???:???)

Ah, I quickly scanned the tree for any other callers, but my search wasn't good enough. The only solution that I can think of is to add another argument like 'bool aDoCheckLoadURIChecks'. Eventually we might want to pass in a principal or something, which we can pass in a system principal for these other callsites. But let's keep it simple for now.

Using a boolean sounds good for now. The only other callsites are within nsNPAPIPluginStreamListener.cpp and as far as I can tell there are no callsites from within JS. If you are fine with that change then I am going to fold this patch into the other one before landing. http://mxr.mozilla.org/mozilla-central/search?string=nsIPluginInstanceOwner&find=.js&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Folding those two patches together into one. Carrying over r+ from Jonas.

Thanks for doing incremental patches btw! That made reviewing a lot easier.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/22458d00ae2503f59fd8f92e18950fa39f3c7e0e changeset: 22458d00ae2503f59fd8f92e18950fa39f3c7e0e user: Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> date: Tue Aug 04 20:05:37 2015 -0700 description: Bug 1182543 - Use channel->ascynOpen2 in dom/plugins/base/nsPluginHost.cpp (r=sicking) url: https://hg.mozilla.org/integration/mozilla-inbound/rev/401e10fc988b0127bcec58bfca4199c17f4896e1 changeset: 401e10fc988b0127bcec58bfca4199c17f4896e1 user: Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> date: Tue Aug 04 20:06:03 2015 -0700 description: Bug 1182543 - Use channel->ascynOpen2 in dom/plugins/base/nsPluginHost.cpp - simplifications in instanceowner (r=sicking)

https://hg.mozilla.org/mozilla-central/rev/22458d00ae25 https://hg.mozilla.org/mozilla-central/rev/401e10fc988b