Baseline minijail with a commandline switch driven main.

Baseline minijail with a commandline switch driven main.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=342

[Will Drewry, 2009-12-05 02:42:37]

I'd love to get some feedback on this (structure, basic approach, etc). I need
to shuffle some comments around and add some missing ones, but this is a start.

Also, I might pull in code.google.com/p/googlemock if there are no complaints as
it'll be easier to mock various Env responses that way... If not, I'll just
heavily macro-ize.

thanks!
will

[Chris Masone, 2009-12-05 20:23:13]

On 2009/12/05 02:42:37, wad wrote:
> I'd love to get some feedback on this (structure, basic approach, etc).

The only thing I don't really dig is that a Minijail implementation is tied to a
specific Options implementation, and there's no real way to enforce that except,
I guess, when you do that cast in MiniJail::Jail :-/  Other than that, the
approach and stuff looks fine to me

 I need
> to shuffle some comments around and add some missing ones, but this is a
start.
> 
> Also, I might pull in code.google.com/p/googlemock if there are no complaints
as
> it'll be easier to mock various Env responses that way... If not, I'll just
> heavily macro-ize.
> 
> thanks!
> will

[Chris Masone, 2009-12-05 20:23:23]

http://codereview.chromium.org/466049/diff/1001/2002
File src/platform/minijail/env.cc (right):

http://codereview.chromium.org/466049/diff/1001/2002#newcode65
src/platform/minijail/env.cc:65: if (prctl(PR_SET_SECUREBITS, 0x3f)) {
is there an explanation somewhere of what 0x3f means in this context?

http://codereview.chromium.org/466049/diff/1001/2002#newcode82
src/platform/minijail/env.cc:82: PLOG(FATAL) << "Failed to change to uid " <<
gid;
s/gid/uid/

http://codereview.chromium.org/466049/diff/1001/2002#newcode99
src/platform/minijail/env.cc:99: LOG_IF(FATAL, cap > 63) << "Valid capabilities
exceeded expected maximum: "
don't you want to relate this to sizeof(cap), not 63?

http://codereview.chromium.org/466049/diff/1001/2006
File src/platform/minijail/minijail.cc (right):

http://codereview.chromium.org/466049/diff/1001/2006#newcode33
src/platform/minijail/minijail.cc:33: // or this will fail.
I don't understand this comment :-)

http://codereview.chromium.org/466049/diff/1001/2008
File src/platform/minijail/minijail_main.cc (right):

http://codereview.chromium.org/466049/diff/1001/2008#newcode46
src/platform/minijail/minijail_main.cc:46: static const char kNamespacePid[] =
"namespace-pid";
Since this is kinda neutered without add-readonly-mounts and namespace-vfs, does
it make sense to keep them separate?  For testing it might, but maybe have an
uber-option that does all three, so that it's less likely we'll set only a
subset of these protections by accident?

http://codereview.chromium.org/466049/diff/1001/2008#newcode59
src/platform/minijail/minijail_main.cc:59: static const std::string kHelp =
"help";
why is this one a string?

[Will Drewry, 2009-12-07 16:04:27]

On 2009/12/05 20:23:13, cmasone wrote:
> On 2009/12/05 02:42:37, wad wrote:
> > I'd love to get some feedback on this (structure, basic approach, etc).
> 
> The only thing I don't really dig is that a Minijail implementation is tied to
a
> specific Options implementation, and there's no real way to enforce that
except,
> I guess, when you do that cast in MiniJail::Jail :-/  Other than that, the
> approach and stuff looks fine to me

Hrm yeh.  Originally, I was laying it out so that you can implement any sort of
"jail" based on the Interface and Options.  However, this is meant for minijail
and I'd like to make mocking easier.  I'll bump the setters/getters up a level
to Options and make them virtual.  That'll make ti easy to mock and default
values can be set.  Since the options just expose Env functionality, it doesn't
seem like we're losing anything (except bouncing through the vtables).

I'll clean that up and the rest of the comments soon

>  I need
> > to shuffle some comments around and add some missing ones, but this is a
> start.
> > 
> > Also, I might pull in code.google.com/p/googlemock if there are no
complaints
> as
> > it'll be easier to mock various Env responses that way... If not, I'll just
> > heavily macro-ize.
> > 
> > thanks!
> > will

[Will Drewry, 2009-12-07 18:11:11]

Some cleanup along with the fixes you outlined.

I also need to add make_pkg.sh, etc and add libcap-dev to the dev package list.
Doh!

http://codereview.chromium.org/466049/diff/1001/2002
File src/platform/minijail/env.cc (right):

http://codereview.chromium.org/466049/diff/1001/2002#newcode65
src/platform/minijail/env.cc:65: if (prctl(PR_SET_SECUREBITS, 0x3f)) {
On 2009/12/05 20:23:23, cmasone wrote:
> is there an explanation somewhere of what 0x3f means in this context?

Yeh - it's in the kernel.  I added a constant and a link into our kernel.git.

http://codereview.chromium.org/466049/diff/1001/2002#newcode82
src/platform/minijail/env.cc:82: PLOG(FATAL) << "Failed to change to uid " <<
gid;
On 2009/12/05 20:23:23, cmasone wrote:
> s/gid/uid/

Done.

http://codereview.chromium.org/466049/diff/1001/2002#newcode99
src/platform/minijail/env.cc:99: LOG_IF(FATAL, cap > 63) << "Valid capabilities
exceeded expected maximum: "
On 2009/12/05 20:23:23, cmasone wrote:
> don't you want to relate this to sizeof(cap), not 63?

done -- sizeof(cap_mask) because I am walking the mask.

http://codereview.chromium.org/466049/diff/1001/2006
File src/platform/minijail/minijail.cc (right):

http://codereview.chromium.org/466049/diff/1001/2006#newcode33
src/platform/minijail/minijail.cc:33: // or this will fail.
On 2009/12/05 20:23:23, cmasone wrote:
> I don't understand this comment :-)

Uh mostly fixed.  The problem was that if a user calls this func and hasn't set
their own gid/uid, it will fail trying to set them to the default: 0,0. Ugh.

Still kludgy, but better-ish for now.  When we add suppl group support, I think
this can all be cleaned up a bit.

http://codereview.chromium.org/466049/diff/1001/2008
File src/platform/minijail/minijail_main.cc (right):

http://codereview.chromium.org/466049/diff/1001/2008#newcode46
src/platform/minijail/minijail_main.cc:46: static const char kNamespacePid[] =
"namespace-pid";
On 2009/12/05 20:23:23, cmasone wrote:
> Since this is kinda neutered without add-readonly-mounts and namespace-vfs,
does
> it make sense to keep them separate?  For testing it might, but maybe have an
> uber-option that does all three, so that it's less likely we'll set only a
> subset of these protections by accident?

Hmm it's separate logically so that we can do the Chrome-style LinuxSUID.  I
agree on having an uber option.  My plan is to let the CLI do one of the
following:
- specify all arguments manually
- specify a jail profile

A jail profile will be pre-configured Options classes that set a specific style
of jail up.  That was we can predefine all our jails in the code and not have to
worry about dropping important args when cutting & pasting minijail use :)

http://codereview.chromium.org/466049/diff/1001/2008#newcode59
src/platform/minijail/minijail_main.cc:59: static const std::string kHelp =
"help";
On 2009/12/05 20:23:23, cmasone wrote:
> why is this one a string?

Because I'm a tool.

Done.

[Chris Masone, 2009-12-07 18:23:26]

lgtm once you fix the wrapping

http://codereview.chromium.org/466049/diff/7001/3009
File src/platform/minijail/minijail_main.cc (right):

http://codereview.chromium.org/466049/diff/7001/3009#newcode131
src/platform/minijail/minijail_main.cc:131:
jail_opts->set_arguments(const_cast<char * const*>(jailed_argv),
loose_args.size());
wrap

http://codereview.chromium.org/466049/diff/7001/3012
File src/platform/minijail/options.cc (right):

http://codereview.chromium.org/466049/diff/7001/3012#newcode20
src/platform/minijail/options.cc:20: DLOG(INFO) << "add_readonly_mounts(true)
implies namespace_vfs(true): correcting.";
wrap

http://codereview.chromium.org/466049/diff/7001/3013
File src/platform/minijail/options.h (right):

http://codereview.chromium.org/466049/diff/7001/3013#newcode101
src/platform/minijail/options.h:101: virtual void set_sanitize_environment(bool
val) { sanitize_environment_ = val; }
wrap

http://codereview.chromium.org/466049/diff/7001/3013#newcode142
src/platform/minijail/options.h:142: virtual void set_install_device_shims(bool
val) { install_device_shims_ = val; }
wrap

[Will Drewry, 2009-12-07 19:09:26]

I'll commit in a min then start in on getting package builds and chroot support,
etc.

thanks!

http://codereview.chromium.org/466049/diff/7001/3009
File src/platform/minijail/minijail_main.cc (right):

http://codereview.chromium.org/466049/diff/7001/3009#newcode131
src/platform/minijail/minijail_main.cc:131:
jail_opts->set_arguments(const_cast<char * const*>(jailed_argv),
loose_args.size());
On 2009/12/07 18:23:26, cmasone wrote:
> wrap

Done.

http://codereview.chromium.org/466049/diff/7001/3012
File src/platform/minijail/options.cc (right):

http://codereview.chromium.org/466049/diff/7001/3012#newcode20
src/platform/minijail/options.cc:20: DLOG(INFO) << "add_readonly_mounts(true)
implies namespace_vfs(true): correcting.";
On 2009/12/07 18:23:26, cmasone wrote:
> wrap

Done.

http://codereview.chromium.org/466049/diff/7001/3013
File src/platform/minijail/options.h (right):

http://codereview.chromium.org/466049/diff/7001/3013#newcode101
src/platform/minijail/options.h:101: virtual void set_sanitize_environment(bool
val) { sanitize_environment_ = val; }
On 2009/12/07 18:23:26, cmasone wrote:
> wrap

Done.

http://codereview.chromium.org/466049/diff/7001/3013#newcode142
src/platform/minijail/options.h:142: virtual void set_install_device_shims(bool
val) { install_device_shims_ = val; }
On 2009/12/07 18:23:26, cmasone wrote:
> wrap
> 

Done.