Don't grant unnecessary handle privileges in OpenProcessHandle.

Don't grant unnecessary handle privileges in OpenProcessHandle.

This patch makes it harder for process handles with more privileges
to leak to untrusted places.

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

[Paweł Hajdan Jr., 2009-06-17 16:25:26]

Please review:

cpu: security
jam: some plugin stuff uses OpenProcessHandle... This patch passes on trybot,
but please help to verify that it won't break plugins. Does plugin code need any
more privileges for process handle than the new OpenProcessHandle grants?

[jam, 2009-06-17 17:04:57]

I'm not sure how this helps anything, since the only calls to it are in
ResourceMessageFilter/PluginChannel.  These are running in non-sandboxed
browser/plugin processes.

Also, RMF needs terminate access, see

if (!msg_is_ok) {
BrowserRenderProcessHost::BadMessageTerminateProcess(message.type(),
handle());
  }

[cpu_(ooo_6.6-7.5), 2009-06-17 17:25:56]

I like it. I don't know if it helps today but it is a good practice. LGTM
pending JAM comments

[Paweł Hajdan Jr., 2009-06-18 15:44:20]

Patch updated. JAM, does it look ok now?

[jam, 2009-06-18 18:40:20]

On 2009/06/18 15:44:20, Paweł Hajdan Jr. wrote:
> Patch updated. JAM, does it look ok now?

While I agree in general about only opening handles with the required privilege
(hey, who wouldn't!), my issue with the change is that having OpenProcessHandle
and OpenPrivilegedProcessHandle with the only difference is that the latter can
wait on the handle and query process information is a bit misleading to someone
looking at headers/function names.

[Paweł Hajdan Jr., 2009-06-18 20:28:04]

On 2009/06/18 18:40:20, John Abd-El-Malek wrote:
> On 2009/06/18 15:44:20, Paweł Hajdan Jr. wrote:
> > Patch updated. JAM, does it look ok now?
> 
> While I agree in general about only opening handles with the required
privilege
> (hey, who wouldn't!), my issue with the change is that having
OpenProcessHandle
> and OpenPrivilegedProcessHandle with the only difference is that the latter
can
> wait on the handle and query process information is a bit misleading to
someone
> looking at headers/function names.

Hm, I don't have better idea for this function name (if you have, I'll gladly
use it). Please also note that "privileged" handle gets PROCESS_VM_READ, which
may be quite sensitive.

I could also move OpenPrivilegedProcessHandle to chrome/test/chrome_process_util
and update comments in base/ . What do you think about that?

[jam, 2009-06-18 20:36:30]

On 2009/06/18 20:28:04, Paweł Hajdan Jr. wrote:
> On 2009/06/18 18:40:20, John Abd-El-Malek wrote:
> > On 2009/06/18 15:44:20, Paweł Hajdan Jr. wrote:
> > > Patch updated. JAM, does it look ok now?
> > 
> > While I agree in general about only opening handles with the required
> privilege
> > (hey, who wouldn't!), my issue with the change is that having
> OpenProcessHandle
> > and OpenPrivilegedProcessHandle with the only difference is that the latter
> can
> > wait on the handle and query process information is a bit misleading to
> someone
> > looking at headers/function names.
> 
> Hm, I don't have better idea for this function name (if you have, I'll gladly
> use it). Please also note that "privileged" handle gets PROCESS_VM_READ, which
> may be quite sensitive.
> 
> I could also move OpenPrivilegedProcessHandle to
chrome/test/chrome_process_util
> and update comments in base/ . What do you think about that?

I don't feel that strongly about it to make you move that code around, so you
can check in as is.