Open bind mount sources from the host userns #2576

alban · 2020-09-07T17:13:05Z

The source of the bind mount might not be accessible in a different usernamespace because a component of the source path might not be traversed under the users and groups mapped inside the user namespace. This caused errors such as the following:

  # time="2020-06-22T13:48:26Z" level=error msg="container_linux.go:367:
  starting container process caused: process_linux.go:459:
  container init caused: rootfs_linux.go:58:
  mounting \"/tmp/busyboxtest/source-inaccessible/dir\"
  to rootfs at \"/tmp/inaccessible\" caused:
  stat /tmp/busyboxtest/source-inaccessible/dir: permission denied"

To solve this problem, this patch performs the following:

in nsexec.c, it opens the source path in the host userns (so we have the right permissions to open it) but in the container mntns (so the kernel cross mntns mount check let us mount it later: https://github.com/torvalds/linux/blob/v5.8/fs/namespace.c#L2312).
in nsexec.c, it passes the file descriptors of the source to the child process with SCM_RIGHTS.
In runc-init in Golang, it finishes the mounts while inside the userns even without access to the some components of the source paths.

Passing the fds with SCM_RIGHTS is necessary because once the child process is in the container mntns, it is already in the container userns so it cannot temporarily join the host mntns.

This patch uses the existing mechanism with LIBCONTAINER* environment variables to pass the file descriptors from runc to runc init.

This patch uses the existing mechanism with the Netlink-style bootstrap to pass information about the list of source mounts to nsexec.c.

Fixes: #2484

TODO:

It does not work yet when the bind mount is configured as read-only in config.json.
Unit tests fail.
a single env var to pass on all the mount fds

alban · 2020-09-08T10:12:30Z

It does not work yet when the bind mount is configured as read-only in config.json.

This was not a bug but normal kernel behaviour because I tested without mount flags nosuid,nodev. When I test with the correct flags, it works. For details, see: #1229 (comment)

alban · 2020-09-09T09:27:13Z

Unit tests fail.

Unit tests seem to fail on the master branch too, it does not seem related to this PR.

kolyshkin · 2020-09-09T16:24:23Z

Unit tests seem to fail on the master branch too, it does not seem related to this PR.

Can you please rebase it on top of current master (merged #2580 should fix CI)

libcontainer/container_linux.go

kolyshkin

Left some comments.

libcontainer/container_linux.go

libcontainer/factory_linux.go

libcontainer/nsenter/nsexec.c

kolyshkin · 2020-09-09T18:46:36Z

In general, can we maybe use a single env var to pass on all the mount fds at once (instead of introducing a variable per each fd)? That would probably be faster and more resource-wise.

libcontainer/nsenter/nsexec.c

tests/integration/userns.bats

alban · 2020-09-10T10:45:02Z

@kolyshkin Thanks for the reviews!

I addressed all the comments except the one about the single env var to pass on all the mount fds. I added that in the TODO list.

The unit tests on Travis still fail after a rebase but I don't see the error message. Do you know why? If not, I'll continue to investigate...

alban · 2020-09-11T10:01:52Z

In general, can we maybe use a single env var to pass on all the mount fds at once (instead of introducing a variable per each fd)? That would probably be faster and more resource-wise.

Added, with _LIBCONTAINER_MOUNT_FILE_FDS containing a list of fds separated by ;.

alban · 2020-09-11T13:03:50Z

The unit tests now pass fine. Changes:

the mount source fd passing is disabled on rootless containers because the runc parent process might not have the capabilities to run setns() on the mount namespace it created. In that case, the mount would be performed as before, and inaccessible dirs would fail to mount as before. If we want to support that use case too, I don't know how to do it
whether to enabled mount source fd passing is now decided in container_linux.go sendMountSources()
the tests are skipped in the rootless case
the tests disable readonly rootfs. Not sure why it didn't work without that

man setns(2):
Changing the mount namespace requires that the caller possess both CAP_SYS_CHROOT and CAP_SYS_ADMIN capabilities in its own user namespace and CAP_SYS_ADMIN in the user namespace that owns the target mount namespace.

libcontainer/nsenter/nsexec.c

cyphar · 2020-09-12T05:47:51Z

libcontainer/factory_linux.go

+				envMountFileFds, err)
+		}
+		mountFile := os.NewFile(uintptr(mountFileFd), "mount-file")
+		defer mountFile.Close()


These defers will never be called because this function ends up calling Execve. There are two things we should do:

Explicitly set O_CLOEXEC on all of the file descriptors (fcntl(F_SETFL, FD_CLOEXEC) is what you want).

Close them as soon as we can.

Thanks. I added a patch that does the following:

the O_CLOEXEC is now set in nsexec.c by dup3()

close them earlier in libcontainer/init_linux.go (for the initSetns case) and in libcontainer/standard_init_linux.go (for the initStandard case).

The "defer FOO.Close()" in this function follows the same code pattern of closing other fds in case i.Init() somehow fails to run Execve.

libcontainer/nsenter/nsexec.c

rata · 2021-10-12T14:08:11Z

@kolyshkin no problem, fixed! Pushed several times to see if the DCO check runs (it says expected, but stays like that for a long time), but didn't help. PTAL :)

rata · 2021-10-12T18:33:20Z

Pushing again as make lint was failing with timeout (locally it passes just fine). Let's see if the CI passes now, if not will try again tomorrow.

kolyshkin · 2021-10-12T20:56:19Z

libcontainer/integration/utils_test.go

+
+		if strings.HasPrefix(tempBase, path) {
+			// We can't safely change permissions if it is not below tempBase.
+			if stats.Mode()&0o5 == 0 {


Looks like the check is wrong. Consider the case when only r or x is set.

haha, I'm embarrassed I missed it 🙈 . I was unlucky and in practice it worked, because the dirs didn't have any permissions for others. But thanks for catching this, fixed now!

kolyshkin · 2021-10-12T20:56:34Z

libcontainer/integration/utils_test.go

+			continue
+		}
+
+		if stats.Mode()&0o5 != 0 {


Fixed too, thanks! 🙈

rata · 2021-10-13T12:58:45Z

@kolyshkin PTAL

libcontainer/integration/utils_test.go

Add a unit test to check that bind mounts that have a part of its path non accessible by others still work when using user namespaces. To do this, we also modify newRoot() to return rootfs directories that can be traverse by others, so the rootfs created works for all test (either running in a userns or not). Signed-off-by: Mauricio Vásquez <[email protected]> Signed-off-by: Rodrigo Campos <[email protected]> Co-authored-by: Rodrigo Campos <[email protected]>

rata · 2021-10-16T15:37:57Z

@kolyshkin Thanks, PTAL. Hopefully this is ready now 🤞

kolyshkin

LGTM

rata · 2021-10-28T15:42:10Z

Thanks all for the time and review!

I think we never added release notes for this. Here they are:

Fix using bind mounts when the user in the user namespace doesn't have permission to traverse the mount path (#2484)

AkihiroSuda · 2021-11-03T08:37:10Z

tests/integration/userns.bats

+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+}


Doesn't seem to work on Fedora 35
#3258

I mentioned it in several places already, but just in case someone is looking here, the PR fixing this is: #3260

alban mentioned this pull request Sep 7, 2020

runc with user namespace enabled fails to bind mount host dirs with 750 permission #2484

Closed

alban force-pushed the alban/userns-2484-take2 branch from 8ec9e25 to 040f486 Compare September 7, 2020 17:16

alban force-pushed the alban/userns-2484-take2 branch from 040f486 to d5a10e2 Compare September 9, 2020 07:51

alban marked this pull request as ready for review September 9, 2020 09:27