Prohibit /proc and /sys to be symlinks

[kolyshkin]

Commit 3291d66 introduced a check for /proc and /sys, making sure the destination (dest) is a directory (and not e.g. a symlink).

Later, a hunk from commit 0ca91f4 switched from using filepath.Join to SecureJoin for dest. As SecureJoin follows and resolves symlinks, the check whether dest is a symlink no longer works.

To fix, do the check without/before using SecureJoin.

Add integration tests (stolen from #3756) to make sure we won't regress.

This is an alternative to #3756.

Fixes: #3751
Fixes: CVE-2023-27561

[cyphar]

With the above comment in mind, I prefer this patch to #3756.

[thaJeztah]

Do we need to filepath.Clean() before checking the prefix?

[cyphar]

dest := filepath.Clean("/"+m.Destination) probably makes the most sense.

[kolyshkin]

In the light of that, I guess, we need to finish what we started in commits 1f1e91b and b31a934. Before doing that, I guess, we could not use filepath.Clean("/"+m.Destination).

[kolyshkin]

And once we'll finish it, it won't be necessary to prepend "/". So, I guess, bare filepath.Clean is enough.

[kolyshkin]

OK, so I have to add this:

--- a/tests/integration/mask.bats
+++ b/tests/integration/mask.bats
@@ -65,6 +65,9 @@ function teardown() {
 }
 
 @test "mask paths [prohibit symlink /sys]" {
+       # In rootless containers, /sys is a bind mount not a real sysfs.
+       requires root
+
        ln -s /symlink rootfs/sys
        runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
        [ "$status" -eq 1 ]

because in rootless containers /sys is not a sysfs mount but a bind mount, so the sysfs case is not working.

I guess it's not a problem for rootless containers anyway, so relaxing the check is fine.

[kolyshkin]

Thinking about one more alternative, which is:

1. ensure dest is cleaned
2. open an fd at roofs+dest with O_PATH|O_NOFOLLOW|O_DIRECTORY
3. In case of ENOENT, mkdir and retry once; bail out for other errors.
4. pass the fd to mountPropagate

This way we'll remove the TOCTOU race and we don't even need openat2.

WDYT @cyphar ?

[kolyshkin]

Thinking about one more alternative, which is:

1. ensure dest is cleaned
2. open an fd at roofs+dest with O_PATH|O_NOFOLLOW|O_DIRECTORY

This might cause rootfs escape if any parent components of dest are symlinks.

So, maybe, do readlink on the file descriptor and check that rootfs is its prefix,
and bail out otherwise.

3. In case of ENOENT, mkdir and retry once; bail out for other errors.
4. pass the fd to mountPropagate

This way we'll remove the TOCTOU race and we don't even need openat2.

WDYT @cyphar ?

[cyphar]

@kolyshkin I don't really like that idea. I would prefer if we just use this solution now and we can solve this (and the many other issues we have) with libpathrs in the future.

You can have container configs that mount sysfs and procfs in places other than /sys and /proc and I'd be far too concerned with that resulting in another awful symlink-related container attack.

[cyphar]

LGTM.

[thaJeztah]

Probably not for this PR, but I noticed that we're inconsistent in handling "where to return". It looks like all switch cases have a return, except for one, and that one is put outside of the switch;

runc/libcontainer/rootfs_linux.go

Lines 475 to 478 in efad7a3

	if err := setRecAttr(m, rootfs); err != nil {
	return err
	}
	return nil

Perhaps we should move that one inside the case "bind" branch to make it clearer what condition it relates to. Doing so may also help linters detect that we're missing a return statement if we would add a new branch to any of the switches.

runc/libcontainer/rootfs_linux.go

Line 436 in efad7a3

case "bind":

[thaJeztah]

LGTM

@AkihiroSuda PTAL

[thaJeztah]

I guess this one needs to be cherry-picked into the release-1.1 branch?

[thaJeztah]

opened #3785

[kolyshkin]

Probably not for this PR, but I noticed that we're inconsistent in handling "where to return". It looks like all switch cases have a return, except for one, and that one is put outside of the switch;

runc/libcontainer/rootfs_linux.go

Lines 475 to 478 in efad7a3

	if err := setRecAttr(m, rootfs); err != nil {
	return err
	}
	return nil

Perhaps we should move that one inside the case "bind" branch to make it clearer what condition it relates to. Doing so may also help linters detect that we're missing a return statement if we would add a new branch to any of the switches.

runc/libcontainer/rootfs_linux.go

Line 436 in efad7a3

case "bind":

Makes sense, opened #3787