Allow suid, exec, dev mount options to cancel nosuid/noexec/nodev #3876

mheon · 2019-08-22T15:27:58Z

Previously, we explicitly set noexec/nosuid/nodev on every mount, with no ability to disable them. The 'mount' command on Linux will accept their inverses without complaint, though - 'noexec' is counteracted by 'exec', 'nosuid' by 'suid', etc. Add support for passing these options at the command line to disable our explicit forcing of security options.

This also cleans up mount option handling significantly. We are still parsing options in more than one place, which isn't good, but option parsing for bind and tmpfs mounts has been unified.

Fixes: #3819
Fixes: #3803

openshift-ci-robot · 2019-08-22T15:28:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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~~ [mheon]

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

pkg/spec/spec.go

mheon · 2019-08-22T15:32:21Z

I'll add a few tests after lunch.

mheon · 2019-08-22T15:36:11Z

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/containerd/continuity v0.0.0-20190426062206-aaeac12a7ffc // indirect
 	github.com/containernetworking/cni v0.7.1
 	github.com/containernetworking/plugins v0.8.1
-	github.com/containers/buildah v1.10.1
+	github.com/containers/buildah v1.8.4-0.20190821140209-376e52ee0142


@vrothberg So I grabbed the top of buildah/master, and it appears to have downgraded, version-wise. We might want to look into that?

The important part is behind the last dash (i.e., the commit 376e52ee0142). It's a bug in go (no joke) which is trying to match a commit to a tag.

Commit 376e52ee0142 looks good.

If we want to get rid of that, we can do podman run --privileged -v pwd:/src:Z -it --rm golang:1.13-rc-buster and go get github.com/containers/buildah@376e52ee0142 inside the container. Go 1.13 will set the correct version/tag.

The annoying thing about that bug is that go mod's min-version algorithm can break here and downgrade a whole bunch of other dependencies.

pkg/spec/spec.go

rhatdan · 2019-08-22T17:02:23Z

pkg/spec/storage.go

 			if _, ok := baseMounts[dest]; ok {
 				continue
 			}
 			localOpts := options
 			if dest == "/run" {
-				localOpts = append(localOpts, "noexec", "size=65536k")
+				localOpts = append(localOpts, "dev", "suid", "noexec", "size=65536k")


Why are you doing this?

Need to make sure that the parser doesn't see the mount is missing nodev/nosuid and add those

rhatdan · 2019-08-22T17:06:40Z

pkg/spec/storage.go

 		}
 		if m.Type == TypeTmpfs && filepath.Clean(m.Destination) != "/dev" {
-			m.Options = append(m.Options, "tmpcopyup")


Do you still have tmpcopyup by default for tmpfs?

Still defaulted

rhatdan · 2019-08-22T17:09:42Z

You need to make sure that the mounts look the same with old podman as with this.
Make sure
podman run fedora mount

matches
./bin/podman run fedora mount

Then add a few other mounts using -v, --tmpfs and --mount and make sure everything is the same.

mheon · 2019-08-22T17:45:01Z

Test failures seem to be a mixed bag. It looks like my bind-mount options parsing code simplification over-simplified and broke things, but some of the rest seem like underlying bugs that are now being detected and reported.

mheon · 2019-08-23T17:27:59Z

Re-added functionality to infer mount options from those used on the base system.

mheon · 2019-08-23T23:08:49Z

I think tests will go green now. Still need to add a few more to cover new functionality.

mheon · 2019-08-26T13:56:33Z

Added tests. Good to go once this goes green.

mheon · 2019-08-27T14:47:54Z

One flake, otherwise green
@rhatdan @baude @vrothberg @TomSweeneyRedHat PTAL

vrothberg · 2019-08-28T09:49:17Z

I did a bigger chunk of review this morning. Maybe I'll find time this afternoon, latest tomorrow morning (EU).

rh-atomic-bot · 2019-08-28T18:25:30Z

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

Vendor some changes to parsing code that we need for Podman. Signed-off-by: Matthew Heon <[email protected]>

Previously, we explicitly set noexec/nosuid/nodev on every mount, with no ability to disable them. The 'mount' command on Linux will accept their inverses without complaint, though - 'noexec' is counteracted by 'exec', 'nosuid' by 'suid', etc. Add support for passing these options at the command line to disable our explicit forcing of security options. This also cleans up mount option handling significantly. We are still parsing options in more than one place, which isn't good, but option parsing for bind and tmpfs mounts has been unified. Fixes: containers#3819 Fixes: containers#3803 Signed-off-by: Matthew Heon <[email protected]>

We already process the options on all tmpfs filesystems during final addition of mounts to the spec. We don't need to do it before that in parseVolumes. Signed-off-by: Matthew Heon <[email protected]>

If I mount, say, /usr/bin into my container - I expect to be able to run the executables in that mount. Unconditionally applying noexec would be a bad idea. Before my patches to change mount options and allow exec/dev/suid being set explicitly, we inferred the mount options from where on the base system the mount originated, and the options it had there. Implement the same functionality for the new option handling. There's a lot of performance left on the table here, but I don't know that this is ever going to take enough time to make it worth optimizing. Signed-off-by: Matthew Heon <[email protected]>

Signed-off-by: Matthew Heon <[email protected]>

For read-only containers set to create tmpfs filesystems over /run and other common destinations, we were incorrectly setting mount options, resulting in duplicate mount options. Signed-off-by: Matthew Heon <[email protected]>

Signed-off-by: Matthew Heon <[email protected]>

vrothberg

Just nits. Nice work, @mheon!

vrothberg · 2019-08-29T08:28:03Z

test/e2e/run_volume_test.go

+		Expect(matches[0]).To(Not(ContainSubstring("noexec")))
+		Expect(matches[0]).To(Not(ContainSubstring("nodev")))
+		Expect(matches[0]).To(Not(ContainSubstring("nosuid")))
+	})


Can we add an additional block box test?

A trivial one could be an executable test.sh:

#!/bin/sh echo "Hello World!"

And then mount & execute it once with exec and grep for Hello World! and then mount & execute it with noexec and grep for Permission denied.

Done - I just mounted in /bin from the host, but it should be a sufficient test.

vrothberg · 2019-08-29T08:37:46Z

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/containerd/continuity v0.0.0-20190426062206-aaeac12a7ffc // indirect
 	github.com/containernetworking/cni v0.7.1
 	github.com/containernetworking/plugins v0.8.1
-	github.com/containers/buildah v1.10.1
+	github.com/containers/buildah v1.8.4-0.20190821140209-376e52ee0142


If we want to get rid of that, we can do podman run --privileged -v pwd:/src:Z -it --rm golang:1.13-rc-buster and go get github.com/containers/buildah@376e52ee0142 inside the container. Go 1.13 will set the correct version/tag.

The annoying thing about that bug is that go mod's min-version algorithm can break here and downgrade a whole bunch of other dependencies.

vrothberg · 2019-08-29T08:52:18Z

pkg/spec/storage.go

-			// (Is this necessary?)
+		case "nosuid", "suid":
+			if setSuid {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'nosuid' and 'suid' options more than once")


Clever trick to group them 👍

vrothberg · 2019-08-29T09:02:09Z

pkg/spec/storage.go

+		return nil, errors.Wrapf(err, "error retrieving system mounts to look up mount options")
+	}
+
+	// TODO: We probably don't need to re-build the mounts array


Nit: I think so too. Operating on inputMounts[i] should work.

I'll leave it off for now, this is large enough already

TomSweeneyRedHat · 2019-08-29T12:18:53Z

LGTM, nothing beyond @vrothbergs comments.

Signed-off-by: Matthew Heon <[email protected]>

mheon · 2019-09-04T20:12:21Z

Should be ready for merge.
@baude @rhatdan @vrothberg @giuseppe PTAL

rhatdan · 2019-09-04T20:22:23Z

/lgtm

openshift-ci-robot requested review from baude and rhatdan August 22, 2019 15:28

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 22, 2019

mheon commented Aug 22, 2019

View reviewed changes

pkg/spec/spec.go Show resolved Hide resolved

mheon commented Aug 22, 2019

View reviewed changes

mheon force-pushed the fix_mount_flags branch 2 times, most recently from 5150078 to e5f0781 Compare August 22, 2019 15:59

rhatdan reviewed Aug 22, 2019

View reviewed changes

mheon force-pushed the fix_mount_flags branch from e5f0781 to aecca85 Compare August 23, 2019 17:27

mheon force-pushed the fix_mount_flags branch from 66a8a7d to 5d261bc Compare August 26, 2019 13:57

mheon added the Release Notes 1.5.2 label Aug 26, 2019

mheon force-pushed the fix_mount_flags branch 2 times, most recently from 822a80e to 8d56081 Compare August 26, 2019 20:10

mheon added 6 commits August 28, 2019 14:28

Update buildah to current master

502536f

Vendor some changes to parsing code that we need for Podman. Signed-off-by: Matthew Heon <[email protected]>

Don't double-process tmpfs options

d45595d

We already process the options on all tmpfs filesystems during final addition of mounts to the spec. We don't need to do it before that in parseVolumes. Signed-off-by: Matthew Heon <[email protected]>

Allow :z and :Z with ProcessOptions

820e242

Signed-off-by: Matthew Heon <[email protected]>

Fix addition of mount options when using RO tmpfs

96812dc

For read-only containers set to create tmpfs filesystems over /run and other common destinations, we were incorrectly setting mount options, resulting in duplicate mount options. Signed-off-by: Matthew Heon <[email protected]>

Add a test for the new suid/exec/dev options

c13a52c

Signed-off-by: Matthew Heon <[email protected]>

mheon force-pushed the fix_mount_flags branch from 8d56081 to c13a52c Compare August 28, 2019 18:29

vrothberg reviewed Aug 29, 2019

View reviewed changes

Add test to verify noexec works with volume mounts

5f15501

Signed-off-by: Matthew Heon <[email protected]>

openshift-ci-robot assigned rhatdan Sep 4, 2019

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

openshift-merge-robot merged commit ab44484 into containers:master Sep 4, 2019

This was referenced Sep 4, 2019

podman network create #3862

Merged

Support running containers without CGroups #3581

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow suid, exec, dev mount options to cancel nosuid/noexec/nodev #3876

Allow suid, exec, dev mount options to cancel nosuid/noexec/nodev #3876

mheon commented Aug 22, 2019

openshift-ci-robot commented Aug 22, 2019

mheon commented Aug 22, 2019

mheon Aug 22, 2019

vrothberg Aug 22, 2019

vrothberg Aug 29, 2019

rhatdan Aug 22, 2019

mheon Aug 22, 2019

rhatdan Aug 22, 2019

mheon Aug 22, 2019

rhatdan commented Aug 22, 2019

mheon commented Aug 22, 2019

mheon commented Aug 23, 2019

mheon commented Aug 23, 2019

mheon commented Aug 26, 2019

mheon commented Aug 27, 2019

vrothberg commented Aug 28, 2019 •

edited

Loading

rh-atomic-bot commented Aug 28, 2019

vrothberg left a comment

vrothberg Aug 29, 2019

mheon Sep 4, 2019

vrothberg Aug 29, 2019

vrothberg Aug 29, 2019

vrothberg Aug 29, 2019

mheon Sep 4, 2019

TomSweeneyRedHat commented Aug 29, 2019

mheon commented Sep 4, 2019

rhatdan commented Sep 4, 2019

Allow suid, exec, dev mount options to cancel nosuid/noexec/nodev #3876

Allow suid, exec, dev mount options to cancel nosuid/noexec/nodev #3876

Conversation

mheon commented Aug 22, 2019

openshift-ci-robot commented Aug 22, 2019

mheon commented Aug 22, 2019

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

rhatdan commented Aug 22, 2019

mheon commented Aug 22, 2019

mheon commented Aug 23, 2019

mheon commented Aug 23, 2019

mheon commented Aug 26, 2019

mheon commented Aug 27, 2019

vrothberg commented Aug 28, 2019 • edited Loading

rh-atomic-bot commented Aug 28, 2019

vrothberg 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

TomSweeneyRedHat commented Aug 29, 2019

mheon commented Sep 4, 2019

rhatdan commented Sep 4, 2019

vrothberg commented Aug 28, 2019 •

edited

Loading