rootfs: prohibit symlinks that conflicts with readonlyPaths and/or maskedPaths to prevent CVE-2023-27561

[AkihiroSuda]

EDIT: this PR was replaced by:

• Prohibit /proc and /sys to be symlinks #3773

---

"/proc" and "/sys" in the container rootfs can no longer be symlinks, as they could be tricked to obtain the host root.

Fix #3751 ("CVE-2019-19921 re-introduction/regression")
Fix CVE-2023-27561

The CVE has been already public, so I'm just going to open this PR publicly.

[AkihiroSuda]

(We should also try to eliminate the TOCTOU issue, but just preventing the symlinks is probably enough as a quick fix for the CVE)

[AkihiroSuda]

RFC: Should we prohibit these symlinks, or should we just prohibit symlinking /proc and /sys ?

The latter might be better for compatibility, but might be less secure.

[kolyshkin]

I think that the former is the right approach (parents should not be symlinks).

[kolyshkin]

LGTM overall, left a couple of comments about how to simplify the code.

[cyphar]

Given that this is still subject to races, I wonder if a better solution might be to make utils.WithProcfd take an argument (or have a utils.WithProcfdNoSymlinks variant) which blocks symlink traversal so that we don't allow mounting on symlinks for paths where we don't want indirection (such as for masked and read-only paths as well as procfs and sysfs)? With openat2 this can be done with RESOLVE_NO_SYMLINKS -- though I'll need to see what to do about path components... Doing it this way is a little bit closer to how libpathrs would be used so might make things easier for that transition too...

To be honest, I read the issue and I don't understand why this appears to be a race condition problem -- isn't the issue that the check we added for the original CVE is completely ignored now because utils.WithProcfd blindly resolves the symlink?

[AkihiroSuda]

openat2

Too new (kernel 5.6)

[AkihiroSuda]

Looks like the problem is just that securejoin follows symlinks?

[AkihiroSuda]

Looks like we have to continue to support following symlinks for most mount operations

runc/tests/integration/mounts.bats

Lines 51 to 65 in 6d0261c

	# https://github.com/opencontainers/runc/issues/2683
	@test "runc run [tmpfs mount with absolute symlink]" {
	# in container, /conf -> /real/conf
	mkdir -p rootfs/real/conf
	ln -s /real/conf rootfs/conf
	update_config ' .mounts += [{
	type: "tmpfs",
	source: "tmpfs",
	destination: "/conf/stack",
	options: ["ro", "nodev", "nosuid"]
	}]
	| .process.args |= ["true"]'
	runc run test_busybox
	[ "$status" -eq 0 ]
	}

/proc and /sys can be exceptions though

[AkihiroSuda]

Looks like we don't use WithProcfd for maskPath()

runc/libcontainer/rootfs_linux.go

Lines 1017 to 1030 in 214c16f

	// maskPath masks the top of the specified path inside a container to avoid
	// security issues from processes reading information from non-namespace aware
	// mounts ( proc/kcore ).
	// For files, maskPath bind mounts /dev/null over the top of the specified path.
	// For directories, maskPath mounts read-only tmpfs over the top of the specified path.
	func maskPath(path string, mountLabel string) error {
	if err := mount("/dev/null", path, "", "", unix.MS_BIND, ""); err != nil && !errors.Is(err, os.ErrNotExist) {
	if errors.Is(err, unix.ENOTDIR) {
	return mount("tmpfs", path, "", "tmpfs", unix.MS_RDONLY, label.FormatMountLabel("", mountLabel))
	}
	return err
	}
	return nil
	}

[cyphar]

I'm not sure I understand how the issue got reintroduced then... I'm taking a look...

[kolyshkin]

@cyphar the issue was reintroduced because the checking code added by 3291d66:

runc/libcontainer/rootfs_linux.go

Line 392 in 206008a

if fi, err := os.Lstat(dest); err != nil {

depends on dest being the actual path provided by a user. It stopped working as it is supposed to after commit 0ca91f4#diff-e0932af06cb46065b2ef5fc89bc2bd9b880c9578e01b89ef4ecaeffb78a8bef0L331 changed from dest = filepath.Join (rootfs, dest) to dest = securejoin.SecureJoin(rootfs, dest).

Now, SecureJoin resolves all symlinks under dest and thus checking is now done for link destination not the link itself.

[kolyshkin]

... thus the check "if dest is a symlink" is no longer working.

[cyphar]

Ah, you're right. I kept missing than when I was looking over it. I personally prefer the approach in #3773. Doing a pre-flight "scan" of the rootfs seems less than ideal (neither approach protects against races but the pre-flight one is more obviously incapable of handling races -- the approach in #3773 seems closer to what libpathrs would do and actually fixes the core bug in the CVE fix I wrote).

[AkihiroSuda]

@opencontainers/runc-maintainers What should we do with this?

[kolyshkin]

Unless @cyphar has any update on this, I think we should merge this (and backport to 1.1.5).

[kolyshkin]

OTOH here's an alternative that makes the original fix from #2207 working again: #3773

[kolyshkin]

Yet another alternative, just for the sake of completeness, is to make SecureJoin not follow symlinks. Here's what I ended up with:

  // noSymVFS is a VFS interface for securejoin which disallows symlinks in the unsafePath.
  type noSymVFS struct{}  
   
  func (o noSymVFS) Lstat(name string) (fi os.FileInfo, err error) {
          fi, err = os.Lstat(name)
          if err == nil && fi.Mode()&os.ModeSymlink == os.ModeSymlink {
                  return nil, fmt.Errorf("%q is a symlink", name)
          }
          return
  }
   
  func (o noSymVFS) Readlink(name string) (string, error) { return os.Readlink(name) }
   
  func secureJoinNoSymlinks(root, unsafePath string) (string, error) {
          return securejoin.SecureJoinVFS(root, unsafePath, &noSymVFS{})
  }
   
  func mountToRootfs(m *configs.Mount, c *mountConfig) error {
  ...          
          secJoin := securejoin.SecureJoin
          if m.Device == "proc" || m.Device == "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
                  // 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.
                  secJoin = secureJoinNoSymlinks
          }
          dest, err := secJoin(rootfs, m.Destination)
          

This has not been tested but I definitely don't like this code. The approach used in #3773 (or here) seems better.

[AkihiroSuda]

Replaced by

• Prohibit /proc and /sys to be symlinks #3773