Sandbox built-in flash player. Spawn broker...

Sandbox built-in flash player. Spawn broker
- Now is chrome duty to spawn flash broker. Flash cannot do it by itself on XP

The flash broker is hosted in rundll32.exe. An extra switch is added to the command line
of the plug-in process so flash player can contact its broker.

BUG=50796
TEST=see bug for testing info


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60018

[cpu_(ooo_6.6-7.5), 2010-09-20 02:20:40]

also forgot to add that I deleted CHROME_FLASH_ROOT that promised before.

[nsylvain, 2010-09-20 17:47:59]

A few questions below

http://codereview.chromium.org/3432014/diff/1/2
File chrome/common/sandbox_policy.cc (right):

http://codereview.chromium.org/3432014/diff/1/2#newcode312
chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const FilePath&
plugin_path, CommandLine* cmd_line) {
Do you think this function really belongs in this file? It does not seem to do
much about the sandbox. But i don't have a strong opinion

http://codereview.chromium.org/3432014/diff/1/2#newcode313
chrome/common/sandbox_policy.cc:313: 
i'd delete this empty line

http://codereview.chromium.org/3432014/diff/1/2#newcode318
chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths with
spaces, so we use the short path.
even when it's quoted?

http://codereview.chromium.org/3432014/diff/1/2#newcode341
chrome/common/sandbox_policy.cc:341: bool
ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line,
unused parameter?

http://codereview.chromium.org/3432014/diff/1/2#newcode392
chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path,
cmd_line)) {
what can cause that to fail? Why not just abort?

[rvargas (doing something else), 2010-09-20 18:24:48]

drive by.

http://codereview.chromium.org/3432014/diff/1/2
File chrome/common/sandbox_policy.cc (right):

http://codereview.chromium.org/3432014/diff/1/2#newcode317
chrome/common/sandbox_policy.cc:317: rundll =
rundll.AppendASCII("rundll32.exe");
Is it possible to use something more specific? Maybe another Chrome.exe? It's
better not to have processed related to us that are not clearly marked as such.

http://codereview.chromium.org/3432014/diff/1/2#newcode320
chrome/common/sandbox_policy.cc:320: if (0 ==
::GetShortPathNameW(plugin_path.value().c_str(),
Do we care about systems that have short names disabled?. We'll see failures
from the field.

http://codereview.chromium.org/3432014/diff/1/2#newcode327
chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph;
nit: ph -> process

[cpu_(ooo_6.6-7.5), 2010-09-20 23:26:08]

Good comments. Updated code. please look again.

http://codereview.chromium.org/3432014/diff/1/2
File chrome/common/sandbox_policy.cc (right):

http://codereview.chromium.org/3432014/diff/1/2#newcode312
chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const FilePath&
plugin_path, CommandLine* cmd_line) {
On 2010/09/20 17:47:59, nsylvain wrote:
> Do you think this function really belongs in this file? It does not seem to do
> much about the sandbox. But i don't have a strong opinion

I think it does. At least the launching of all processes happens in this file
(lines 521, 566).

http://codereview.chromium.org/3432014/diff/1/2#newcode313
chrome/common/sandbox_policy.cc:313: 
On 2010/09/20 17:47:59, nsylvain wrote:
> i'd delete this empty line

Done.

http://codereview.chromium.org/3432014/diff/1/2#newcode317
chrome/common/sandbox_policy.cc:317: rundll =
rundll.AppendASCII("rundll32.exe");
On 2010/09/20 18:24:49, rvargas wrote:
> Is it possible to use something more specific? Maybe another Chrome.exe? It's
> better not to have processed related to us that are not clearly marked as
such.

Not sure what you mean here. Acrobat reader and media player have random
processes as childs. Task manager does not show the relation, you need process
explorer for that.

http://codereview.chromium.org/3432014/diff/1/2#newcode318
chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths with
spaces, so we use the short path.
On 2010/09/20 17:47:59, nsylvain wrote:
> even when it's quoted?

rundll32 does not understand quotes there :( I tried.

http://codereview.chromium.org/3432014/diff/1/2#newcode320
chrome/common/sandbox_policy.cc:320: if (0 ==
::GetShortPathNameW(plugin_path.value().c_str(),
On 2010/09/20 18:24:49, rvargas wrote:
> Do we care about systems that have short names disabled?. We'll see failures
> from the field.

Yes we care. In that case we return false and we bail via line 395 which allows
you to run flash with weak policy. Which requires no broker.

http://codereview.chromium.org/3432014/diff/1/2#newcode327
chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph;
On 2010/09/20 18:24:49, rvargas wrote:
> nit: ph -> process

Done.

http://codereview.chromium.org/3432014/diff/1/2#newcode341
chrome/common/sandbox_policy.cc:341: bool
ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line,
On 2010/09/20 17:47:59, nsylvain wrote:
> unused parameter?

Good catch. fixed.

http://codereview.chromium.org/3432014/diff/1/2#newcode392
chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path,
cmd_line)) {
On 2010/09/20 17:47:59, nsylvain wrote:
> what can cause that to fail? Why not just abort?

You can disable short names on a volume. It would be crazy to do so but it
happens. In that case we still run flash in a weak sandbox so that it does not
need a broker.

[nsylvain, 2010-09-20 23:35:53]

lg

On Mon, Sep 20, 2010 at 4:26 PM, <cpu@chromium.org> wrote:

> Good comments. Updated code. please look again.
>
>
>
>
> http://codereview.chromium.org/3432014/diff/1/2
> File chrome/common/sandbox_policy.cc (right):
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode312
> chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const
> FilePath& plugin_path, CommandLine* cmd_line) {
> On 2010/09/20 17:47:59, nsylvain wrote:
>
>> Do you think this function really belongs in this file? It does not
>>
> seem to do
>
>> much about the sandbox. But i don't have a strong opinion
>>
>
> I think it does. At least the launching of all processes happens in this
> file (lines 521, 566).
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode313
> chrome/common/sandbox_policy.cc:313:
>
> On 2010/09/20 17:47:59, nsylvain wrote:
>
>> i'd delete this empty line
>>
>
> Done.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode317
> chrome/common/sandbox_policy.cc:317: rundll =
> rundll.AppendASCII("rundll32.exe");
> On 2010/09/20 18:24:49, rvargas wrote:
>
>> Is it possible to use something more specific? Maybe another
>>
> Chrome.exe? It's
>
>> better not to have processed related to us that are not clearly marked
>>
> as such.
>
> Not sure what you mean here. Acrobat reader and media player have random
> processes as childs. Task manager does not show the relation, you need
> process explorer for that.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode318
> chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths
> with spaces, so we use the short path.
> On 2010/09/20 17:47:59, nsylvain wrote:
>
>> even when it's quoted?
>>
>
> rundll32 does not understand quotes there :( I tried.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode320
> chrome/common/sandbox_policy.cc:320: if (0 ==
> ::GetShortPathNameW(plugin_path.value().c_str(),
> On 2010/09/20 18:24:49, rvargas wrote:
>
>> Do we care about systems that have short names disabled?. We'll see
>>
> failures
>
>> from the field.
>>
>
> Yes we care. In that case we return false and we bail via line 395 which
> allows you to run flash with weak policy. Which requires no broker.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode327
> chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph;
> On 2010/09/20 18:24:49, rvargas wrote:
>
>> nit: ph -> process
>>
>
> Done.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode341
> chrome/common/sandbox_policy.cc:341: bool
> ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line,
> On 2010/09/20 17:47:59, nsylvain wrote:
>
>> unused parameter?
>>
>
> Good catch. fixed.
>
>
> http://codereview.chromium.org/3432014/diff/1/2#newcode392
> chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path,
> cmd_line)) {
> On 2010/09/20 17:47:59, nsylvain wrote:
>
>> what can cause that to fail? Why not just abort?
>>
>
> You can disable short names on a volume. It would be crazy to do so but
> it happens. In that case we still run flash in a weak sandbox so that it
> does not need a broker.
>
>
> http://codereview.chromium.org/3432014/show
>

[rvargas (doing something else), 2010-09-20 23:48:01]

http://codereview.chromium.org/3432014/diff/1/2
File chrome/common/sandbox_policy.cc (right):

http://codereview.chromium.org/3432014/diff/1/2#newcode317
chrome/common/sandbox_policy.cc:317: rundll =
rundll.AppendASCII("rundll32.exe");
On 2010/09/20 23:26:08, cpu wrote:
> On 2010/09/20 18:24:49, rvargas wrote:
> > Is it possible to use something more specific? Maybe another Chrome.exe?
It's
> > better not to have processed related to us that are not clearly marked as
> such.
> 
> Not sure what you mean here. Acrobat reader and media player have random
> processes as childs. Task manager does not show the relation, you need process
> explorer for that.
> 

Now I'm not sure what you mean :). Sure, anybody can have random children
processes, but we don't (so far). With task manager it is possible to see all
processes that we launch directly (chrome*), and if we are taking the trouble to
control what this plugin can do, it seems that we could fix the naming. I just
hate rundll32 :)