From 8bd3b8786b5ca0917e8b244b48edfeae92128fcc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 18 Dec 2020 12:56:48 -0800 Subject: [PATCH 1/3] test: add case for GH #2086 Commit 5e0e67d7 ("fix permission denied") modified some code but did not provide a test case. This is a test case that was tested to fail before and succeed after the above commit. For more details, see https://github.com/opencontainers/runc/pull/2086 Signed-off-by: Kir Kolyshkin --- tests/integration/cwd.bats | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 tests/integration/cwd.bats diff --git a/tests/integration/cwd.bats b/tests/integration/cwd.bats new file mode 100644 index 000000000..fbc5eefb7 --- /dev/null +++ b/tests/integration/cwd.bats @@ -0,0 +1,30 @@ +#!/usr/bin/env bats + +load helpers + +function setup() { + teardown_busybox + setup_busybox +} + +function teardown() { + teardown_busybox +} + +# Test case for https://github.com/opencontainers/runc/pull/2086 +@test "runc exec --user with no access to cwd" { + requires root + + chown 42 rootfs/root + chmod 700 rootfs/root + + update_config ' .process.cwd = "/root" + | .process.user.uid = 42 + | .process.args |= ["sleep", "1h"]' + + runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox + [ "$status" -eq 0 ] + + runc exec --user 0 test_busybox true + [ "$status" -eq 0 ] +} From d869d05aba0fc2519060b3b20b679ac5a5068e78 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Thu, 19 Nov 2020 09:38:05 -0500 Subject: [PATCH 2/3] libctr/init_linux: reorder chdir commit 5e0e67d76cc99d76c8228d48f38f37034503f315 moved the chdir to be one of the first steps of finalizing the namespace of the container. However, this causes issues when the cwd is not accessible by the user running runc, but rather as the container user. Thus, setupUser has to happen before we call chdir. setupUser still happens before setting the caps, so the user should be privileged enough to mitigate the issues fixed in 5e0e67d76cc99d76c8228d48f38f37034503f315 Signed-off-by: Peter Hunt --- libcontainer/init_linux.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 575dd4612..bb8ff0b52 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -129,12 +129,6 @@ func finalizeNamespace(config *initConfig) error { return errors.Wrap(err, "close exec fds") } - if config.Cwd != "" { - if err := unix.Chdir(config.Cwd); err != nil { - return fmt.Errorf("chdir to cwd (%q) set in config.json failed: %v", config.Cwd, err) - } - } - capabilities := &configs.Capabilities{} if config.Capabilities != nil { capabilities = config.Capabilities @@ -156,6 +150,14 @@ func finalizeNamespace(config *initConfig) error { if err := setupUser(config); err != nil { return errors.Wrap(err, "setup user") } + // Change working directory AFTER the user has been set up. + // Otherwise, if the cwd is also a volume that's been chowned to the container user (and not the user running runc), + // this command will EPERM. + if config.Cwd != "" { + if err := unix.Chdir(config.Cwd); err != nil { + return fmt.Errorf("chdir to cwd (%q) set in config.json failed: %v", config.Cwd, err) + } + } if err := system.ClearKeepCaps(); err != nil { return errors.Wrap(err, "clear keep caps") } From 27268b1a7ecbf7d9044574dc9c4979069aa36728 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 18 Dec 2020 13:14:54 -0800 Subject: [PATCH 3/3] tests/int/cwd: add test case for cwd not owned by runc The test setup is complicated, as we want to run it as non-root, but we need to create a directory owned by a particular user, and root is needed for chown. To do that, we hijack enable_idmap. Now, there is no ideal place to clean up AUX_DIR. A function cleanup is called after the test to do so. Note that it won't be called if the test fails. This was verified to fail before and pass after the previous commit. Signed-off-by: Kir Kolyshkin --- tests/integration/cwd.bats | 29 +++++++++++++++++++++++++++++ tests/rootless.sh | 20 ++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/tests/integration/cwd.bats b/tests/integration/cwd.bats index fbc5eefb7..6fe533a91 100644 --- a/tests/integration/cwd.bats +++ b/tests/integration/cwd.bats @@ -28,3 +28,32 @@ function teardown() { runc exec --user 0 test_busybox true [ "$status" -eq 0 ] } + +# Verify a cwd owned by the container user can be chdir'd to, +# even if runc doesn't have the privilege to do so. +@test "runc create sets up user before chdir to cwd" { + requires rootless rootless_idmap + + # Some setup for this test (AUX_DIR and AUX_UID) is done + # by rootless.sh. Check that setup is done... + if [[ ! -d "$AUX_DIR" || -z "$AUX_UID" ]]; then + skip "bad/unset AUX_DIR/AUX_UID" + fi + # ... and is correct, i.e. the current user + # does not have permission to access AUX_DIR. + if ls -l "$AUX_DIR" 2>/dev/null; then + skip "bad AUX_DIR permissions" + fi + + update_config ' .mounts += [{ + source: "'"$AUX_DIR"'", + destination: "'"$AUX_DIR"'", + options: ["bind"] + }] + | .process.user.uid = '"$AUX_UID"' + | .process.cwd = "'"$AUX_DIR"'" + | .process.args |= ["ls", "'"$AUX_DIR"'"]' + + runc run test_busybox + [ "$status" -eq 0 ] +} diff --git a/tests/rootless.sh b/tests/rootless.sh index e298cc616..929fec2c2 100755 --- a/tests/rootless.sh +++ b/tests/rootless.sh @@ -50,6 +50,18 @@ function enable_idmap() { # Reactivate new{uid,gid}map helpers if applicable. [ -e /usr/bin/unused-newuidmap ] && mv /usr/bin/{unused-,}newuidmap [ -e /usr/bin/unused-newgidmap ] && mv /usr/bin/{unused-,}newgidmap + + # Create a directory owned by $AUX_UID inside container, to be used + # by a test case in cwd.bats. This setup can't be done by the test itself, + # as it needs root for chown. + set -e + export AUX_UID=1024 + AUX_DIR="$(mktemp -d)" + # 1000 is linux.uidMappings.containerID value, + # as set by runc_rootless_idmap + chown "$((ROOTLESS_UIDMAP_START - 1000 + AUX_UID))" "$AUX_DIR" + export AUX_DIR + set +e } function disable_idmap() { @@ -65,6 +77,13 @@ function disable_idmap() { [ -e /usr/bin/newgidmap ] && mv /usr/bin/{,unused-}newgidmap } +function cleanup() { + if [ -n "$AUX_DIR" ]; then + rmdir "$AUX_DIR" + unset AUX_DIX + fi +} + # FEATURE: Opportunistic cgroups support, allowing a rootless container to set # resource limits on condition that cgroupsPath is set to a path the # rootless user has permissions on. @@ -151,4 +170,5 @@ for enabled_features in $features_powerset; do sudo -HE -u rootless PATH="$PATH" $(which bats) -t "$ROOT/tests/integration$ROOTLESS_TESTPATH" fi set +e + cleanup done