userns: prevent /sys/kernel/* paths in the container #2899

giuseppe · 2019-04-11T09:54:09Z

when we run in a user namespace, there are cases where we have not
enough privileges to mount a fresh sysfs on /sys. To circumvent this
limitation, we rbind /sys from the host. This carries inside of the
container also some mounts we probably don't want to. We are also
limited by the kernel to use rbind instead of bind, as allowing a bind
would uncover paths that were not previously visible.

This is a slimmed down version of the intermediate mount namespace
logic we had before, where we only set /sys to slave, so the umounts
done to the storage by the cleanup process are propagated back to the
host. We also don't setup any new directory, so there is no
additional cleanup to do.

Signed-off-by: Giuseppe Scrivano [email protected]

openshift-ci-robot · 2019-04-11T09:54:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2019-04-11T10:32:21Z

libpod/oci_linux.go

+						return errors.Wrapf(err, "cannot make /sys slave")
+					}
+
+					paths := []string{


Could we make this more future proof. Remove all submounts of /sys? Or at least anything begining with /sys/kernel?

rhatdan · 2019-04-11T10:33:14Z

test/system/030-run.bats

@@ -31,4 +31,12 @@ echo $rand        |   0 | $rand
    done < <(parse_table "$tests")
 }

+@test "podman run - uidmapping has no /sys/kernel mounts" {
+      run_podman $expected_rc run --uidmapping 0:100:10000 $IMAGE mount | grep /sys/kernel


I think you need --net=host here to make the current behaviour happen.

I've added another test with --net=host, so that we can test both cases

rhatdan · 2019-04-11T12:16:27Z

LGTM
@TomSweeneyRedHat PTAL
@vrothberg @mheon @baude PTAL

vrothberg · 2019-04-11T12:21:23Z

@giuseppe rootless tests aren't happy

vrothberg

Code LGTM (very elegant)

giuseppe · 2019-04-11T12:26:06Z

ah, this wouldn't work with rootless. An unprivileged user cannot umount these paths. @rhatdan is it an issue for buildah?

TomSweeneyRedHat · 2019-04-11T13:30:03Z

libpod/oci_linux.go

+					if err = unix.Unshare(unix.CLONE_NEWNS); err != nil {
+						return err
+					}
+					defer unix.Setns(int(fd.Fd()), unix.CLONE_NEWNS)


is this defer needed? Seems to be a duplicate of line 112

good catch! Fixed

I think we don't need the one above.

rhatdan · 2019-04-11T13:39:03Z

@giuseppe Yes.
podman run --net=host fedora mount | grep /sys/kernel
securityfs on /sys/kernel/security type securityfs (rw,nosuid,nodev,noexec,relatime)
configfs on /sys/kernel/config type configfs (rw,relatime)
debugfs on /sys/kernel/debug type debugfs (rw,relatime,seclabel)

when we run in a user namespace, there are cases where we have not enough privileges to mount a fresh sysfs on /sys. To circumvent this limitation, we rbind /sys from the host. This carries inside of the container also some mounts we probably don't want to. We are also limited by the kernel to use rbind instead of bind, as allowing a bind would uncover paths that were not previously visible. This is a slimmed down version of the intermediate mount namespace logic we had before, where we only set /sys to slave, so the umounts done to the storage by the cleanup process are propagated back to the host. We also don't setup any new directory, so there is no additional cleanup to do. Signed-off-by: Giuseppe Scrivano <[email protected]>

rhatdan · 2019-04-11T13:41:19Z

We might need to mask these in rootless mode. I am not sure if @TomSweeneyRedHat has tried buildah bud --isolation chroot inside of rootless podman yet.

We really need this for root running container anyways, especially where if you don't have SELinux running you can potentially modify the kernel.

In rootless mode this would obviously be blocked.

giuseppe · 2019-04-11T13:42:48Z

@giuseppe Yes.
podman run --net=host fedora mount | grep /sys/kernel
securityfs on /sys/kernel/security type securityfs (rw,nosuid,nodev,noexec,relatime)
configfs on /sys/kernel/config type configfs (rw,relatime)
debugfs on /sys/kernel/debug type debugfs (rw,relatime,seclabel)

I think there is no way for an unprivileged user to drop that mounts. It cannot mount a new sysfs and it cannot umount anything that will reveal what is under the mount point. On the other hand, is it really a security issue? The unprivileged user won't have any new privilege that it didn't have on the host.

rhatdan · 2019-04-11T13:45:12Z

Right I am worried about information reveal though. The information in /sys/kernel/debug might be useful to a hacked container. If we can not umount we should mount a tmpfs over them.

giuseppe · 2019-04-11T13:51:32Z

Right I am worried about information reveal though. The information in /sys/kernel/debug might be useful to a hacked container. If we can not umount we should mount a tmpfs over them.

we can mask it, altough /sys/kernel/debug has mode 0700, an unprivileged user cannot access it

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2019-04-11T13:55:54Z

Right I am worried about information reveal though. The information in /sys/kernel/debug might be useful to a hacked container. If we can not umount we should mount a tmpfs over them.

I've added a patch to mask /sys/kernel if we are bind mounting /sys and we are rootless

rhatdan · 2019-04-11T13:58:17Z

Well this looks good.
find /sys/kernel/ | wc
find: '/sys/kernel/security': Permission denied
find: '/sys/kernel/debug': Permission denied
find: '/sys/kernel/config': Permission denied
5615 5615 213373
So I think for rootless we can ignore the mounts.

rhatdan · 2019-04-11T14:00:05Z

LGTM

giuseppe · 2019-04-11T14:01:42Z

Well this looks good.
find /sys/kernel/ | wc
find: '/sys/kernel/security': Permission denied
find: '/sys/kernel/debug': Permission denied
find: '/sys/kernel/config': Permission denied
5615 5615 213373
So I think for rootless we can ignore the mounts.

should I drop the second patch or is it still a valid additional defense?

mheon · 2019-04-11T14:25:47Z

LGTM. I think we keep the masked path - defense in depth is good.

mheon · 2019-04-11T14:26:36Z

Holding /lgtm until tests go green

mheon · 2019-04-11T15:05:39Z

/lgtm

edsantiago · 2019-04-15T17:00:50Z

This commit seems to have broken the new unbrivileged-access BATS test:

# bats test/system/400-unprivileged-access.bats
 ✗ podman container storage is not accessible by unprivileged users
   (in test file test/system/400-unprivileged-access.bats, line 74)
     `find $GRAPH_ROOT $RUN_ROOT \! -type l -perm -o+w -print | while read i; do' failed
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman rm --all --force
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman images --all --format {{.Repository}}:{{.Tag}} {{.ID}}
   quay.io/libpod/alpine_labels:latest   4fab981df737
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman run --name c_uidmap --uidmap 0:10000:10000 quay.io/libpod/alpine_labels:latest true
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman run --name c_uidmap_v --uidmap 0:10000:10000 -v foo:/foo quay.io/libpod/alpine_labels:latest true
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman run --name c_mount quay.io/libpod/alpine_labels:latest sh -c echo hi > /myfile;mkdir -p /mydir/mysubdir; chmod 777 /myfile /mydir /mydir/mysubdir
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman mount c_mount
   /var/lib/containers/storage/overlay/1440f9d7c7b5a8fc4bb1f4d699ef156960f89980663357b2481258ca7430b3d1/merged
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman info --format {{.store.GraphRoot}}
   /var/lib/containers/storage
   $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman info --format {{.store.RunRoot}}
   /var/run/containers/storage
   chmod: changing permissions of '/var/lib/containers/storage/overlay/71071cf29c423da6351954badf4aae660aa3ca1ebcdb8fedce8fda7c659b4599/merged': Operation not permitted
   chmod: changing permissions of '/var/lib/containers/storage/overlay/71071cf29c423da6351954badf4aae660aa3ca1ebcdb8fedce8fda7c659b4599/merged/tmp': Operation not permitted
   #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
   #| FAIL: Able to run 'ls /var/lib/containers/storage/overlay/71071cf29c423da6351954badf4aae660aa3ca1ebcdb8fedce8fda7c659b4599/merged/tmp' without error
   #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@giuseppe any suggestions on how to fix it

giuseppe · 2019-04-15T20:26:52Z

This commit seems to have broken the new unbrivileged-access BATS test:

I think it is related to the new tests that are using a mapping not correct for the unprivileged access tests

giuseppe · 2019-04-15T21:13:34Z

I am surprised how that could happen though, directories under /var/lib/containers/storage/overlay have mode drwx------ that blocks completely access to other users

edsantiago · 2019-04-15T23:03:44Z

Not any more. Immediately after this command, /var/lib/containers/storage becomes 711:

# ./bin/podman run --name c_uidmap   --uidmap 0:10000:10000 alpine_labels true

Furthermore, I am now stuck in a system where I can no longer remove that directory:

# rm -rf /var/lib/containers/storage
rm: cannot remove '/var/lib/containers/storage/overlay/0edc5e399bf04596f445dfaf7073d15699e2327dc45e1f62dca43638fc4095b8/merged': Device or resource busy

(podman rm -a, podman ps -a, podman images -a, all show nothing).

master @ c1e2b58. I don't have time this evening to do a full investigateion but wanted to leave you with something you can check out tomorrow morning.

giuseppe · 2019-04-16T07:42:38Z

how do directories under /var/lib/containers/storage/overlay look like?

$ sudo ls -l /var/lib/containers/storage/overlay
total 16
drwx------. 5 root root  4096 Apr 15 23:39 3f72eff9ab5617186824e3a431489b5bb70fadc3004734b3c66008da3f3f0724
drwx------. 6 root root  4096 Apr 15 23:26 a464c54f93a9e88fc1d33df1e0e39cca427d60145a360962e8f19a1dbf900da9
drwx------. 7  100 users 4096 Apr 15 23:11 df64d3292fd6194b7865d7326af5255db6d81e9df29f48adde61a918fbd8c332
drwx------. 2 root root  4096 Apr 16 09:39 l

They must have mode drwx------.

edsantiago · 2019-04-16T15:53:32Z

Sorry for the late reply. I can no longer reproduce this on my system, and OSP10 is down so I can't get a new virt for testing. The only difference on my system is that I'm still stuck with the undeletable directory mentioned above, so instead of rm -rf /var/lib/containers/storage I've done mv ...{,.DELETE-ME}. There is otherwise no difference: no reboot, no dnf-upgrade. I've tried everything I can to reproduce.

openshift-ci-robot added the size/M label Apr 11, 2019

openshift-ci-robot requested review from rhatdan and TomSweeneyRedHat April 11, 2019 09:54

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2019

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 3d0b90a to 507c640 Compare April 11, 2019 10:02

rhatdan reviewed Apr 11, 2019

View reviewed changes

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 507c640 to 0a5df19 Compare April 11, 2019 11:09

vrothberg reviewed Apr 11, 2019

View reviewed changes

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 0a5df19 to 2ea8946 Compare April 11, 2019 12:25

TomSweeneyRedHat reviewed Apr 11, 2019

View reviewed changes

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 2ea8946 to b780088 Compare April 11, 2019 13:40

oci: add /sys/kernel to the masked paths

42eb9ea

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 1b60e4b to 1367c65 Compare April 11, 2019 13:55

spec: mask /sys/kernel when bind mounting /sys

2c9c40d

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the prevent-sys-fs-kernel-paths-in-userns branch from 1367c65 to 2c9c40d Compare April 11, 2019 13:55

openshift-ci-robot assigned mheon Apr 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2019

openshift-merge-robot merged commit b281c34 into containers:master Apr 11, 2019

edsantiago mentioned this pull request Apr 15, 2019

run BATS tests in Cirrus #2947

Merged

mheon added the Release Notes 1.3.0 label Apr 17, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

userns: prevent /sys/kernel/* paths in the container #2899

userns: prevent /sys/kernel/* paths in the container #2899

giuseppe commented Apr 11, 2019

openshift-ci-robot commented Apr 11, 2019

rhatdan Apr 11, 2019

giuseppe Apr 11, 2019

rhatdan Apr 11, 2019

giuseppe Apr 11, 2019

rhatdan commented Apr 11, 2019

vrothberg commented Apr 11, 2019

vrothberg left a comment

giuseppe commented Apr 11, 2019

TomSweeneyRedHat Apr 11, 2019

giuseppe Apr 11, 2019

rhatdan Apr 11, 2019

rhatdan commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

giuseppe commented Apr 11, 2019

rhatdan commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

mheon commented Apr 11, 2019

mheon commented Apr 11, 2019

mheon commented Apr 11, 2019

edsantiago commented Apr 15, 2019

giuseppe commented Apr 15, 2019

giuseppe commented Apr 15, 2019

edsantiago commented Apr 15, 2019

giuseppe commented Apr 16, 2019

edsantiago commented Apr 16, 2019

userns: prevent /sys/kernel/* paths in the container #2899

userns: prevent /sys/kernel/* paths in the container #2899

Conversation

giuseppe commented Apr 11, 2019

openshift-ci-robot commented Apr 11, 2019

rhatdan Apr 11, 2019

Choose a reason for hiding this comment

giuseppe Apr 11, 2019

Choose a reason for hiding this comment

rhatdan Apr 11, 2019

Choose a reason for hiding this comment

giuseppe Apr 11, 2019

Choose a reason for hiding this comment

rhatdan commented Apr 11, 2019

vrothberg commented Apr 11, 2019

vrothberg left a comment

Choose a reason for hiding this comment

giuseppe commented Apr 11, 2019

TomSweeneyRedHat Apr 11, 2019

Choose a reason for hiding this comment

giuseppe Apr 11, 2019

Choose a reason for hiding this comment

rhatdan Apr 11, 2019

Choose a reason for hiding this comment

rhatdan commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

giuseppe commented Apr 11, 2019

rhatdan commented Apr 11, 2019

rhatdan commented Apr 11, 2019

giuseppe commented Apr 11, 2019

mheon commented Apr 11, 2019

mheon commented Apr 11, 2019

mheon commented Apr 11, 2019

edsantiago commented Apr 15, 2019

giuseppe commented Apr 15, 2019

giuseppe commented Apr 15, 2019

edsantiago commented Apr 15, 2019

giuseppe commented Apr 16, 2019

edsantiago commented Apr 16, 2019