diff options
| -rw-r--r-- | libpod/container_internal.go | 37 | ||||
| -rw-r--r-- | libpod/container_internal_linux.go | 13 | ||||
| -rw-r--r-- | libpod/util.go | 26 | ||||
| -rw-r--r-- | test/system/030-run.bats | 42 | ||||
| -rw-r--r-- | test/system/075-exec.bats | 22 | ||||
| -rw-r--r-- | test/system/400-unprivileged-access.bats | 65 | ||||
| -rw-r--r-- | test/system/410-selinux.bats | 9 | ||||
| -rw-r--r-- | test/system/helpers.bash | 32 | ||||
| -rwxr-xr-x | test/system/helpers.t | 70 |
9 files changed, 289 insertions, 27 deletions
diff --git a/libpod/container_internal.go b/libpod/container_internal.go index e98e20b9b..a79b9e5a8 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -28,7 +28,6 @@ import ( securejoin "github.com/cyphar/filepath-securejoin" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" - "github.com/opencontainers/selinux/go-selinux/label" "github.com/opentracing/opentracing-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -1759,32 +1758,40 @@ func (c *Container) postDeleteHooks(ctx context.Context) error { return nil } -// writeStringToRundir copies the provided file to the runtimedir -func (c *Container) writeStringToRundir(destFile, output string) (string, error) { +// writeStringToRundir writes the given string to a file with the given name in +// the container's temporary files directory. The file will be chown'd to the +// container's root user and have an appropriate SELinux label set. +// If a file with the same name already exists, it will be deleted and recreated +// with the new contents. +// Returns the full path to the new file. +func (c *Container) writeStringToRundir(destFile, contents string) (string, error) { destFileName := filepath.Join(c.state.RunDir, destFile) if err := os.Remove(destFileName); err != nil && !os.IsNotExist(err) { return "", errors.Wrapf(err, "error removing %s for container %s", destFile, c.ID()) } - f, err := os.Create(destFileName) - if err != nil { - return "", errors.Wrapf(err, "unable to create %s", destFileName) - } - defer f.Close() - if err := f.Chown(c.RootUID(), c.RootGID()); err != nil { + if err := writeStringToPath(destFileName, contents, c.config.MountLabel, c.RootUID(), c.RootGID()); err != nil { return "", err } - if _, err := f.WriteString(output); err != nil { - return "", errors.Wrapf(err, "unable to write %s", destFileName) - } - // Relabel runDirResolv for the container - if err := label.Relabel(destFileName, c.config.MountLabel, false); err != nil { + return destFileName, nil +} + +// writeStringToStaticDir writes the given string to a file with the given name +// in the container's permanent files directory. The file will be chown'd to the +// container's root user and have an appropriate SELinux label set. +// Unlike writeStringToRundir, will *not* delete and re-create if the file +// already exists (will instead error). +// Returns the full path to the new file. +func (c *Container) writeStringToStaticDir(filename, contents string) (string, error) { + destFileName := filepath.Join(c.config.StaticDir, filename) + + if err := writeStringToPath(destFileName, contents, c.config.MountLabel, c.RootUID(), c.RootGID()); err != nil { return "", err } - return filepath.Join(c.state.RunDir, destFile), nil + return destFileName, nil } // appendStringToRundir appends the provided string to the runtimedir file diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index b2711745e..255505416 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -214,6 +214,9 @@ func (c *Container) getUserOverrides() *lookup.Overrides { } } } + if path, ok := c.state.BindMounts["/etc/passwd"]; ok { + overrides.ContainerEtcPasswdPath = path + } return &overrides } @@ -1513,6 +1516,14 @@ func (c *Container) generatePasswd() (string, error) { if !c.config.AddCurrentUserPasswdEntry && c.config.User == "" { return "", nil } + if MountExists(c.config.Spec.Mounts, "/etc/passwd") { + return "", nil + } + // Re-use passwd if possible + passwdPath := filepath.Join(c.config.StaticDir, "passwd") + if _, err := os.Stat(passwdPath); err == nil { + return passwdPath, nil + } pwd := "" if c.config.User != "" { entry, err := c.generateUserPasswdEntry() @@ -1536,7 +1547,7 @@ func (c *Container) generatePasswd() (string, error) { if err != nil && !os.IsNotExist(err) { return "", errors.Wrapf(err, "unable to read passwd file %s", originPasswdFile) } - passwdFile, err := c.writeStringToRundir("passwd", string(orig)+pwd) + passwdFile, err := c.writeStringToStaticDir("passwd", string(orig)+pwd) if err != nil { return "", errors.Wrapf(err, "failed to create temporary passwd file") } diff --git a/libpod/util.go b/libpod/util.go index 8c2d946ba..a8d405b5f 100644 --- a/libpod/util.go +++ b/libpod/util.go @@ -18,6 +18,7 @@ import ( "github.com/cri-o/ocicni/pkg/ocicni" "github.com/fsnotify/fsnotify" spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -273,3 +274,28 @@ func makeInspectPortBindings(bindings []ocicni.PortMapping) map[string][]define. } return portBindings } + +// Write a given string to a new file at a given path. +// Will error if a file with the given name already exists. +// Will be chown'd to the UID/GID provided and have the provided SELinux label +// set. +func writeStringToPath(path, contents, mountLabel string, uid, gid int) error { + f, err := os.Create(path) + if err != nil { + return errors.Wrapf(err, "unable to create %s", path) + } + defer f.Close() + if err := f.Chown(uid, gid); err != nil { + return err + } + + if _, err := f.WriteString(contents); err != nil { + return errors.Wrapf(err, "unable to write %s", path) + } + // Relabel runDirResolv for the container + if err := label.Relabel(path, mountLabel, false); err != nil { + return err + } + + return nil +} diff --git a/test/system/030-run.bats b/test/system/030-run.bats index bc6347012..13fec20ad 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -242,4 +242,46 @@ echo $rand | 0 | $rand run_podman rmi myi } +# #6735 : complex interactions with multiple user namespaces +# The initial report has to do with bind mounts, but that particular +# symptom only manifests on a fedora container image -- we have no +# reproducer on alpine. Checking directory ownership is good enough. +@test "podman run : user namespace preserved root ownership" { + for priv in "" "--privileged"; do + for user in "--user=0" "--user=100"; do + for keepid in "" "--userns=keep-id"; do + opts="$priv $user $keepid" + + for dir in /etc /usr;do + run_podman run --rm $opts $IMAGE stat -c '%u:%g:%n' $dir + remove_same_dev_warning # grumble + is "$output" "0:0:$dir" "run $opts ($dir)" + done + done + done + done +} + +# #6829 : add username to /etc/passwd inside container if --userns=keep-id +@test "podman run : add username to /etc/passwd if --userns=keep-id" { + # Default: always run as root + run_podman run --rm $IMAGE id -un + is "$output" "root" "id -un on regular container" + + # This would always work on root, but is new behavior on rootless: #6829 + # adds a user entry to /etc/passwd + run_podman run --rm --userns=keep-id $IMAGE id -un + is "$output" "$(id -un)" "username on container with keep-id" + + # --privileged should make no difference + run_podman run --rm --privileged --userns=keep-id $IMAGE id -un + remove_same_dev_warning # grumble + is "$output" "$(id -un)" "username on container with keep-id" + + # ...but explicitly setting --user should override keep-id + run_podman run --rm --privileged --userns=keep-id --user=0 $IMAGE id -un + remove_same_dev_warning # grumble + is "$output" "root" "--user=0 overrides keep-id" +} + # vim: filetype=sh diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats index f8c7f2766..945bcfa2d 100644 --- a/test/system/075-exec.bats +++ b/test/system/075-exec.bats @@ -6,8 +6,6 @@ load helpers @test "podman exec - basic test" { - skip_if_remote - rand_filename=$(random_string 20) rand_content=$(random_string 50) @@ -80,4 +78,24 @@ load helpers run_podman rm $cid } +# #6829 : add username to /etc/passwd inside container if --userns=keep-id +# #6593 : doesn't actually work with podman exec +@test "podman exec - with keep-id" { + skip "Please enable once #6593 is fixed" + + run_podman run -d --userns=keep-id $IMAGE sh -c \ + "echo READY;while [ ! -f /stop ]; do sleep 1; done" + cid="$output" + wait_for_ready $cid + + run_podman exec $cid id -un + is "$output" "$(id -un)" "container is running as current user" + + # Until #6593 gets fixed, this just hangs. The server process barfs with: + # unable to find user <username>: no matching entries in passwd file + run_podman exec --user=$(id -un) $cid touch /stop + run_podman wait $cid + run_podman rm $cid +} + # vim: filetype=sh diff --git a/test/system/400-unprivileged-access.bats b/test/system/400-unprivileged-access.bats index 98f8b8211..1384c0ab8 100644 --- a/test/system/400-unprivileged-access.bats +++ b/test/system/400-unprivileged-access.bats @@ -1,6 +1,7 @@ #!/usr/bin/env bats -*- bats -*- # # Tests #2730 - regular users are not able to read/write container storage +# Tests #6957 - /sys/dev (et al) are masked from unprivileged containers # load helpers @@ -97,4 +98,68 @@ EOF run_podman rm c_uidmap c_uidmap_v } + +# #6957 - mask out /proc/acpi, /sys/dev, and other sensitive system files +@test "sensitive mount points are masked without --privileged" { + # FIXME: this should match the list in pkg/specgen/generate/config_linux.go + local -a mps=( + /proc/acpi + /proc/kcore + /proc/keys + /proc/latency_stats + /proc/timer_list + /proc/timer_stats + /proc/sched_debug + /proc/scsi + /sys/firmware + /sys/fs/selinux + /sys/dev + ) + + # Some of the above may not exist on our host. Find only the ones that do. + local -a subset=() + for mp in ${mps[@]}; do + if [ -e $mp ]; then + subset+=($mp) + fi + done + + # Run 'stat' on all the files, plus /dev/null. Get path, file type, + # number of links, major, and minor (see below for why). Do it all + # in one go, to avoid multiple podman-runs + run_podman run --rm $IMAGE stat -c'%n:%F:%h:%T:%t' /dev/null ${subset[@]} + local devnull= + for result in "${lines[@]}"; do + # e.g. /proc/acpi:character special file:1:3:1 + local IFS=: + read path type nlinks major minor <<<"$result" + + if [[ $path = "/dev/null" ]]; then + # /dev/null is our reference point: masked *files* (not directories) + # will be created as /dev/null clones. + # This depends on 'stat' returning results in argv order, + # so /dev/null is first, so we have a reference for others. + # If that ever breaks, this test will have to be done in two passes. + devnull="$major:$minor" + elif [[ $type = "character special file" ]]; then + # Container file is a character device: it must match /dev/null + is "$major:$minor" "$devnull" "$path: major/minor matches /dev/null" + elif [[ $type = "directory" ]]; then + # Directories: must be empty (only two links). + # FIXME: this is a horrible almost-worthless test! It does not + # actually check for files in the directory (expect: zero), + # merely for the nonexistence of any subdirectories! It relies + # on the observed (by Ed) fact that all the masked directories + # contain further subdirectories on the host. If there's ever + # a new masked directory that contains only files, this test + # will silently pass without any indication of error. + # If you can think of a better way to do this check, + # please feel free to fix it. + is "$nlinks" "2" "$path: directory link count" + else + die "$path: Unknown file type '$type'" + fi + done +} + # vim: filetype=sh diff --git a/test/system/410-selinux.bats b/test/system/410-selinux.bats index 1769730f0..497e29b3e 100644 --- a/test/system/410-selinux.bats +++ b/test/system/410-selinux.bats @@ -19,15 +19,8 @@ function check_label() { # FIXME: on some CI systems, 'run --privileged' emits a spurious # warning line about dup devices. Ignore it. + remove_same_dev_warning local context="$output" - if [ ${#lines[@]} -gt 1 ]; then - if expr "${lines[0]}" : "WARNING: .* type, major" >/dev/null; then - echo "# ${lines[0]} [ignored]" >&3 - context="${lines[1]}" - else - die "FAILED: too much output, expected one single line" - fi - fi is "$context" ".*_u:system_r:.*" "SELinux role should always be system_r" diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 5301644d6..4239ef876 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -392,5 +392,37 @@ function find_exec_pid_files() { find $storage_path -type f -iname 'exec_pid_*' fi } + + +############################# +# remove_same_dev_warning # Filter out useless warning from output +############################# +# +# On some CI systems, 'podman run --privileged' emits a useless warning: +# +# WARNING: The same type, major and minor should not be used for multiple devices. +# +# This obviously screws us up when we look at output results. +# +# This function removes the warning from $output and $lines +# +function remove_same_dev_warning() { + # No input arguments. We operate in-place on $output and $lines + + local i=0 + local -a new_lines=() + while [[ $i -lt ${#lines[@]} ]]; do + if expr "${lines[$i]}" : 'WARNING: .* same type, major.* multiple' >/dev/null; then + : + else + new_lines+=("${lines[$i]}") + fi + i=$(( i + 1 )) + done + + lines=("${new_lines[@]}") + output=$(printf '%s\n' "${lines[@]}") +} + # END miscellaneous tools ############################################################################### diff --git a/test/system/helpers.t b/test/system/helpers.t index 7b4e48a84..a022f11c4 100755 --- a/test/system/helpers.t +++ b/test/system/helpers.t @@ -23,7 +23,8 @@ rc=0 function check_result { testnum=$(expr $testnum + 1) if [ "$1" = "$2" ]; then - echo "ok $testnum $3 = $1" + # Multi-level echo flattens newlines, makes success messages readable + echo $(echo "ok $testnum $3 = $1") else echo "not ok $testnum $3" echo "# expected: $2" @@ -141,5 +142,72 @@ done < <(parse_table "$table") # END dprint ############################################################################### +# BEGIN remove_same_dev_warning + +# Test-helper function: runs remove_same_dev_warning, compares resulting +# value of $lines and $output to expected values given on command line +function check_same_dev() { + local testname="$1"; shift + local -a expect_lines=("$@") + local nl=" +" + + remove_same_dev_warning + + # After processing, check the expected number of lines + check_result "${#lines[@]}" "${#@}" "$testname: expected # of lines" + + # ...and each expected line + local expect_output="" + local i=0 + while [ $i -lt ${#expect_lines[@]} ]; do + check_result "${lines[$i]}" "${expect_lines[$i]}" "$testname: line $i" + expect_output+="${expect_lines[$i]}$nl" + i=$(( i + 1 )) + done + + # ...and the possibly-multi-line $output + check_result "$output" "${expect_output%%$nl}" "$testname: output" +} + +# Simplest case: nothing removed. +declare -a lines=("a b c" "d" "e f") +check_same_dev "abc" "a b c" "d" "e f" + +# Confirm that the warning message is removed from the beginning +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "a" + "b" + "c" +) +check_same_dev "warning is removed" a b c + +# ...and from the middle (we do not expect to see this) +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "a" + "b" + "WARNING: The same type, major and minor should not be used for multiple devices." + "c" +) +check_same_dev "multiple warnings removed" a b c + +# Corner case: two lines of output, only one of which we care about +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." + "this is the only line we care about" +) +check_same_dev "one-line output" "this is the only line we care about" + +# Corner case: one line of output, but we expect zero. +declare -a lines=( + "WARNING: The same type, major and minor should not be used for multiple devices." +) +check_same_dev "zero-line output" + + +# END remove_same_dev_warning +############################################################################### exit $rc |