rootfs: do not permit /proc mounts to non-directories

[cyphar]

mount(2) will blindly follow symlinks, which is a problem because it
allows a malicious container to trick runc into mounting /proc to an
entirely different location (and thus within the attacker's control for
a rename-exchange attack).

This is just a hotfix (to "stop the bleeding"), and the more complete
fix would be finish libpathrs and port runc to it (to avoid these types
of attacks entirely, and defend against a variety of other /proc-related
attacks). It can be bypased by someone having "/" be a volume controlled
by another container.

Fixes: CVE-2019-19921
Fixes #2197
Signed-off-by: Aleksa Sarai [email protected]

[cyphar]

/cc @leoluk @opencontainers/runc-maintainers

[leoluk]

LGTM

[cyphar]

/cc @opencontainers/runc-maintainers

[giuseppe]

@cyphar I've added to crun an extra check to allow procfs and sysfs to be mounted only under the rootfs, so no /foo/proc to prevent the mount to happen on a volume where another container has CAP_SYS_ADMIN access (hopefully unlikely) and could do a mount(MS_MOVE) between the mount setup and the MS_RDONLY. Do you think it makes sense to add here as well?

[cyphar]

Even given how crazy VOLUME is, I don't think you can trick Docker (or other such tools) into mounting procfs on /foo/proc. Also these are temporary restrictions, there's really no need to over-engineer them -- I want to drop this the second runc is ported to libpathrs.

Not to mention that if someone decides to create a container with CAP_SYS_ADMIN then they are running an insecure setup and it isn't our job to help protect them at that point -- there is literally nothing we can do to protect someone in that case (because the container can do many more malicious things than just mount(MS_MOVE)). If you have full-on CAP_SYS_ADMIN you can just re-mount anything as read-write. I'm not sure that the protection you mentioned actually addresses the "you have a container with CAP_SYS_ADMIN" threat model.

[thaJeztah]

/cc @justincormack

[giuseppe]

If you have full-on CAP_SYS_ADMIN you can just re-mount anything as read-write. I'm not sure that the protection you mentioned actually addresses the "you have a container with CAP_SYS_ADMIN" threat model.

true, I was thinking of a way to make the writeable /proc persistent RW when mounted on a shared volume, but I don't see a way without requiring a weird configuration (if possible at all with seccomp) to allow mount(MS_MOVE) but not allowing a remount/umount.

But even without CAP_SYS_ADMIN: if you can mount /proc on a shared bind mount, there will still be the possibility for a process in another container to open a file before the /proc mount is remounted read only. Allowing /proc just in the rootfs limits the attack as the rootfs itself must be accessible from the other malicious container.

[cyphar]

I see your point, but IMHO the threat model for OCI runtimes is not an attacker being able to control the configuration (because in that case, it's game over no matter what you do). The fact that higher-level runtimes allow for this (such as Docker's VOLUME) is something we have to deal with anyway, but I'd prefer to avoid giving anyone the impression that we can solve the general (or even semi-general) case here.

And while it isn't possible (AFAICS) to trick Docker into creating a /foo/proc mount for procfs (in such a way that it isn't also possible to mount anything), it is very possible to get Docker to make / a volume -- which as I mentioned above (and in the other issue) allows you to bypass all of these checks entirely (including the one you described).

[cyphar]

Then again, all of our masked-path logic is very much tied to the actual path (not the filesystem) so there is an argument for making procfs only mountable on /proc. My main disagreement is that I don't think that change helps protect against the threat model it's trying to protect against -- because if you can provide a way to mount procfs to a custom path there are plenty of other ways of causing trouble.

[cyphar]

/ping @opencontainers/runc-maintainers

[hqhq]

LGTM

[mrunalp]

LGTM

[thaJeztah]

Will this be tagged as v1.0.0-rc10 ?

[thaJeztah]

oh, nevermind; I see the e-mail thread 👍