rootfs: prohibit symlinks that conflicts with readonlyPaths, maskedPaths

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

Fix issue 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.

Signed-off-by: Akihiro Suda <[email protected]>

Author: AkihiroSuda

libcontainer/rootfs_linux.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
@@ -52,6 +53,10 @@ func needsSetupDev(config *configs.Config) bool {
 // finalizeRootfs after this function to finish setting up the rootfs.
 func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds []int) (err error) {
 	config := iConfig.Config
+	if err := validateRoot(config); err != nil {
+		return fmt.Errorf("error validating rootfs: %w", err)
+	}
+
 	if err := prepareRoot(config); err != nil {
 		return fmt.Errorf("error preparing rootfs: %w", err)
 	}
@@ -1115,3 +1120,51 @@ func setRecAttr(m *configs.Mount, rootfs string) error {
 		return unix.MountSetattr(-1, procfd, unix.AT_RECURSIVE, m.RecAttr)
 	})
 }
+
+// validateRoot detects symlinks in config.Rootfs that may break config.ReadonlyPaths
+// and config.MaskPaths.
+//
+// For preventing CVE-2023-27561 (CVE-2019-19921 re-introduction/regression).
+// https://github.com/opencontainers/runc/issues/3751
+func validateRoot(config *configs.Config) error {
+	mustNotBeSymlink := make(map[string]struct{})
+	for _, f := range append(config.ReadonlyPaths, config.MaskPaths...) {
+		mustNotBeSymlink[f] = struct{}{}
+		a, err := ancestorPaths(f)
+		if err != nil {
+			return fmt.Errorf("failed to compute the ancestor paths of %q: %w", f, err)
+		}
+		for _, g := range a {
+			mustNotBeSymlink[g] = struct{}{}
+		}
+	}
+	// mustNotBeSymlink typically contains {"/sys", "/proc"} in addition to readonlyPaths and maskedPaths.
+	for f := range mustNotBeSymlink {
+		if f != filepath.Clean(f) {
+			return fmt.Errorf("dirty path: %q", f)
+		}
+		// securejoin can't be used here, as it follows symlinks.
+		absMustNotBeSymlink := filepath.Join(config.Rootfs, f)
+		st, err := os.Lstat(absMustNotBeSymlink)
+		// absMustNotBeSymlink isn't always stattable, so we discard non-nil err here.
+		if err == nil && st.Mode()&fs.ModeSymlink != 0 {
+			return fmt.Errorf("must not be a symlink (conflicts with readonlyPaths and/or maskedPaths): %q", absMustNotBeSymlink)
+		}
+	}
+	return nil
+}
+
+// ancestorPaths returns ancestor paths excluding "/".
+//
+// ancestorPaths("/foo/bar/baz") = {"/foo/bar", "/foo"} ("/" is not included).
+func ancestorPaths(f string) ([]string, error) {
+	if f != filepath.Clean(f) {
+		return nil, fmt.Errorf("dirty path: %q", f)
+	}
+	dir := filepath.Dir(f)
+	if dir == "/" {
+		return []string{}, nil
+	}
+	rec, err := ancestorPaths(dir)
+	return append([]string{dir}, rec...), err
+}

libcontainer/rootfs_linux_test.go

@@ -1,6 +1,8 @@
 package libcontainer
 
 import (
+	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/opencontainers/runc/libcontainer/configs"
@@ -105,3 +107,43 @@ func TestNeedsSetupDevStrangeSourceDest(t *testing.T) {
 		t.Fatal("expected needsSetupDev to be true, got false")
 	}
 }
+
+func TestAncestorPaths(t *testing.T) {
+	type testCase struct {
+		f             string
+		expected      []string
+		expectedError string
+	}
+	testCases := []testCase{
+		{
+			f:        "/foo/bar/baz",
+			expected: []string{"/foo/bar", "/foo"},
+		},
+		{
+			f:        "/",
+			expected: []string{},
+		},
+		{
+			f:             "/foo/../../../../bar",
+			expectedError: "dirty path",
+		},
+	}
+	for _, f := range testCases {
+		got, err := ancestorPaths(f.f)
+		if f.expectedError == "" {
+			if err != nil {
+				t.Errorf("ancestorPaths(%q): %v", f.f, err)
+			}
+			if !reflect.DeepEqual(f.expected, got) {
+				t.Errorf("expected ancestorPaths(%q)=%v, got %v", f.f, f.expected, got)
+			}
+		} else {
+			if err == nil {
+				t.Errorf("expected ancestorPaths(%q) to return an error", f.f)
+			}
+			if !strings.Contains(err.Error(), f.expectedError) {
+				t.Errorf("expected ancestorPaths(%q) to return an error that contains %q", f.f, f.expectedError)
+			}
+		}
+	}
+}

tests/integration/mask.bats

@@ -6,11 +6,12 @@ function setup() {
 	setup_busybox
 
 	# Create fake rootfs.
-	mkdir rootfs/testdir
+	mkdir rootfs/testdir rootfs/testdir2
 	echo "Forbidden information!" >rootfs/testfile
+	echo "Forbidden information! (in a nested dir)" >rootfs/testdir2/file2
 
 	# add extra masked paths
-	update_config '(.. | select(.maskedPaths? != null)) .maskedPaths += ["/testdir", "/testfile"]'
+	update_config '(.. | select(.maskedPaths? != null)) .maskedPaths += ["/testdir", "/testfile", "/testdir2/testfile2"]'
 }
 
 function teardown() {
@@ -56,3 +57,41 @@ function teardown() {
 	[ "$status" -eq 1 ]
 	[[ "${output}" == *"Operation not permitted"* ]]
 }
+
+@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 not be a symlink"* ]]
+}
+
+@test "mask paths [prohibit symlink /sys]" {
+	ln -s /symlink rootfs/sys
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	[ "$status" -eq 1 ]
+	[[ "${output}" == *"must not be a symlink"* ]]
+}
+
+@test "mask paths [prohibit symlink /testdir]" {
+	rmdir rootfs/testdir
+	ln -s /symlink rootfs/testdir
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	[ "$status" -eq 1 ]
+	[[ "${output}" == *"must not be a symlink"* ]]
+}
+
+@test "mask paths [prohibit symlink /testfile]" {
+	rm -f rootfs/testfile
+	ln -s /symlink rootfs/testfile
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	[ "$status" -eq 1 ]
+	[[ "${output}" == *"must not be a symlink"* ]]
+}
+
+@test "mask paths [prohibit symlink /testdir2 (parent of /testdir2/testfile2)]" {
+	rm -rf rootfs/testdir2
+	ln -s /symlink rootfs/testdir2
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	[ "$status" -eq 1 ]
+	[[ "${output}" == *"must not be a symlink"* ]]
+}