Add a sandbox policy for duplicating handles into the broker.

Add a sandbox policy for duplicating handles into the broker.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138008

[jschuh, 2012-05-18 18:21:32]

It was easiest to extend the existing DuplicateHandle policy, and the happy side
effect is that it provides very fine-grained control.

I still have to verify this solves Ben's problem, but it certainly should.

[cpu_(ooo_6.6-7.5), 2012-05-18 19:28:13]

lgtm, it actually looks cleaner than I expected. One thing can you create an
actual bug on your 'todo' and add the gpu folks.

[jschuh, 2012-05-18 20:03:24]

On 2012/05/18 19:28:13, cpu wrote:
> lgtm, it actually looks cleaner than I expected. One thing can you create an
> actual bug on your 'todo' and add the gpu folks.

Done. crbug.com/128786

[rvargas (doing something else), 2012-05-18 21:02:51]

LGTM after the two changes.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:34: if (!duplicate_rule.AddNumberMatch(IF,
HandleTarget::TARGET,
So we need two explicit rules in case we want to give a process access to
duplicate stuff anywhere. I'm fine with that, just checking.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:75: if (NULL == remote_target_process)
!IsValid()

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:81: HANDLE target_process =
remote_target_process.IsValid() ?
interesting artifact of the limitations of ScopedHandle...

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
File sandbox/src/handle_policy_test.cc (right):

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:92: TestRunner target;
No need for a target

[jschuh, 2012-05-18 21:40:53]

I checked with Ben and it looks like I need to apply this policy somewhere else.
So, expect there to be at least one more exception in sandbox_policy.cc.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:34: if (!duplicate_rule.AddNumberMatch(IF,
HandleTarget::TARGET,
On 2012/05/18 21:02:52, rvargas wrote:
> So we need two explicit rules in case we want to give a process access to
> duplicate stuff anywhere. I'm fine with that, just checking.

Yep. It's the same effort to implement either way, but I preferred this
approach.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:75: if (NULL == remote_target_process)
On 2012/05/18 21:02:52, rvargas wrote:
> !IsValid()

Done.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:81: HANDLE target_process =
remote_target_process.IsValid() ?
On 2012/05/18 21:02:52, rvargas wrote:
> interesting artifact of the limitations of ScopedHandle... 

Yeah, we can blame MS for making the process pseudo-handle and invalid handle
share the same numerical value.

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
File sandbox/src/handle_policy_test.cc (right):

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:92: TestRunner target;
On 2012/05/18 21:02:52, rvargas wrote:
> No need for a target

Done.

[rvargas (doing something else), 2012-05-18 22:07:02]

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/10389210/diff/8002/sandbox/src/handle_...
sandbox/src/handle_policy.cc:81: HANDLE target_process =
remote_target_process.IsValid() ?
On 2012/05/18 21:40:53, Justin Schuh wrote:
> On 2012/05/18 21:02:52, rvargas wrote:
> > interesting artifact of the limitations of ScopedHandle... 
> 
> Yeah, we can blame MS for making the process pseudo-handle and invalid handle
> share the same numerical value.

Except that a sane (auto-) implementation would not care about that, and
using/closing the pseudo handle works just fine

[commit-bot: I haz the power, 2012-05-18 23:18:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/10389210/10004

[commit-bot: I haz the power, 2012-05-19 00:36:55]

Try job failure for 10389210-10004 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-19 01:39:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/10389210/10004