should we use MS_NODEV|MS_NOEXEC|MS_NOSUID everywhere by default ?  we could add pseudo flags like MS_DEV that would disable that, although at the cost of there not being a real "MS_DEV" flag.  the alternative would be that, if they specify any MS bits, then we don't add nodev/noexec/nosuid automatically.  maybe that's good enough.

for tmpfs+mode, changing the default to mode=0755 sounds OK.

from a grep of /etc/init/ and /usr/share/cros/init/ for all the boards i have built locally, only cras was omitting the flags for the -k option, and Allen has fixed that.  so i don't think we'll see any regressions on the CrOS side.

landed default MS flags now:
  https://android-review.googlesource.com/729648

tmpfs is up now:
  https://android-review.googlesource.com/737002

this has landed now and been uprevved in ToT:
  https://chromium-review.googlesource.com/1196089

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/07aab0ef7b8eca06e0be9a2181cb6497515aaf97

commit 07aab0ef7b8eca06e0be9a2181cb6497515aaf97
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Sep 05 04:03:53 2018

minijail: uprev

Roll in these changes:
  b7803c810f46 minijail0: change default mount settings for tmpfs mounts
  aeab0e125494 Remove semi-pointless info() message.
  cb8674d56e3f minijail0: change default mount flags with -k
  3d98f3cdfd5e Rename running_with_asan_or_hwasan (NFC).
  cf48b4411d34 drop syscall filter line length check
  825828c2757c Skip setting seccomp filter under HWASan, same as ASan.

BUG= chromium:873216 
TEST=precq passes

Change-Id: I2a4cce4b392076caaf67ade91b069c588309a6b5
Reviewed-on: https://chromium-review.googlesource.com/1196089
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[rename] https://crrev.com/07aab0ef7b8eca06e0be9a2181cb6497515aaf97/chromeos-base/minijail/minijail-6-r22.ebuild

Does it make sense to automatically add uid=<UID> and gid=<GID> when it implicitly specifies mode=0755 for a tmpfs mount if minijail0 is run with -g and -u options?
Tmpfs with uid=0,gid=0,mode=0755 doesn't allow the daemon to write any data if -g and -u are specified.

That sounds reasonable to me.

i'm 50/50 on that.  for /var and /run usage, we use tmpfs just to get a writable place to further bind mount paths (like /run/dbus or /var/lib/xxx).  we don't really expect or want those parent paths to be writable.

off the top of my head, it seems like /tmp is the most common tmpfs usage where we want it to be writable, but we have a -t mode that takes care of that (and uses 1777 perms).

so far we've seen /mnt usage with the arc mounter where it was relying on 1777 mode to get write access by the dropped user.  what other scenario do we expect to typically come up where we'd want uid=/gid= to be set ?

I'm not sure about other possible scenarios, but it's really difficult to respect the default settings with the current behavior because "mode=0755,size=10M" is silently dropped when any values are specified. Also, this type=="tmpfs" specific default behavior itself is surprising.

To fix this, I think we should:
 1. Automatically add "uid=<UID>,gid=<GID>" by default to make the behavior less surprising, and
 2. If values are specified, add the default values to the specified values unless these parameters are overridden by the specified values.


Regarding the tmpfs usage for bind mount path setup, does it make sense to add a new minijail0 flag dedicated for this purpose?
For the path setup, tmpfs does not need to be writable but it's still writable by default.
I think it's more convenient to provide a dedicated flag for this purpose which comes with securer default behavior.

the new behavior is no worse than the old behavior.  imo, it's objectively better.  we had people constantly omitting any settings leading to the kernel defaults being used which are bad (as noted earlier in this bug).  if people specify custom settings, needing to add mode=/size= themselves is unchanged from before my CLs here landed -- you should have been explicitly setting them already.

wrt this behavior being only for tmpfs, i don't really buy the argument that it's confusing.  people already find the kernel defaults of size=50%/mode=1777 surprising.  which is why we opted to have mj change them in the first place.  the reason it's restricted to tmpfs mounts is that the kernel tmpfs defaults are the only ones we currently find concerning.  no other filesystem defaults to mode=1777 or to allowing users to easily allocate 50% of system RAM w/out being charged back to a process.  e.g. if i malloc() a ton, the kernel OOM killer will kill that process and release resources.  but if i write a ton of data to the tmpfs mount, the kernel has nothing to kill, so instead it panics & reboots.

wrt uid/gid automatically being added, that regresses the much more common case of using /run and /var to bind mount sub paths.  now we'd have to go back through all those daemons and update them to explicitly set uid=0,gid=0.  imo, this is not an improvement for a single regression in one package.  if it was a more common pattern, we can reevaluate.

wrt parsing the option string and adding mode= and size= if they weren't specified individually, that sounds reasonable for tmpfs.  i don't think parsing it will be *too* bad, but i guess i'll have to try it and find out.

wrt read-only tmpfs, you can't mount it initially read-only ... it needs to be writable for all the -b flags to process.  so you could probably use -k /run, then use a bunch of -b /run/xxx, then use another -k to remount /run read-only.  i've filed issue 882954 to track making a dedicated option for this.

however, even if that were implemented, i don't think we should revert changing default tmpfs mode=/size=.  people have and will continue to screw this up.

Thank you for filing issue 882954.
It seems it's almost impossible to make -k securer and easier to use without causing any regressions (e.g. the CL in this bug already caused b/114654651).
Probably we should add more new options instead of tweaking the default behavior of -k, and discourage the use of -k?

Originally, -k was just a wrapper of mount(2) so what developers needed to know was the behavior of mount and tmpfs which are quite stable.
Now -k started doing complicated things itself so developers need to know what minijail0 does which frequently changes.

The problem of -k is that it's too powerful thus unsafe, but there is no other way to do the same thing.
AFAIK currently -k is used for the following purposes:
 1. Mounting tmpfs on /tmp
 2. Mounting tmpfs on /run and /var for bind mounts
 3. Mounting writable tmpfs
 4. Bind mount with customized flags
 5. Changing the mount flags of an existing mount

By providing dedicated options with safer default behaviors, we can discourage the use of -k so that we can make -k just a wrapper of mount(2) without any complicated default behaviors again which is kept just as the last resort (maybe it'll be even possible to remove -k).
For #1, we already have -t.
For #2, issue 882954 was filed to add a new option.
For #3, we can extend -t by making other parameters customizable (i.e. mode, uid, gid, path).
For #4, we can extend -b to accept flags
For #5, probably we need a new option.
WDYT?

we had to change -k defaults precisely because people consistently got it wrong (the arc++ mounter included). so saying "people just had to know mount and tmpfs" isn't really true when people don't even know those. I'd bet that even after I caught bad usage of -k in CLs, most authors already forgot the lessons learned.

not breaking people is of course ideal, but if we can make the defaults secure, then it's worth it imo. certainly one regression is nothing when the CL simultaneously fixed more than one other daemon that was getting it wrong, and future proofed other bad users.

(1) no, you should *not* be using -k to set up /tmp. use the long existing -t flag instead, or the minimalistic-mountns profile. I'm not aware of any such code, so if you are, please file bugs or post CLs.
(3) this looks exactly what -k would be for. extending -t is not the right route as that is only for /tmp. the number of users for this is very low from what I can see with all our existing calls.
(4) extending -b shouldn't be too hard. please file a bug and enumerate the use cases you want to support.
(5) this also needs a bug with an extensive review of existing cases to see if it's worthwhile. if it's an uncommon idiom, sticking with -k isn't the worst.

even if we had these extra options, I still don't see it as justification to make -k insecure again. people will continue to copy and paste things, or write from memory, or wing it from the docs. the -k option shouldn't be insecure by default if we can help it, and I'm not seeing any arguments to the contrary. no amount of testing or linting will catch all bad users, and clearly review time isn't sufficient.