Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions libcontainer/rootfs_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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)
}
Expand Down Expand Up @@ -1115,3 +1119,37 @@ 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 {
Copy link
Member

@cyphar cyphar Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openat2

Too new (kernel 5.6)

Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the problem is just that securejoin follows symlinks?

Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Member

@cyphar cyphar Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kolyshkin kolyshkin Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@kolyshkin kolyshkin Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cyphar cyphar Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

mustNotBeSymlink := make(map[string]struct{})
for _, f := range append(config.ReadonlyPaths, config.MaskPaths...) {
if !filepath.IsAbs(f) || f != filepath.Clean(f) {
return fmt.Errorf("bad path: %q", f)
}
for f != "/" {
mustNotBeSymlink[f] = struct{}{}
f = filepath.Dir(f)
}
}
// mustNotBeSymlink typically contains {"/sys", "/proc"} in addition to readonlyPaths and maskedPaths.
rootFD, err := unix.Open(config.Rootfs, unix.O_DIRECTORY|unix.O_RDONLY, 0o600)
if err != nil {
return &os.PathError{Op: "open", Path: config.Rootfs, Err: err}
}
defer unix.Close(rootFD)
for f := range mustNotBeSymlink {
// fstatat(2): "If pathname is absolute, then dirfd is ignored."
fRel := "./" + f
var st unix.Stat_t
err := unix.Fstatat(rootFD, fRel, &st, unix.AT_SYMLINK_NOFOLLOW)
if err == nil && st.Mode&unix.S_IFMT == unix.S_IFLNK {
return fmt.Errorf("must not be a symlink (conflicts with readonlyPaths and/or maskedPaths): %q", f)
}
}
return nil
}
43 changes: 41 additions & 2 deletions tests/integration/mask.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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)]" {
Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kolyshkin kolyshkin Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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"* ]]
}