Merge pull request #3876 from mheon/fix_mount_flags

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

3 files changed, 228 insertions, 151 deletions

diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go
index 156d6849d..44bbda885 100644
--- a/pkg/spec/spec.go
+++ b/pkg/spec/spec.go
@@ -2,13 +2,11 @@ package createconfig
 
 import (
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/pkg/cgroups"
 	"github.com/containers/libpod/pkg/rootless"
-	pmount "github.com/containers/storage/pkg/mount"
 	"github.com/docker/docker/oci/caps"
 	"github.com/docker/go-units"
 	"github.com/opencontainers/runc/libcontainer/user"
@@ -368,7 +366,11 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 	// BIND MOUNTS
 	configSpec.Mounts = supercedeUserMounts(userMounts, configSpec.Mounts)
 	// Process mounts to ensure correct options
-	configSpec.Mounts = initFSMounts(configSpec.Mounts)
+	finalMounts, err := initFSMounts(configSpec.Mounts)
+	if err != nil {
+		return nil, err
+	}
+	configSpec.Mounts = finalMounts
 
 	// BLOCK IO
 	blkio, err := config.CreateBlockIO()
@@ -394,43 +396,6 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 		}
 	}
 
-	// Make sure that the bind mounts keep options like nosuid, noexec, nodev.
-	mounts, err := pmount.GetMounts()
-	if err != nil {
-		return nil, err
-	}
-	for i := range configSpec.Mounts {
-		m := &configSpec.Mounts[i]
-		isBind := false
-		for _, o := range m.Options {
-			if o == "bind" || o == "rbind" {
-				isBind = true
-				break
-			}
-		}
-		if !isBind {
-			continue
-		}
-		mount, err := findMount(m.Source, mounts)
-		if err != nil {
-			return nil, err
-		}
-		if mount == nil {
-			continue
-		}
-	next_option:
-		for _, o := range strings.Split(mount.Opts, ",") {
-			if o == "nosuid" || o == "noexec" || o == "nodev" {
-				for _, e := range m.Options {
-					if e == o {
-						continue next_option
-					}
-				}
-				m.Options = append(m.Options, o)
-			}
-		}
-	}
-
 	// Add annotations
 	if configSpec.Annotations == nil {
 		configSpec.Annotations = make(map[string]string)
@@ -490,25 +455,6 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 	return configSpec, nil
 }
 
-func findMount(target string, mounts []*pmount.Info) (*pmount.Info, error) {
-	var err error
-	target, err = filepath.Abs(target)
-	if err != nil {
-		return nil, errors.Wrapf(err, "cannot resolve %s", target)
-	}
-	var bestSoFar *pmount.Info
-	for _, i := range mounts {
-		if bestSoFar != nil && len(bestSoFar.Mountpoint) > len(i.Mountpoint) {
-			// Won't be better than what we have already found
-			continue
-		}
-		if strings.HasPrefix(target, i.Mountpoint) {
-			bestSoFar = i
-		}
-	}
-	return bestSoFar, nil
-}
-
 func blockAccessToKernelFilesystems(config *CreateConfig, g *generate.Generator) {
 	if !config.Privileged {
 		for _, mp := range []string{
diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go
index b634f4cac..bc0eaad6d 100644
--- a/pkg/spec/storage.go
+++ b/pkg/spec/storage.go
@@ -10,6 +10,7 @@ import (
 	"github.com/containers/buildah/pkg/parse"
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/pkg/util"
+	pmount "github.com/containers/storage/pkg/mount"
 	"github.com/containers/storage/pkg/stringid"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
@@ -160,22 +161,18 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 	}
 
 	// If requested, add tmpfs filesystems for read-only containers.
-	// Need to keep track of which we created, so we don't modify options
-	// for them later...
-	readonlyTmpfs := map[string]bool{
-		"/tmp":     false,
-		"/var/tmp": false,
-		"/run":     false,
-	}
 	if config.ReadOnlyRootfs && config.ReadOnlyTmpfs {
+		readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
 		options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
-		for dest := range readonlyTmpfs {
+		for _, dest := range readonlyTmpfs {
 			if _, ok := baseMounts[dest]; ok {
 				continue
 			}
 			localOpts := options
 			if dest == "/run" {
 				localOpts = append(localOpts, "noexec", "size=65536k")
+			} else {
+				localOpts = append(localOpts, "exec")
 			}
 			baseMounts[dest] = spec.Mount{
 				Destination: dest,
@@ -183,7 +180,6 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 				Source:      "tmpfs",
 				Options:     localOpts,
 			}
-			readonlyTmpfs[dest] = true
 		}
 	}
 
@@ -202,16 +198,6 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 	// Final step: maps to arrays
 	finalMounts := make([]spec.Mount, 0, len(baseMounts))
 	for _, mount := range baseMounts {
-		// All user-added tmpfs mounts need their options processed.
-		// Exception: mounts added by the ReadOnlyTmpfs option, which
-		// contain several exceptions to normal options rules.
-		if mount.Type == TypeTmpfs && !readonlyTmpfs[mount.Destination] {
-			opts, err := util.ProcessTmpfsOptions(mount.Options)
-			if err != nil {
-				return nil, nil, err
-			}
-			mount.Options = opts
-		}
 		if mount.Type == TypeBind {
 			absSrc, err := filepath.Abs(mount.Source)
 			if err != nil {
@@ -226,9 +212,6 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 		finalVolumes = append(finalVolumes, volume)
 	}
 
-	logrus.Debugf("Got mounts: %v", finalMounts)
-	logrus.Debugf("Got volumes: %v", finalVolumes)
-
 	return finalMounts, finalVolumes, nil
 }
 
@@ -250,14 +233,17 @@ func (config *CreateConfig) getVolumesFrom(runtime *libpod.Runtime) (map[string]
 			splitVol = strings.SplitN(vol, ":", 2)
 		)
 		if len(splitVol) == 2 {
-			if strings.Contains(splitVol[1], "Z") ||
-				strings.Contains(splitVol[1], "private") ||
-				strings.Contains(splitVol[1], "slave") ||
-				strings.Contains(splitVol[1], "shared") {
-				return nil, nil, errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z", splitVol[1])
+			splitOpts := strings.Split(splitVol[1], ",")
+			for _, checkOpt := range splitOpts {
+				switch checkOpt {
+				case "z", "ro", "rw":
+					// Do nothing, these are valid options
+				default:
+					return nil, nil, errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z'", splitVol[1])
+				}
 			}
 
-			if options, err = parse.ValidateVolumeOpts(strings.Split(splitVol[1], ",")); err != nil {
+			if options, err = parse.ValidateVolumeOpts(splitOpts); err != nil {
 				return nil, nil, err
 			}
 		}
@@ -403,9 +389,7 @@ func getBindMount(args []string) (spec.Mount, error) {
 		Type: TypeBind,
 	}
 
-	setSource := false
-	setDest := false
-	setRORW := false
+	var setSource, setDest, setRORW, setSuid, setDev, setExec bool
 
 	for _, val := range args {
 		kv := strings.Split(val, "=")
@@ -440,9 +424,23 @@ func getBindMount(args []string) (spec.Mount, error) {
 			} else {
 				return newMount, errors.Wrapf(optionArgError, "badly formatted option %q", val)
 			}
-		case "nosuid", "nodev", "noexec":
-			// TODO: detect duplication of these options.
-			// (Is this necessary?)
+		case "nosuid", "suid":
+			if setSuid {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'nosuid' and 'suid' options more than once")
+			}
+			setSuid = true
+			newMount.Options = append(newMount.Options, kv[0])
+		case "nodev", "dev":
+			if setDev {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'nodev' and 'dev' options more than once")
+			}
+			setDev = true
+			newMount.Options = append(newMount.Options, kv[0])
+		case "noexec", "exec":
+			if setExec {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'noexec' and 'exec' options more than once")
+			}
+			setExec = true
 			newMount.Options = append(newMount.Options, kv[0])
 		case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z":
 			newMount.Options = append(newMount.Options, kv[0])
@@ -497,12 +495,34 @@ func getTmpfsMount(args []string) (spec.Mount, error) {
 		Source: TypeTmpfs,
 	}
 
-	setDest := false
+	var setDest, setRORW, setSuid, setDev, setExec bool
 
 	for _, val := range args {
 		kv := strings.Split(val, "=")
 		switch kv[0] {
-		case "ro", "nosuid", "nodev", "noexec":
+		case "ro", "rw":
+			if setRORW {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'ro' and 'rw' options more than once")
+			}
+			setRORW = true
+			newMount.Options = append(newMount.Options, kv[0])
+		case "nosuid", "suid":
+			if setSuid {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'nosuid' and 'suid' options more than once")
+			}
+			setSuid = true
+			newMount.Options = append(newMount.Options, kv[0])
+		case "nodev", "dev":
+			if setDev {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'nodev' and 'dev' options more than once")
+			}
+			setDev = true
+			newMount.Options = append(newMount.Options, kv[0])
+		case "noexec", "exec":
+			if setExec {
+				return newMount, errors.Wrapf(optionArgError, "cannot pass 'noexec' and 'exec' options more than once")
+			}
+			setExec = true
 			newMount.Options = append(newMount.Options, kv[0])
 		case "tmpfs-mode":
 			if len(kv) == 1 {
@@ -543,14 +563,34 @@ func getTmpfsMount(args []string) (spec.Mount, error) {
 func getNamedVolume(args []string) (*libpod.ContainerNamedVolume, error) {
 	newVolume := new(libpod.ContainerNamedVolume)
 
-	setSource := false
-	setDest := false
+	var setSource, setDest, setRORW, setSuid, setDev, setExec bool
 
 	for _, val := range args {
 		kv := strings.Split(val, "=")
 		switch kv[0] {
-		case "ro", "nosuid", "nodev", "noexec":
-			// TODO: detect duplication of these options
+		case "ro", "rw":
+			if setRORW {
+				return nil, errors.Wrapf(optionArgError, "cannot pass 'ro' and 'rw' options more than once")
+			}
+			setRORW = true
+			newVolume.Options = append(newVolume.Options, kv[0])
+		case "nosuid", "suid":
+			if setSuid {
+				return nil, errors.Wrapf(optionArgError, "cannot pass 'nosuid' and 'suid' options more than once")
+			}
+			setSuid = true
+			newVolume.Options = append(newVolume.Options, kv[0])
+		case "nodev", "dev":
+			if setDev {
+				return nil, errors.Wrapf(optionArgError, "cannot pass 'nodev' and 'dev' options more than once")
+			}
+			setDev = true
+			newVolume.Options = append(newVolume.Options, kv[0])
+		case "noexec", "exec":
+			if setExec {
+				return nil, errors.Wrapf(optionArgError, "cannot pass 'noexec' and 'exec' options more than once")
+			}
+			setExec = true
 			newVolume.Options = append(newVolume.Options, kv[0])
 		case "volume-label":
 			return nil, errors.Errorf("the --volume-label option is not presently implemented")
@@ -692,6 +732,9 @@ func (config *CreateConfig) getTmpfsMounts() (map[string]spec.Mount, error) {
 		var options []string
 		spliti := strings.Split(i, ":")
 		destPath := spliti[0]
+		if err := parse.ValidateVolumeCtrDir(spliti[0]); err != nil {
+			return nil, err
+		}
 		if len(spliti) > 1 {
 			options = strings.Split(spliti[1], ",")
 		}
@@ -775,16 +818,75 @@ func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.M
 }
 
 // Ensure mount options on all mounts are correct
-func initFSMounts(inputMounts []spec.Mount) []spec.Mount {
+func initFSMounts(inputMounts []spec.Mount) ([]spec.Mount, error) {
+	// We need to look up mounts so we can figure out the proper mount flags
+	// to apply.
+	systemMounts, err := pmount.GetMounts()
+	if err != nil {
+		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
 	var mounts []spec.Mount
 	for _, m := range inputMounts {
 		if m.Type == TypeBind {
-			m.Options = util.ProcessOptions(m.Options)
+			baseMnt, err := findMount(m.Destination, systemMounts)
+			if err != nil {
+				return nil, errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Destination)
+			}
+			var noexec, nosuid, nodev bool
+			for _, baseOpt := range strings.Split(baseMnt.Opts, ",") {
+				switch baseOpt {
+				case "noexec":
+					noexec = true
+				case "nosuid":
+					nosuid = true
+				case "nodev":
+					nodev = true
+				}
+			}
+
+			defaultMountOpts := new(util.DefaultMountOptions)
+			defaultMountOpts.Noexec = noexec
+			defaultMountOpts.Nosuid = nosuid
+			defaultMountOpts.Nodev = nodev
+
+			opts, err := util.ProcessOptions(m.Options, false, defaultMountOpts)
+			if err != nil {
+				return nil, err
+			}
+			m.Options = opts
 		}
 		if m.Type == TypeTmpfs && filepath.Clean(m.Destination) != "/dev" {
-			m.Options = append(m.Options, "tmpcopyup")
+			opts, err := util.ProcessOptions(m.Options, true, nil)
+			if err != nil {
+				return nil, err
+			}
+			m.Options = opts
 		}
+
 		mounts = append(mounts, m)
 	}
-	return mounts
+	return mounts, nil
+}
+
+// TODO: We could make this a bit faster by building a tree of the mountpoints
+// and traversing it to identify the correct mount.
+func findMount(target string, mounts []*pmount.Info) (*pmount.Info, error) {
+	var err error
+	target, err = filepath.Abs(target)
+	if err != nil {
+		return nil, errors.Wrapf(err, "cannot resolve %s", target)
+	}
+	var bestSoFar *pmount.Info
+	for _, i := range mounts {
+		if bestSoFar != nil && len(bestSoFar.Mountpoint) > len(i.Mountpoint) {
+			// Won't be better than what we have already found
+			continue
+		}
+		if strings.HasPrefix(target, i.Mountpoint) {
+			bestSoFar = i
+		}
+	}
+	return bestSoFar, nil
 }
diff --git a/pkg/util/mountOpts.go b/pkg/util/mountOpts.go
index 9b2c734c0..670daeaf9 100644
--- a/pkg/util/mountOpts.go
+++ b/pkg/util/mountOpts.go
@@ -10,91 +10,120 @@ var (
 	// ErrBadMntOption indicates that an invalid mount option was passed.
 	ErrBadMntOption = errors.Errorf("invalid mount option")
 	// ErrDupeMntOption indicates that a duplicate mount option was passed.
-	ErrDupeMntOption = errors.Errorf("duplicate option passed")
+	ErrDupeMntOption = errors.Errorf("duplicate mount option passed")
 )
 
-// ProcessOptions parses the options for a bind mount and ensures that they are
-// sensible and follow convention.
-func ProcessOptions(options []string) []string {
-	var (
-		foundbind, foundrw, foundro bool
-		rootProp                    string
-	)
-
-	for _, opt := range options {
-		switch opt {
-		case "bind", "rbind":
-			foundbind = true
-		case "ro":
-			foundro = true
-		case "rw":
-			foundrw = true
-		case "private", "rprivate", "slave", "rslave", "shared", "rshared":
-			rootProp = opt
-		}
-	}
-	if !foundbind {
-		options = append(options, "rbind")
-	}
-	if !foundrw && !foundro {
-		options = append(options, "rw")
-	}
-	if rootProp == "" {
-		options = append(options, "rprivate")
-	}
-	return options
+// DefaultMountOptions sets default mount options for ProcessOptions.
+type DefaultMountOptions struct {
+	Noexec bool
+	Nosuid bool
+	Nodev  bool
 }
 
-// ProcessTmpfsOptions parses the options for a tmpfs mountpoint and ensures
-// that they are sensible and follow convention.
-func ProcessTmpfsOptions(options []string) ([]string, error) {
+// ProcessOptions parses the options for a bind or tmpfs mount and ensures that
+// they are sensible and follow convention. The isTmpfs variable controls
+// whether extra, tmpfs-specific options will be allowed.
+// The defaults variable controls default mount options that will be set. If it
+// is not included, they will be set unconditionally.
+func ProcessOptions(options []string, isTmpfs bool, defaults *DefaultMountOptions) ([]string, error) {
 	var (
-		foundWrite, foundSize, foundProp, foundMode bool
+		foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ bool
 	)
 
-	baseOpts := []string{"noexec", "nosuid", "nodev"}
 	for _, opt := range options {
 		// Some options have parameters - size, mode
 		splitOpt := strings.SplitN(opt, "=", 2)
 		switch splitOpt[0] {
+		case "exec", "noexec":
+			if foundExec {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'noexec' and 'exec' can be used")
+			}
+			foundExec = true
+		case "suid", "nosuid":
+			if foundSuid {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'nosuid' and 'suid' can be used")
+			}
+			foundSuid = true
+		case "nodev", "dev":
+			if foundDev {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'nodev' and 'dev' can be used")
+			}
+			foundDev = true
 		case "rw", "ro":
 			if foundWrite {
-				return nil, errors.Wrapf(ErrDupeMntOption, "only one of rw and ro can be used")
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'rw' and 'ro' can be used")
 			}
 			foundWrite = true
-			baseOpts = append(baseOpts, opt)
 		case "private", "rprivate", "slave", "rslave", "shared", "rshared":
 			if foundProp {
 				return nil, errors.Wrapf(ErrDupeMntOption, "only one root propagation mode can be used")
 			}
 			foundProp = true
-			baseOpts = append(baseOpts, opt)
 		case "size":
+			if !isTmpfs {
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'size' option is only allowed with tmpfs mounts")
+			}
 			if foundSize {
 				return nil, errors.Wrapf(ErrDupeMntOption, "only one tmpfs size can be specified")
 			}
 			foundSize = true
-			baseOpts = append(baseOpts, opt)
 		case "mode":
+			if !isTmpfs {
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'mode' option is only allowed with tmpfs mounts")
+			}
 			if foundMode {
 				return nil, errors.Wrapf(ErrDupeMntOption, "only one tmpfs mode can be specified")
 			}
 			foundMode = true
-			baseOpts = append(baseOpts, opt)
-		case "noexec", "nodev", "nosuid":
-			// Do nothing. We always include these even if they are
-			// not explicitly requested.
+		case "tmpcopyup":
+			if !isTmpfs {
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'tmpcopyup' option is only allowed with tmpfs mounts")
+			}
+			if foundCopyUp {
+				return nil, errors.Wrapf(ErrDupeMntOption, "the 'tmpcopyup' option can only be set once")
+			}
+			foundCopyUp = true
+		case "bind", "rbind":
+			if isTmpfs {
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'bind' and 'rbind' options are not allowed with tmpfs mounts")
+			}
+			if foundBind {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'rbind' and 'bind' can be used")
+			}
+			foundBind = true
+		case "z", "Z":
+			if isTmpfs {
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'z' and 'Z' options are not allowed with tmpfs mounts")
+			}
+			if foundZ {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'z' and 'Z' can be used")
+			}
 		default:
-			return nil, errors.Wrapf(ErrBadMntOption, "unknown tmpfs option %q", opt)
+			return nil, errors.Wrapf(ErrBadMntOption, "unknown mount option %q", opt)
 		}
 	}
 
 	if !foundWrite {
-		baseOpts = append(baseOpts, "rw")
+		options = append(options, "rw")
 	}
 	if !foundProp {
-		baseOpts = append(baseOpts, "rprivate")
+		options = append(options, "rprivate")
+	}
+	if !foundExec && (defaults == nil || defaults.Noexec) {
+		options = append(options, "noexec")
+	}
+	if !foundSuid && (defaults == nil || defaults.Nosuid) {
+		options = append(options, "nosuid")
+	}
+	if !foundDev && (defaults == nil || defaults.Nodev) {
+		options = append(options, "nodev")
+	}
+	if isTmpfs && !foundCopyUp {
+		options = append(options, "tmpcopyup")
+	}
+	if !isTmpfs && !foundBind {
+		options = append(options, "rbind")
 	}
 
-	return baseOpts, nil
+	return options, nil
 }