Drop root privileges after initialization to reduce attack surface

[cemeyer]

This is enough to load device.map (main() → grub_util_biosdisk_init (arguments.dev_map)), drop privileges to nobody:nobody after initialization, and subsequently load a Linux guest with linux and initrd commands from an ext4 disk image. Hostfs ("(host)/foo") access continues to work, as constrained by the Unix permissions of user nobody.

I would not be surprised if other functionality were broken by this change. In particular, anything in grub that writes to disk images specified in device.map is broken by this change (only because the files are preopened O_RDONLY). I don't know that this is a particularly important or desirable use case in a Bhyve context. If it is, it would be trivial to preopen the files as O_RDWR instead.

There may be other functionality that tries to access the filesystem after this point. Some of it may even be a legitimate use; if it is, we can probably preopen it before the privilege boundary.

The key consideration here is that bhyve-grub runs as root, in order to create and allocate the vmm device, but also interprets untrusted guest grub.cfg input. Grub was designed to run in a privileged context where grub.cfg was trusted, and is not especially hardened to untrusted input. In grub-bhyve we reuse it in this context, which is somewhat dangerous. This is a broad and relatively small mitigation that may prevent a class of privilege escalation by malicious or compromised guests.

A next step would be further sandboxing with Capsicum.

[cemeyer]

See also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231480

[grehan-freebsd]

I've not tested this but the idea is fine.

The main use for "(host)" is to allow a grub.cfg location to be specified on the host filesystem e.g. https://github.com/churchers/vm-bhyve/blob/master/lib/vm-guest#L165

So, matt may need a heads-up if that is going to break.

[cemeyer]

It looks like vm-bhyve's writing ${VM_DS_PATH}/${_name}/grub.cfg on the host and then invoking grub-bhyve with -r host -d ${VM_DS_PATH}/${_name}. So it doesn't need writes from the guest. This series as-is might break guest access to that path, though. To fix it, In -r host mode, we could:

1. Pre-open a provided -d parameter during initialization and use relative lookups and O_BENEATH once we have dropped permissions. Problems: this exposes other host files that are not grub.cfg to the guest; grub.cfg might not be created with o+r permissions, depending on user umask.
2. Or more narrowly, just preopen any grub.cfg we find in -d. This doesn't expose other host files vm-bhyve didn't intend to share with the guest, and because we open it while still root, shouldn't be impacted by umask.

I like (2) better and plan to prepare a 3rd patch in this series to address this use case. Thanks!

[cemeyer]

The 3rd patch in the series accounts for -r host grub.cfgs, such as used by vm-bhyve, automatically and readonly, without exposing any additional host files to the sandboxed grub-bhyve.

Please go ahead and give this a review when you get a chance, I'd love to see it integrated into grub-bhyve. I don't believe this should regress vm-bhyve. I was able to get sandboxed grub-bhyve to load a grub.cfg from my host.

[grehan-freebsd]

Thanks: looking now.

[cemeyer]

For other readers: now that it is public, the motivation for this kind of sandbox is a more general solution to the kinds of attacks recently nop'd out here:

https://svnweb.freebsd.org/ports?view=revision&revision=525916

(Recognizing that the design of GRUB and the use in grub-bhyve has a kind of fundamental trust mismatch, we should attempt to run guest code and commands only after entering an untrusted sandbox.)

[grehan-freebsd]

This all looks fine. I'll give it a run with some of the developer use cases I have to see what if any limitations might show up, and then merge.

[cemeyer]

Great, thanks!

[cemeyer]

Any update?

[grehan-freebsd]

Just hit by the unrelated PR 244549 - should be able to finish testing once that's resolved.

[cemeyer]

Ah, that one bit me too. Thanks for the update.

[cemeyer]

What testing is pending? Anything I can do to help?

[grehan-freebsd]

Managed to do some testing.

Using (local) works for interactive commands - not sure if it was intended to allow that,but it is very useful for testing.

A boot of Ubuntu 19.10 post-install looks like it writes to the guest disk image, so O_RDWR may be required.

[cemeyer]

Managed to do some testing.

Very much appreciated!

Using (local) works for interactive commands - not sure if it was intended to allow that,but it is very useful for testing.

Can you give me a pointer to more about (local)? I'm unfamiliar with it and having trouble locating the command to understand the remark.

A boot of Ubuntu 19.10 post-install looks like it writes to the guest disk image, so O_RDWR may be required.

It's a little surprising to me that Ubuntu's grub2 would be writing to the guest disk, although of course we do so with nextboot(8) (and probably should with /boot/entropy as well).

I guess I don't see any real security issues with allowing the guest-controlled GRUB to modify the guest image. Do you? I was initially thinking we could add a -w flag to permit loader disk modification, but again, it seems harmless, so why break Ubuntu installs. I will plan to switch the disk pre-opening to O_RDWR, without the flag, if you do not object to the idea.

P.S., I know I still owe you an updated D23165 and will try to get to that soon. Sorry about the delay.

[grehan-freebsd]

Braino, meant to type "(host)" but ended up as "(local)".

I think it's fine for the guest to request grub-bhyve to write to the disk image since it owns it.

No problems with D23165.

[cemeyer]

Ah, yes, (host) access works modulo access permissions of user:group nobody. If we proceed with also entering a sandbox (a future direction I think makes sense), then it will stop working (except for any implicit grub.cfg, if -r host specified). It would be relatively easy to add a mode to opt-in to (host) access in such a design. I am saving that work for a future PR.

[cemeyer]

I updated the device.map caching patch ("hostdisk: Preopen and retain fds ...") to prefer O_RDWR access, but fallback to O_RDONLY if we lack permissions (EACCES).

[grehan-freebsd]

Thanks; that fixed it. I'll merge this and drop the stdio.in.h diff from ports on top of it.

[cemeyer]

Ping?

[cemeyer]

Thanks!