New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846959 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

Expand Minijail documentation around -i/-I.

Project Member Reported by semenzato@chromium.org, May 25 2018

Issue description

Just a couple of thoughts for minijail after my recent setup experience. Could be a decent intern/noogler starter project.

1. it's a bit strange to have to lock down a number of things of which I barely know the existence (for instance, system V IPC).  A better default would be: lock down everything, then have flags to selectively unlock what I need.

1.1 I realize that backward compatibility works better this way.  However that can be fixed in other ways, for instance with a --version flag which preserves the lockdown behavior when new restrictions are added, or even encode the version number in the name of the executable (and is that maybe why it's called minijail0?)

2. Either way it would be useful to have long names for all flags.  In fact it does not seem too useful to have short names at all --- this isn't really something that needs shorthand.  But it's probably it's better to leave them in for compatibility.
 

Comment 1 by vapier@chromium.org, May 29 2018

(1) i think our goal with the --profile option is to do just this ... provide a good set of defaults that most things can work off of

(2) we've been looking at using long options for new flags and moving away from short options unless the short char is kind of obvious.  we haven't gone back and added long options for existing short options, but there's really no reason we couldn't do that.  dropping existing options just isn't worth the hassle.

Comment 2 by vapier@chromium.org, May 29 2018

(1) i remember now why we can't do this in general: we have no way of making this future proof.  so adding an option or changing minijail to default "everything" to off would work today for the set of all things minijail tracks, but we'd be unable to add anything new to it w/out going back and retesting every single daemon/user in the system to see if turning off the new thing (which was previously on) would suddenly break it.  for example, consider that every namespace we have today did not exist when CrOS first started.  there's no way we could start turning off pid/net namespaces for every daemon and not expect a bunch to break.

so as much as the current situation is unfortunate, it's the only way we can move forward, as it requires every change to restrict something new to be tested/validated by itself.
Which still doesn't mean we cannot have a "very restricted" profile that does (1), and then we ask folks to try that first, and only use individual flags when things don't work. But Mike is absolutely correct that we cannot realistically change defaults to make them more restrictive when the use of Minijail is so widespread.
Cc: wad@chromium.org
#2 and #3 yes that's why I was mentioning "backward compatibility" and I also realize these things keep evolving.

To summarize:

1. #3 yes I would like a "very restricted" profile, but I also realize that such an option has to be carefully named because, again, of future new available restrictions.

2. regarding #2 part 2: I don't think you need to "move away" from short options, just add the long form for existing functionality (new functionality could use long options only).

3. each option should have an "unlock" version; so if you start with "lock down everything that's lockable as of kernel 4.4" (for instance) then you can unlock what you need.

4. so there are multiple versions of "lock down everything"---I don't know if they can be the same as kernel versions, maybe safer to have separate versioning.  That's why I was thinking of minijail0, minijail1, etc.---all linked to the same binary---Will Drewry, is that why it's called minijail0?  Otherwise you can have a "--use-version" option.

Pros:

A. easier to add daemons 
A.1 don't have to go through a list of all currently available locking options
A.2 don't have to worry about which locking is available on which kernel (I guess this is avoided now by having some options be no-ops when not supported?)

B. easier to see what capabilities each daemon needs (don't need to take the set difference of all locking options and the ones used by the daemon)

Cons:

C. the "default lockdown" list for each minijail version is not obvious (although it can be made easy to look up in the code, or even with a "--print-default-lockdown" flag).

Anyway, it's an interesting exercise in versioning, to say the least.

Comment 5 by vapier@chromium.org, Jun 21 2018

Components: -OS>Systems OS>Systems>Minijail
Also, why does minijail0 stick around after starting its child?  Shouldn't it be able to set up the environment, then just exec the daemon?

you're asking about the -i flag
Yes I guess I was asking about that.  The next question then is why it doesn't get used.  Maybe this should be a separate bug.

localhost ~ # ps x | grep minijail
 1751 ?        Ss     0:00 /sbin/minijail0 -u shill -g shill -G -n -B 20 -c 800003de0 --ambient -- /usr/bin/shill --log-level=0 --log-scopes= --vmodule=object_proxy=0,dbus_object=0,bus=0 --device-black-list=eth_test,faketap0,wlan1,wlan2,managed0,managed1,pseudomodem0p --accept-hostname-from=pseudoethernet0 --arc-device=arcbr0 --jail-vpn-clients
 1927 ?        S      0:00 minijail0 -i -l -p -N -n -v -P /var/empty -b / -b /dev -b /proc -k tmpfs /run tmpfs 0xe -b /run/dbus -t -r -S /usr/share/policy/midis-seccomp.policy -c 0 -u midis -g midis -G -- /usr/bin/midis
 1981 ?        Ss     0:00 minijail0 --profile minimalistic-mountns -l -p --uts -c 0 -n -u arc-oemcrypto -g arc-oemcrypto -b /sys -k tmpfs /run tmpfs 0xe -b /run/dbus -b /run/shill -k tmpfs /var tmpfs 0xe -b /var/lib/oemcrypto /var/lib/oemcrypto 1 -S /usr/share/policy/arc-oemcrypto-seccomp.policy -- /usr/sbin/arc-oemcrypto --allow_dev_mode --provision_test_keybox
 1994 ?        Ss     0:00 minijail0 -u cras -g cras -G -n --uts -v -l -P /var/empty -b / / -k tmpfs /run tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /run/cras /run/cras 1 -b /run/dbus /run/dbus 1 -b /run/udev /run/udev -b /dev /dev -b /dev/shm /dev/shm 1 -k proc /proc proc -b /sys /sys -k tmpfs /var tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /var/lib/metrics/ /var/lib/metrics/ 1 -S /usr/share/policy/cras-seccomp.policy -- /usr/bin/cras
 2032 ?        S      0:00 minijail0 --profile minimalistic-mountns -l -p --uts -c 0 -n -u arc-oemcrypto -g arc-oemcrypto -b /sys -k tmpfs /run tmpfs 0xe -b /run/dbus -b /run/shill -k tmpfs /var tmpfs 0xe -b /var/lib/oemcrypto /var/lib/oemcrypto 1 -S /usr/share/policy/arc-oemcrypto-seccomp.policy -- /usr/sbin/arc-oemcrypto --allow_dev_mode --provision_test_keybox
 2519 ?        S      0:00 minijail0 -u bluetooth -g bluetooth -G -i -n -l -p -v -r -t --uts -e --profile minimalistic-mountns -k /run /run tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -k /var /var tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /dev/log /dev/log 1 -b /run/dbus -b /var/lib/bluetooth -- /usr/bin/btdispatch --vmodule=
 2555 ?        Ss     0:00 /sbin/minijail0 -u bluetooth -g bluetooth -G -c 3500 -n -- /usr/libexec/bluetooth/bluetoothd --nodetach --configfile=/etc/bluetooth/main.conf
 2598 ?        S      0:00 minijail0 -i -p -v -r --uts -l --profile minimalistic-mountns -b /dev/log -b /dev/rtc -k /run /run tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /run/dbus  1 -b /run/shill -k /var /var tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /var/cache/tlsdated  1 /usr/bin/tlsdated -v -- /usr/bin/tlsdate -v -C /usr/share/chromeos-ca-certificates -l
 3165 ?        S      0:00 minijail0 --profile minimalistic-mountns -b /dev/log -b /sys -b /sys/kernel/debug  1 -k /var /var tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /var/log/memd  1 -k /run /run tmpfs MS_NODEV MS_NOEXEC MS_NOSUID mode=755,size=10M -b /run/dbus --uts -p -e -n -l -S /usr/share/policy/memd-seccomp.policy -i -- /usr/bin/memd


i think you're mixing up use cases here.  looks like -i is used in most places already which means you're also asking about -I handling.

the existence of minijail w/out -i and/or w/out -I is the normal/expected behavior which is why people have to explicitly opt into those flags.

the difference can be illustrated by shell usage.  when you're at a cmdline, which of these do you run & why ?
$ ls    <- default
$ ls &  <- -i/-I
OK, let me ask this question then: Is there any reason why the default behavior is to leave minijail0 running, instead of using the -i behavior by default.  (And perhaps adding a --foreground option for debugging).

Also, there may be a bug.  Three of the minijail0 processes in #8 were started with -i, so I would not expect to see them.
imo having -i be opt-in or opt-out is a bikeshedding exercise -- you can reasonably argue for either behavior.  at this point, the ship has sailed.

wrt the comment #8 output, that is expected behavior when the daemon isn't using -I with -p (which is the case for all three).
> that is expected behavior when the daemon isn't using -I with -p

Maybe expected, but not documented and far from obvious, at least to me.

Since the presence of useless instances of -i is not bothering anybody, then we could make -i be a no-op, and always exec the daemon (or fork and exit, if the fork is necessary).  Also add a --noexec flag to keep the fork/exec behavior should anybody need it for some reason, including debugging (but I still don't see why).

However, if the presence of useless instances of minijail0 is also not bothering anybody, then we're good.
the fork+exec logic is necessary for properly propagating some of the security bits into the new process via the preload hack when the current linux security knobs are insufficient to propagate across the exec.  which is why the -i flag exists.

understanding the -I/-p flag is predicated upon understanding pid namespaces.  every pid namespace (including the very first one) needs pid 1 to behave as an init and manage/reap children.  most daemons/programs are not written with this knowledge, so minijail by default serves as the init process inside of pid 1.  for the very small minority that can manage it, they can use -I.  investing the time/effort in updating all the daemons to do double duty as init is arguably not worth the effort when it looks like the resource overhead of letting mj do it is not that high.

on the other hand, most daemons don't need mj to wait for it to propagate back up status, and upstart handles -i behavior like a double fork/daemon, which is why it's easy to use -i in most service places.  but we also have one-off scripts in the system that run a small util through mj where we def do not want -i behavior as the script will break.  the boot firmware/touchpad stuff comes to mind.

so making -i a nop and forcing people to specify a --foreground option is possible, but the debate is whether it's worth the effort.  imo it is not, but others might feel differently.

we probably could expand the minijail0.1 man page to explain -i/-I handling in a bit more depth.
Thank you for the explanation.  I didn't think of small utilities run inside minijail.  I agree with the status quo then.
Summary: Expand Minijail documentation around -i/-I. (was: minijail suggestions)
I don't think switching -i and then adding --foreground is time well spent. Documenting the man page/documentation is, so I'm updating the bug to match that.
Owner: vapier@chromium.org
Status: Fixed (was: Untriaged)
i've expanded the -i/-I docs:
  https://android-review.googlesource.com/773165

Sign in to add a comment