[Linux] Use PR_SET_NO_NEW_PRIVS by default in base/process/launch.h.

[Linux] Use PR_SET_NO_NEW_PRIVS by default in base/process/launch.h.

BUG=358713
R=jln@chromium.org, thakis@chromium.org

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

[Robert Sesek, 2014-04-01 21:39:57]

thakis: base/ and chrome/
jln: content/browser/zygote_host/

Had this mitigation been in place, we would have blocked part of a recent Pwnium
exploit chain. I don't think this is too invasive (most changes are just for
tests), and it's a win for security since it mitigates a class of privesc-like
bugs. My one concern is someone adding a new test and forgetting to set this
flag (it'll fail saying "failed to move to new PID namespace"), but hopefully
codesearch would rescue them.

Kees has also asked me to do a PSA since this may break some downstream dev-mode
ChromeOS tools.

Background on NO_NEW_PRIVS:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Document...

[jln (very slow on Chromium), 2014-04-01 21:54:23]

lgtm

I'm glad to see this tried with NO_NEW_PRIV as the default.

Do you want to add a clear message in linux/suid/sandbox.c?

I would add, around line 437, a check for "0 == geteuid()" with a clear message
that either NO_NEW_PRIV is set or that a an unprivilege process is attached via
ptrace. Hopefully this should lead folks to the right direction.

https://codereview.chromium.org/197213015/diff/120001/base/process/launch_pos...
File base/process/launch_posix.cc (right):

https://codereview.chromium.org/197213015/diff/120001/base/process/launch_pos...
base/process/launch_posix.cc:438: prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
Do you want to at least DCHECK_EQ(EINVAL, errno) if this returns -1?

[Nico, 2014-04-02 01:44:17]

lgtm

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_restore_browsertest.cc:930: // new child will be
a test browser process.
Since this is in a few places, would it make sense to have a
base::LaunchProcessForTesting() (maybe in a testing namespace) which does this?

[Robert Sesek, 2014-04-02 15:39:22]

On 2014/04/01 21:54:23, jln wrote:
> Do you want to add a clear message in linux/suid/sandbox.c?
> 
> I would add, around line 437, a check for "0 == geteuid()" with a clear
message
> that either NO_NEW_PRIV is set or that a an unprivilege process is attached
via
> ptrace. Hopefully this should lead folks to the right direction.

I had to move the check a little later than line 437 because of
--adjust-oom-score. But done. Let me know if you'd like me to tweak the message.

https://codereview.chromium.org/197213015/diff/120001/base/process/launch_pos...
File base/process/launch_posix.cc (right):

https://codereview.chromium.org/197213015/diff/120001/base/process/launch_pos...
base/process/launch_posix.cc:438: prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
On 2014/04/01 21:54:23, jln wrote:
> Do you want to at least DCHECK_EQ(EINVAL, errno) if this returns -1?

Done.

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_restore_browsertest.cc:930: // new child will be
a test browser process.
On 2014/04/02 01:44:17, Nico wrote:
> Since this is in a few places, would it make sense to have a
> base::LaunchProcessForTesting() (maybe in a testing namespace) which does
this?

I don't know. It seems weird to expose new base/ API functions for basically
setting a single flag. If you think that's fine, I'll do it, but I'm not quite
sure it's warranted.

[Nico, 2014-04-02 15:43:58]

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_restore_browsertest.cc:930: // new child will be
a test browser process.
On 2014/04/02 15:39:22, rsesek wrote:
> On 2014/04/02 01:44:17, Nico wrote:
> > Since this is in a few places, would it make sense to have a
> > base::LaunchProcessForTesting() (maybe in a testing namespace) which does
> this?
> 
> I don't know. It seems weird to expose new base/ API functions for basically
> setting a single flag. If you think that's fine, I'll do it, but I'm not quite
> sure it's warranted.

I think it makes sense: Currently, the same 8 lines are repeated in 4+ places,
and if the new function has a good name the callers would express intent more
clearly. (But I lgtm'd without that too, so I don't insist on this if you thing
it's a bad idea.)

[jln (very slow on Chromium), 2014-04-02 19:06:55]

https://chromiumcodereview.appspot.com/197213015/diff/140001/sandbox/linux/su...
File sandbox/linux/suid/sandbox.c (right):

https://chromiumcodereview.appspot.com/197213015/diff/140001/sandbox/linux/su...
sandbox/linux/suid/sandbox.c:491: fprintf(stderr, "The setuid sandbox is not
running as root. Did the parent "
Nit: s/the parent/a parent/ ?

https://chromiumcodereview.appspot.com/197213015/diff/140001/sandbox/linux/su...
sandbox/linux/suid/sandbox.c:492: "process prctl(PR_SET_NO_NEW_PRIVS, ...)?\n");
Could you add something such as: "Are you using a debugger?" (which is the most
common case for this problem).

[Robert Sesek, 2014-04-02 19:57:04]

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://codereview.chromium.org/197213015/diff/120001/chrome/browser/sessions...
chrome/browser/sessions/session_restore_browsertest.cc:930: // new child will be
a test browser process.
On 2014/04/02 15:43:58, Nico wrote:
> On 2014/04/02 15:39:22, rsesek wrote:
> > On 2014/04/02 01:44:17, Nico wrote:
> > > Since this is in a few places, would it make sense to have a
> > > base::LaunchProcessForTesting() (maybe in a testing namespace) which does
> > this?
> > 
> > I don't know. It seems weird to expose new base/ API functions for basically
> > setting a single flag. If you think that's fine, I'll do it, but I'm not
quite
> > sure it's warranted.
> 
> I think it makes sense: Currently, the same 8 lines are repeated in 4+ places,
> and if the new function has a good name the callers would express intent more
> clearly. (But I lgtm'd without that too, so I don't insist on this if you
thing
> it's a bad idea.)

Fair enough. I didn't like creating a new LaunchProcess variant, but I added a
helper LaunchOptionsForTest() to return a base::LaunchOptions configured for a
test environment.

https://codereview.chromium.org/197213015/diff/140001/sandbox/linux/suid/sand...
File sandbox/linux/suid/sandbox.c (right):

https://codereview.chromium.org/197213015/diff/140001/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:491: fprintf(stderr, "The setuid sandbox is not
running as root. Did the parent "
On 2014/04/02 19:06:55, jln wrote:
> Nit: s/the parent/a parent/ ?

Done.

https://codereview.chromium.org/197213015/diff/140001/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:492: "process prctl(PR_SET_NO_NEW_PRIVS, ...)?\n");
On 2014/04/02 19:06:55, jln wrote:
> Could you add something such as: "Are you using a debugger?" (which is the
most
> common case for this problem).

Done.

[Nico, 2014-04-02 20:09:11]

lgtm

[jln (very slow on Chromium), 2014-04-02 20:36:02]

lgtm

[Robert Sesek, 2014-04-09 20:06:39]

Committed patchset #7 manually as r262786.