| From 75a8f5acaabb024a1cbbd451b15f3d95d6e624e2 Mon Sep 17 00:00:00 2001 |
| From: Kir Kolyshkin <kolyshkin@gmail.com> |
| Date: Thu, 16 Mar 2023 14:35:50 -0700 |
| Subject: [PATCH] Prohibit /proc and /sys to be symlinks |
| |
| Commit 3291d66b9844 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 0ca91f44f 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 to make sure we won't regress. |
| |
| Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> |
| (cherry picked from commit 0d72adf96dda1b687815bf89bb245b937a2f603c) |
| Signed-off-by: Sebastiaan van Stijn <github@gone.nl> |
| --- |
| libcontainer/rootfs_linux.go | 27 +++++++++++++++++++-------- |
| tests/integration/mask.bats | 19 +++++++++++++++++++ |
| 2 files changed, 38 insertions(+), 8 deletions(-) |
| |
| diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go |
| index f96268194754..140ad26e26fa 100644 |
| --- a/libcontainer/rootfs_linux.go |
| +++ b/libcontainer/rootfs_linux.go |
| @@ -378,31 +378,42 @@ func doTmpfsCopyUp(m *configs.Mount, rootfs, mountLabel string) (Err error) { |
| |
| func mountToRootfs(m *configs.Mount, c *mountConfig) error { |
| rootfs := c.root |
| - mountLabel := c.label |
| - dest, err := securejoin.SecureJoin(rootfs, m.Destination) |
| - if err != nil { |
| - return err |
| - } |
| |
| + // procfs and sysfs are special because we need to ensure they are actually |
| + // mounted on a specific path in a container without any funny business. |
| switch m.Device { |
| case "proc", "sysfs": |
| // If the destination already exists and is not a directory, we bail |
| - // out This is to avoid mounting through a symlink or similar -- which |
| + // out. This is to avoid mounting through a symlink or similar -- which |
| // has been a "fun" attack scenario in the past. |
| // TODO: This won't be necessary once we switch to libpathrs and we can |
| // stop all of these symlink-exchange attacks. |
| + dest := filepath.Clean(m.Destination) |
| + if !strings.HasPrefix(dest, rootfs) { |
| + // Do not use securejoin as it resolves symlinks. |
| + dest = filepath.Join(rootfs, dest) |
| + } |
| if fi, err := os.Lstat(dest); err != nil { |
| if !os.IsNotExist(err) { |
| return err |
| } |
| - } else if fi.Mode()&os.ModeDir == 0 { |
| + } else if !fi.IsDir() { |
| return fmt.Errorf("filesystem %q must be mounted on ordinary directory", m.Device) |
| } |
| if err := os.MkdirAll(dest, 0o755); err != nil { |
| return err |
| } |
| - // Selinux kernels do not support labeling of /proc or /sys |
| + // Selinux kernels do not support labeling of /proc or /sys. |
| return mountPropagate(m, rootfs, "") |
| + } |
| + |
| + mountLabel := c.label |
| + dest, err := securejoin.SecureJoin(rootfs, m.Destination) |
| + if err != nil { |
| + return err |
| + } |
| + |
| + switch m.Device { |
| case "mqueue": |
| if err := os.MkdirAll(dest, 0o755); err != nil { |
| return err |
| diff --git a/tests/integration/mask.bats b/tests/integration/mask.bats |
| index 272c879c6857..7730807f1bb6 100644 |
| --- a/tests/integration/mask.bats |
| +++ b/tests/integration/mask.bats |
| @@ -75,3 +75,22 @@ function teardown() { |
| # so we merely check that it fails, and do not check the exact error |
| # message like for /proc above. |
| } |
| + |
| +@test "mask paths [prohibit symlink /proc]" { |
| + ln -s /symlink rootfs/proc |
| + runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox |
| + [ "$status" -eq 1 ] |
| + [[ "${output}" == *"must be mounted on ordinary directory"* ]] |
| +} |
| + |
| +@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 ] |
| + # On cgroup v1, this may fail before checking if /sys is a symlink, |
| + # so we merely check that it fails, and do not check the exact error |
| + # message like for /proc above. |
| +} |
| -- |
| 2.29.3 |
| |