Only allow proc mount if it is procfs

[crosbymichael]

Fixes #2128

This allows proc to be bind mounted for host and rootless namespace usecases but
it removes the ability to mount over the top of proc with a directory.

> sudo docker run --rm  apparmor
docker: Error response from daemon: OCI runtime create failed:
container_linux.go:346: starting container process caused "process_linux.go:449:
container init caused \"rootfs_linux.go:58: mounting
\\\"/var/lib/docker/volumes/aae28ea068c33d60e64d1a75916cf3ec2dc3634f97571854c9ed30c8401460c1/_data\\\"
to rootfs
\\\"/var/lib/docker/overlay2/a6be5ae911bf19f8eecb23a295dec85be9a8ee8da66e9fb55b47c841d1e381b7/merged\\\"
at \\\"/proc\\\" caused
\\\"\\\\\\\"/var/lib/docker/overlay2/a6be5ae911bf19f8eecb23a295dec85be9a8ee8da66e9fb55b47c841d1e381b7/merged/proc\\\\\\\"
cannot be mounted because it is not of type proc\\\"\"": unknown.

> sudo docker run --rm -v /proc:/proc apparmor

docker-default (enforce)        root     18989  0.9  0.0   1288     4 ?
Ss   16:47   0:00 sleep 20

Signed-off-by: Michael Crosby [email protected]

[crosbymichael]

Tested using the test-case in the issue.

[cyphar]

This looks reasonable, though I'm a bit iffy about allowing procfs bind-mounts inside /proc since it can also pretty easily defeat masked-paths (especially given that we didn't allow it before). We do have to bind-mount the "host" proc for some peculiar nested container scenarios, but I'm not sure if we ever need to explicitly bind-mount something under procfs (I might be mis-remembering though).

EDIT: I don't think I am mis-remembering -- #1832 only concerns the "mount /proc usecase".

[crosbymichael]

The logic to allow it came from #1832,

We can always revert that pr and that "should" fix the issue

[cyphar]

Maybe I'm just being a little bit dense (it is pretty late at night here), but unless I'm mistaken the original check was:

if path == "." || !strings.HasPrefix(path, "..") { /* error */ }

Which blocks mounting on /proc or underneath it. This was changed in #1832 to:

if path != "." && !strings.HasPrefix(path, "..")  { /* error */ }

Which just allowed /proc but still disallowed any mounts under /proc. This PR changes it to:

if path == "." || !strings.HasPrefix(path, "..")  { /* error _unless_ src is procfs */ }

Which allows mount under /proc as long as its procfs. My point was that I don't think we need to allow it, since even #1832 didn't allow it. I'd say that the special procfs check should only apply for path == "." and disallow !strings.HasPrefix(path, "..") entirely.

[crosbymichael]

@cyphar updated

[cyphar]

LGTM.

[bennofs]

Does this also check for mounting to /? If I mount on top of /, the same issue should occur, right? (Because I can just create a fake proc directory in my mount on top of /)

[crosbymichael]

@bennofs can you expand on what you mean by mounting to /? What are you mounting and where?

[crosbymichael]

ping @mrunalp @giuseppe @rhatdan Can you plz review this one?

[bennofs]

@crosbymichael If you have a volume like this in your Dockerfile:

VOLUME /

That can also override /proc, no? Even though it doesn't mount below proc.

[mrunalp]

Yeah, I will make a pass today.

[cyphar]

@bennofs Maybe we should also include a check for direct-ancestors -- though it should be noted that if you volume-mount over / then the container won't start (so as an attack it's pretty useless). For example:

% docker run -it foo bash
docker: Error response from daemon: OCI runtime create failed: container_linux.go:345: starting container process caused "process_linux.go:430: container init caused \"open /dev/ptmx: no such file or directory\"": unknown.
% docker run foo bash
docker: Error response from daemon: OCI runtime create failed: container_linux.go:345: starting container process caused "process_linux.go:430: container init caused \"apply apparmor profile: apparmor failed to apply profile: open /proc/self/attr/exec: no such file or directory\"": unknown.

If you modify it to work around AppArmor, the process appears to die really early anyway:

% docker run foo bash
docker: Error response from daemon: OCI runtime start failed: container process is already dead: unknown.

[giuseppe]

ping @mrunalp @giuseppe @rhatdan Can you plz review this one?

the patch LGTM.

@crosbymichael @cyphar: I wonder if it possible to apply the profile before the pivot_root to be sure it happens on the host procfs

[crosbymichael]

@giuseppe i'd rather take this approach because your suggestion about moving it before pivot_root won't work with exec'd processes

[giuseppe]

I think this PR makes completely sense and we should have it. I was only thinking of moving it as an additional countermeasure

[cyphar]

CVE-2019-16884 has been assigned for this issue.

[mrunalp]

LGTM

[mrueg]

@cyphar @crosbymichael can you release a new RC for this?

[cyphar]

@mrueg This fix is not sufficient to fix the CVE completely -- see the discussion in #2128. We need to merge #2130 and opencontainers/selinux#59 first. Once that's done, I'll send out a vote for rc9.

[rhatdan]

The SELinux fix is merged.

[cyphar]

Yup, I'm updating #2130 to include a vendor bump (though strangely vndr appears to be breaking quite badly for me -- I wonder if it's because of Go modules...).

[rhatdan]

Looks like everything is merged, are we cutting a new release?

[cyphar]

Yes, I will send out the vote tomorrow morning (it's quite late here).