Don't use no_new_privs mode when running NM process on Linux

Don't use no_new_privs mode when running NM process on Linux

BUG=378012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273785

[Lambros, 2014-05-29 01:52:27]

lgtm

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:78: //
Don't use no_new_privs mode, e.g. in case the host needs to use sudo.
Is "host" the right terminology?

[Sergey Ulanov, 2014-05-29 01:57:59]

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:78: //
Don't use no_new_privs mode, e.g. in case the host needs to use sudo.
On 2014/05/29 01:52:28, Lambros wrote:
> Is "host" the right terminology?

Yes. Native Messaging Host.

[Sergey Ulanov, 2014-05-29 02:20:38]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-05-29 02:24:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/304083002/1

[Robert Sesek, 2014-05-29 02:41:32]

The CQ bit was unchecked by rsesek@chromium.org

[Robert Sesek, 2014-05-29 02:42:42]

On 2014/05/29 02:41:32, rsesek wrote:
> The CQ bit was unchecked by mailto:rsesek@chromium.org

Please wait for a proper sign off from security.

not lgtm

[Sergey Ulanov, 2014-05-29 03:06:46]

+jschuh, jln

Robert,
Security team did sign off on the design and implementation of Native Messaging
API long time ago. crrev.com/262786 broke that feature. I'm merely trying to fix
this regression, not adding new functionality.

[Robert Sesek, 2014-05-29 19:17:58]

On 2014/05/29 03:06:46, Sergey Ulanov wrote:
> +jschuh, jln
> 
> Robert,
> Security team did sign off on the design and implementation of Native
Messaging
> API long time ago. crrev.com/262786 broke that feature. I'm merely trying to
fix
> this regression, not adding new functionality.

Right, but people seemed surprised when I mentioned that this functionality was
being used to execute a setuid binary. I don't think this was a scenario that
was considered during the initial security review, which is why it's requiring a
closer look now. I definitely appreciate trying to fix regressions, but we want
to make sure we're not introducing a security regression at the same time.

[jln (very slow on Chromium), 2014-05-29 19:18:57]

Please be careful in the future to not disable security features without talking
with security@chromium.org

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:79:
options.allow_new_privs = true;
As discussed in the bug, let's scope this to non Chrome OS only.

[Sergey Ulanov, 2014-05-29 19:57:39]

On 2014/05/29 19:18:57, jln wrote:
> Please be careful in the future to not disable security features without
talking
> with mailto:security@chromium.org

I did add rsesek@ on this CL, so it's not like I was trying to sneak up this
change without security team noticing.

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:79:
options.allow_new_privs = true;
On 2014/05/29 19:18:58, jln wrote:
> As discussed in the bug, let's scope this to non Chrome OS only.

Done.

[Sergey Ulanov, 2014-05-29 19:59:37]

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:79:
options.allow_new_privs = true;
On 2014/05/29 19:57:39, Sergey Ulanov wrote:
> On 2014/05/29 19:18:58, jln wrote:
> > As discussed in the bug, let's scope this to non Chrome OS only.
> 
> Done.

Please note that currently this code is never executed on Chrome OS (but it may
be useful in the future to fix crbug.com/334087).

[jln (very slow on Chromium), 2014-05-29 20:51:59]

lgtm

https://codereview.chromium.org/304083002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:14:
#include "chrome/common/chrome_paths.h"
#include "build/build_config.h"

[Robert Sesek, 2014-05-29 20:52:11]

LGTM, thanks

[Sergey Ulanov, 2014-05-29 21:14:52]

https://codereview.chromium.org/304083002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc
(right):

https://codereview.chromium.org/304083002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_process_launcher_posix.cc:14:
#include "chrome/common/chrome_paths.h"
On 2014/05/29 20:52:00, jln wrote:
> #include "build/build_config.h"

Done.

[Sergey Ulanov, 2014-05-29 21:15:21]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:16:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/304083002/30001

[commit-bot: I haz the power, 2014-05-30 01:09:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10741)

[commit-bot: I haz the power, 2014-05-30 01:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 01:38:11]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10791)

[Sergey Ulanov, 2014-05-30 05:09:22]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-05-30 05:12:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sergeyu@chromium.org/304083002/50001

[commit-bot: I haz the power, 2014-05-30 10:24:33]

Change committed as 273785