Setuid sandbox API versioning

Setuid sandbox API versioning

We introduce API versioning to the setuid sandbox and issue warnings when
the versions Chrome and the Sandbox expect are different.

1. The Zygote launcher in the browser will export the API version it expects
to the environment.

2. The setuid sandbox will match its own version with the one in the
environment.

3. Afterwards, it will export the API it provides to the environment for the
sandboxed process.

4. The Zygote (the sandboxed process) will in turn check for the API number.

The double check is needed because a version of the browser or of the setuid
sandbox that does check for API could co-exist with a version that does not.

The various utilities that are part of the setuid sandbox are not versioned
because they have callers that are external to Chrome (in ChromeOS).

When environment variables are not found, we assume version 0. Since the API
is for now set to 0, this change will not produce any warning at the moment.

BUG=None
TEST=None


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

[jln (very slow on Chromium), 2012-06-02 02:56:12]

Setuid sandbox API versioning.

I use the environment to check/set for the setuid sandbox version (info why
below).

If nothing is found, we assume API version 0, which is also the API we provide /
expect in this version.

This means that this change should not break anything even if an old sandbox is
used with a new Chrome or a old Chrome with a new sandbox.

We should let this bake for a little bit. Then at some point we can switch the
API to 1.

PS: This is quite hairy for a number of reasons.

1. that binary got abused and performs a number of functions. It gets also
called from code that is not even in Chrome (for ChromeOS).

2. The command line parsing is extremely fragile. So I could not add a check via
the command line because things would fail in a weird way if an old sandbox got
used.

3. The Zygote launcher doesn't check for return value or anything. Instead it
gets the status by "waiting" for the Zygote to appear and could wait forever.

That's why I'm using the environment.

[Brad Chen, 2012-06-02 15:57:02]

I think this might be more useful if there were a test (probably a browser_test)
that will fail when a version mismatch occurs. I previously added a test
SUIDSandboxUITest.FLAKY_testSUIDSandboxEnabled to detect if the sandbox is
enabled. I intend to make it not FLAKY and flip the polarity of the test once
the sandbox is enabled on the bots, then it should always pass.

Maybe worth adding an adjacent test that checks sandbox version? Current test is
scraping about:sandbox.

https://chromiumcodereview.appspot.com/10492006/diff/1/base/linux_util.h
File base/linux_util.h (right):

https://chromiumcodereview.appspot.com/10492006/diff/1/base/linux_util.h#newc...
base/linux_util.h:22: static const char kSandboxEnvironmentApiRequest[] =
"SBX_API_RQ";
I'm wondering if it would be worth making these names a little bit more
self-documenting, so if somebody was groveling through setenv | less they would
know these were for Chrome. Not obvious that clarity is better in this case.

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:537: // Check if the SUID sandbox provides
the correct API version.
Are you adding these checks only for sandbox use in the bots or is it needed in
Chrome as well? I assume it's needed everywhere but if it's only useful in the
bots then it might make sense to exclude the checking code in official builds.

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:553:
"https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment."
Should this doc be updated to document the version dependency?

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

On 2012/06/02 15:57:02, Brad Chen wrote:
> I think this might be more useful if there were a test (probably a
browser_test)
> that will fail when a version mismatch occurs. I previously added a test
> SUIDSandboxUITest.FLAKY_testSUIDSandboxEnabled to detect if the sandbox is
> enabled. I intend to make it not FLAKY and flip the polarity of the test once
> the sandbox is enabled on the bots, then it should always pass.

I'm not sure about a test failing. It's a difficult problem and the big issue
here is that sending an atomic CL that will update the API is not enough because
the setuid sandbox needs to be manually updated on the build bots first.

At least for now I would prefer not to fail.

> Maybe worth adding an adjacent test that checks sandbox version? Current test
is
> scraping about:sandbox.

Should we wait a little bit for that until we can figure out a way to update the
setuid binary faster?

(Unrelated: we should try to change your test to query Zygote if possible,
about:sandbox may change soon).

[jln (very slow on Chromium), 2012-06-04 19:26:23]

https://chromiumcodereview.appspot.com/10492006/diff/1/base/linux_util.h
File base/linux_util.h (right):

https://chromiumcodereview.appspot.com/10492006/diff/1/base/linux_util.h#newc...
base/linux_util.h:22: static const char kSandboxEnvironmentApiRequest[] =
"SBX_API_RQ";
On 2012/06/02 15:57:03, Brad Chen wrote:
> I'm wondering if it would be worth making these names a little bit more
> self-documenting, so if somebody was groveling through setenv | less they
would
> know these were for Chrome. Not obvious that clarity is better in this case.

Done. I didn't change the old environment variables names though, for
compatibility reasons.

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
File content/zygote/zygote_main_linux.cc (right):

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:537: // Check if the SUID sandbox provides
the correct API version.
On 2012/06/02 15:57:03, Brad Chen wrote:
> Are you adding these checks only for sandbox use in the bots or is it needed
in
> Chrome as well? I assume it's needed everywhere but if it's only useful in the
> bots then it might make sense to exclude the checking code in official builds.

Ohh yes, both. Well, in official builds it's unlikely to happen (because both
the sandbox and Chrome will be built at the same time).

But if anything, it should be more strict on Release builds, shouldn't it?

https://chromiumcodereview.appspot.com/10492006/diff/1/content/zygote/zygote_...
content/zygote/zygote_main_linux.cc:553:
"https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment."
On 2012/06/02 15:57:03, Brad Chen wrote:
> Should this doc be updated to document the version dependency?

Yes, good point. I'll do it when this lands.

[Jorge Lucangeli Obes, 2012-06-04 21:09:04]

Mechanism looks sane.

lgtm

[Markus (顧孟勤), 2012-06-04 21:41:06]

lgtm

With the exception of confusing return codes, this looks good.

But you really shouldn't return "1" from a "bool" function, if you want to
signal failure :-) Once that is fixed, you are welcome to check in the code
without needing another review from me.

[jln (very slow on Chromium), 2012-06-04 21:49:14]

On 2012/06/04 21:41:06, Markus (顧孟勤) wrote:
> lgtm
> 
> With the exception of confusing return codes, this looks good.
> 
> But you really shouldn't return "1" from a "bool" function, if you want to
> signal failure :-) Once that is fixed, you are welcome to check in the code
> without needing another review from me.

Oops! Fixed.

Brad, Chase, does this look ok to you ?

[Brad Chen, 2012-06-04 22:58:48]

LGTM

[jln (very slow on Chromium), 2012-06-04 23:03:57]

Brett, could you please approve this for /base/linux_util.h ?

[brettw, 2012-06-04 23:21:59]

base LGTM

https://chromiumcodereview.appspot.com/10492006/diff/14001/content/browser/zy...
File content/browser/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10492006/diff/14001/content/browser/zy...
content/browser/zygote_host_impl_linux.cc:44: scoped_ptr<base::Environment>
env(base::Environment::Create());
You got 3 space indents here. Undent one.

[commit-bot: I haz the power, 2012-06-04 23:52:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10492006/19001

[commit-bot: I haz the power, 2012-06-05 01:03:36]

Change committed as 140456