userns: do not use an intermediate mount namespace #2730

giuseppe · 2019-03-21T11:32:30Z

We have an issue in the current implementation where the cleanup
process is not able to umount the storage as it is running in a
separate namespace.

Simplify the implementation for user namespaces by not using an
intermediate mount namespace. For doing it, we need to relax the
permissions on the parent directories and allow browsing
them. Containers that are running without a user namespace, will still
maintain mode 0700 on their directory.

Alternative for: #2728

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

rhatdan · 2019-03-21T12:49:30Z

But this means a non priv user can get to the images correct? This has the potential to allow information leak and opening up of more 777 directories for users to write into.

giuseppe · 2019-03-21T19:16:38Z

But this means a non priv user can get to the images correct? This has the potential to allow information leak and opening up of more 777 directories for users to write into.

The directories under /var/lib/containers/storage/overlay still have mode 0700.

Let's get #2716 first, then I will rebase on top of that and verify there is nothing accessible by an unprivileged user

rh-atomic-bot · 2019-03-22T09:05:02Z

☔ The latest upstream changes (presumably #2716) made this pull request unmergeable. Please resolve the merge conflicts.

giuseppe · 2019-03-22T13:18:39Z

I've verified there is nothing accessible to unprivileged users. All the directories that are owned by the final root in the container have mode 0700. Said so, we need to be more careful when we add more content in the storage directory.

In any case, even if I like the way this could simplify the implementation and need to worry of different code paths, I don't want to be the guy who caused CVEs in the storage :-) here there is a third option, that depends on a patch in conmon: #2730

mheon · 2019-03-22T13:47:13Z

If we can work through the security issues, I am definitely in favor of this one - the simplification of one of the most complex parts of libpod is a wonderful side effect of the fix.

giuseppe · 2019-03-22T13:49:54Z

If we can work through the security issues, I am definitely in favor of this one - the simplification of one of the most complex parts of libpod is a wonderful side effect of the fix.

agreed. Having an intermediate mount namespace will always be somehow fragile, and difficult to debug

rhatdan · 2019-03-22T14:15:40Z

Is there a way we could at least add a test to prove that a world writable directory in containers/storage was not reachable via non root users.

su NONROOT find $STORAGEPATH -perm -o+w

giuseppe · 2019-03-22T14:37:36Z

Is there a way we could at least add a test to prove that a world writable directory in containers/storage was not reachable via non root users.

good idea. We could add a test like:

find /var/run/libpod/ /var/lib/containers/storage -perm -o+w | while read i; do if chroot --userspec 1000:1000 / bash -c 'chmod +w $(dirname $i); chmod +w $i; echo hello > $i/test' 2> /dev/null ; then echo "HACKED"; fi; done

where would be the best place to add it? More stuff we have in the storage, the better it would be

rhatdan · 2019-03-22T16:22:32Z

I think adding this to libpod at least in the system tests. @edsantiago WDYT?

edsantiago · 2019-03-22T18:12:38Z

@rhatdan good idea; on my todo list for Monday, thanks.

giuseppe · 2019-03-22T20:10:50Z

I've added a patch to test an unprivileged user cannot access files in the storage. We might need to add the check to more tests. @edsantiago could you review it?

rhatdan · 2019-03-25T11:10:10Z

@nalind PTAL

edsantiago

I love that you're extending the BATS tests, thank you! Some gotchas inline; and also a broader question: why add this as a function in helpers.bash? That would be appropriate if this were a general-purpose function useful for multiple subtests. This is a single independent one-time test, so I would recommend making it a new standalone .bats test instead. Look at 000-TEMPLATE, copy it to NNN-pick-a-name.bats, and put the code inside a @test context.

edsantiago · 2019-03-25T11:49:05Z

test/system/helpers.bash

+# Check that an unprivileged user cannot read or write to the storage
+function check_no_unprivileged_access() {
+    GRAPH_ROOT=$(run_podman info --format json | jq .store.GraphRoot)
+    RUN_ROOT=$(run_podman info --format json | jq .store.RunRoot)


Unfortunately, in the BATS world, run_podman | xxx doesn't work. It has to be done as two steps, run_podman then operating on $output. And in these cases, I would probably write it as:

run_podman info --format '{{.storage.Graphroot}}' ! then the same for RunRoot GRAPH_ROOT="$output"

thanks! I've fixed it in the new version

edsantiago · 2019-03-25T11:55:31Z

test/system/helpers.bash

+function check_no_unprivileged_access() {
+    GRAPH_ROOT=$(run_podman info --format json | jq .store.GraphRoot)
+    RUN_ROOT=$(run_podman info --format json | jq .store.RunRoot)
+    find $GRAPH_ROOT $RUN_ROOT -perm -o+w | while read i; do


Did you mean to include -type f? Or ! -type l? There are an awful lot of symlinks that don't really seem to be useful to test.

no I meant that. I'd prefer to test as many files/directories as possible, if it turns to be too expensive we can tune it

What I mean is: can you try running that find command manually, with -ls instead of -print? I think you'll find that there are a lot of symlinks, because all symlinks are mode 777, but that's meaningless. There is no point whatsoever to testing those.

a lot of them in my current storage are coming from busybox, we could probably try to skip these as we are really testing the same thing multiple times. I'd still like though to test symlinks and that we don't have access to rm them

Let me try another way: can you explain what it is you think you are testing by paying attention to symlinks? I think there's a major misunderstanding going on here. Let's say the find produces output like this:

/x -> /y

What is the purpose of running any tests on /x? It is the same as running on /y, and:

if /y is a writable file or directory, the find command will emit it as part of its output

if /y is not a writable file or directory, there's no point in testing it

in the last version I've pushed, I am also checking for rm.

In the example you've given /x -> /y the unprivileged user could still be able to rm /x but not having privileges for doing it on /y

Permissions on /y are irrelevant for rm; only the parent directory permissions matter.

I'm working on a little bit of a rewrite; I'll push it when I'm comfortable with it. It might be in a few hours.

and symlink ownership. But given anyway that we are hardcoding the user, it probably doesn't make sense to test symlinks this way and we can safely skip them

edsantiago · 2019-03-25T12:00:36Z

test/system/helpers.bash

+    GRAPH_ROOT=$(run_podman info --format json | jq .store.GraphRoot)
+    RUN_ROOT=$(run_podman info --format json | jq .store.RunRoot)
+    find $GRAPH_ROOT $RUN_ROOT -perm -o+w | while read i; do
+        if chroot --userspec 1000:1000 / bash -c 'chmod +w $(dirname $i); chmod +w $i; cat $i || echo hello > $i || echo hello > $i/test' 2> /dev/null; then


Careful with plain $i! Any space in the filename or any parent directory will cause all of these to fail. The convention is to double-quote every reference: "$i". (It's even better to use find ... -print0 | xargs -0, but that would be harder to read in this case.

thanks, fixed now

edsantiago · 2019-03-25T13:29:50Z

test/system/030-run.bats

+@test "podman run - basic tests --uidmapping"  {
+    run_podman --uidmapping 0:10000:10000 run $IMAGE true
+
+    run_podman --uidmapping 0:10000:10000 -v foo:/foo run $IMAGE true


Neither of these work. Hint: you can test via PODMAN=/path/to/podman bats test/system/030-run.bats

There are other problems; I'm working on some modifications to the framework, will submit them as a PR on your PR.

edsantiago · 2019-03-25T15:59:56Z

giuseppe#1

giuseppe · 2019-03-25T16:29:01Z

libpod/oci_linux.go

-		var fd *os.File
-		fd, err = os.Open(fmt.Sprintf("/proc/%d/task/%d/ns/mnt", os.Getpid(), unix.Gettid()))
+// makeAccessible changes the path permission and each parent directory to have --x--x--x
+func makeAccessible(path string, uid, gid int) error {


@vrothberg @rhatdan @nalind this logic should probably be moved to containers/storage as it is not really specific to libpod

Do we want to do this now, or merge this PR first and move later?

rhatdan · 2019-03-26T10:46:33Z

Are there other paths under /var/lib/containers/storage that we don't want a non privileged user to read?

Volumes directory?
DRIVER-containers?

openshift-ci-robot · 2019-03-26T14:12:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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

mheon · 2019-03-26T14:30:38Z

@giuseppe Are we good to merge here?

giuseppe · 2019-03-26T16:04:02Z

@rhatdan I've verified that volumes and anything overlay is not accessible to an unprivileged user.

I am fine with the PR as it is. Said so, I'd prefer if @nalind could review these changes.

mheon · 2019-03-26T18:01:00Z

I'd like to get this in before 1.2

rhatdan · 2019-03-27T09:14:40Z

LGTM

mheon · 2019-03-28T14:08:06Z

I'll ping @nalind in scrum to ask for a quick review

rh-atomic-bot · 2019-03-28T14:21:37Z

☔ The latest upstream changes (presumably #2789) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2019-03-29T12:48:54Z

☔ The latest upstream changes (presumably #2575) made this pull request unmergeable. Please resolve the merge conflicts.

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

We have an issue in the current implementation where the cleanup process is not able to umount the storage as it is running in a separate namespace. Simplify the implementation for user namespaces by not using an intermediate mount namespace. For doing it, we need to relax the permissions on the parent directories and allow browsing them. Containers that are running without a user namespace, will still maintain mode 0700 on their directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

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

mheon · 2019-03-29T14:24:28Z

/lgtm

openshift-ci-robot requested review from rhatdan and vrothberg March 21, 2019 11:32

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Mar 21, 2019

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from d892dba to 5e7caf7 Compare March 22, 2019 11:29

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from fbc4a26 to 1bc2d9e Compare March 22, 2019 17:11

giuseppe mentioned this pull request Mar 24, 2019

conmon: add --exit-command-namespace cri-o/cri-o#2160

Closed

edsantiago suggested changes Mar 25, 2019

View reviewed changes

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from 1bc2d9e to 99f0495 Compare March 25, 2019 12:11

edsantiago reviewed Mar 25, 2019

View reviewed changes

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from 99f0495 to 85e3f99 Compare March 25, 2019 13:31

mheon added the Release Notes 1.2.0 label Mar 25, 2019

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from 85e3f99 to 596ffa0 Compare March 25, 2019 16:25

giuseppe commented Mar 25, 2019

View reviewed changes

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from a684267 to 60f1bd5 Compare March 25, 2019 16:36

edsantiago approved these changes Mar 26, 2019

View reviewed changes

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from 60f1bd5 to f575047 Compare March 26, 2019 18:03

mheon mentioned this pull request Mar 28, 2019

userns: join the parent namespace for the cleanup #2743

Closed

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from f575047 to 462e2b3 Compare March 29, 2019 09:00

giuseppe and others added 3 commits March 29, 2019 14:04

volumes: push the chown logic to runtime_volume_linux.go

f7e72bc

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

test: test that an unprivileged user cannot access the storage

1ae8a5b

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

giuseppe force-pushed the userns-take-rid-of-intermediate-mountns branch from 462e2b3 to 1ae8a5b Compare March 29, 2019 13:04

openshift-ci-robot assigned mheon Mar 29, 2019

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

openshift-merge-robot merged commit 83cea5d into containers:master Mar 29, 2019

This was referenced Mar 29, 2019

Rework named volumes in DB #2774

Merged

Add support to migrate containers #2272

Merged

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

userns: do not use an intermediate mount namespace #2730

userns: do not use an intermediate mount namespace #2730

Conversation

giuseppe commented Mar 21, 2019

rhatdan commented Mar 21, 2019

giuseppe commented Mar 21, 2019

rh-atomic-bot commented Mar 22, 2019

giuseppe commented Mar 22, 2019

mheon commented Mar 22, 2019

giuseppe commented Mar 22, 2019

rhatdan commented Mar 22, 2019

giuseppe commented Mar 22, 2019

rhatdan commented Mar 22, 2019

edsantiago commented Mar 22, 2019

giuseppe commented Mar 22, 2019

rhatdan commented Mar 25, 2019

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Mar 26, 2019

openshift-ci-robot commented Mar 26, 2019

mheon commented Mar 26, 2019

giuseppe commented Mar 26, 2019

mheon commented Mar 26, 2019

rhatdan commented Mar 27, 2019

mheon commented Mar 28, 2019

rh-atomic-bot commented Mar 28, 2019

rh-atomic-bot commented Mar 29, 2019

mheon commented Mar 29, 2019