Add support for 'exec', 'suid', 'dev' mount flags

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

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

libpod/options.go

@@ -1360,10 +1360,15 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption {
 			}
 			destinations[vol.Dest] = true
 
+			mountOpts, err := util.ProcessOptions(vol.Options, false)
+			if err != nil {
+				return errors.Wrapf(err, "error processing options for named volume %q mounted at %q", vol.Name, vol.Dest)
+			}
+
 			ctr.config.NamedVolumes = append(ctr.config.NamedVolumes, &ContainerNamedVolume{
 				Name:    vol.Name,
 				Dest:    vol.Dest,
-				Options: util.ProcessOptions(vol.Options),
+				Options: mountOpts,
 			})
 		}
 

pkg/spec/spec.go

@@ -368,7 +368,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 +398,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)

pkg/spec/storage.go

@@ -160,22 +160,16 @@ 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 {
-		options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
+		readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
-		for dest := range readonlyTmpfs {
+		options := []string{"rw", "rprivate", "exec", "nosuid", "nodev", "tmpcopyup"}
+		for _, dest := range readonlyTmpfs {
 			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")
 			}
 			baseMounts[dest] = spec.Mount{
 				Destination: dest,
@@ -183,7 +177,6 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 				Source:      "tmpfs",
 				Options:     localOpts,
 			}
-			readonlyTmpfs[dest] = true
 		}
 	}
 
@@ -205,8 +198,8 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount,
 		// 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] {
+		if mount.Type == TypeTmpfs {
-			opts, err := util.ProcessTmpfsOptions(mount.Options)
+			opts, err := util.ProcessOptions(mount.Options, true)
 			if err != nil {
 				return nil, nil, err
 			}
@@ -226,9 +219,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 +240,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") ||
+			splitOpts := strings.Split(splitVol[1], ",")
-				strings.Contains(splitVol[1], "private") ||
+			for _, checkOpt := range splitOpts {
-				strings.Contains(splitVol[1], "slave") ||
+				switch checkOpt {
-				strings.Contains(splitVol[1], "shared") {
+				case "z", "ro", "rw":
-				return nil, nil, errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z", splitVol[1])
+					// 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 +396,7 @@ func getBindMount(args []string) (spec.Mount, error) {
 		Type: TypeBind,
 	}
 
-	setSource := false
+	var setSource, setDest, setRORW, setSuid, setDev, setExec bool
-	setDest := false
-	setRORW := false
 
 	for _, val := range args {
 		kv := strings.Split(val, "=")
@@ -440,9 +431,23 @@ func getBindMount(args []string) (spec.Mount, error) {
 			} else {
 				return newMount, errors.Wrapf(optionArgError, "badly formatted option %q", val)
 			}
-		case "nosuid", "nodev", "noexec":
+		case "nosuid", "suid":
-			// TODO: detect duplication of these options.
+			if setSuid {
-			// (Is this necessary?)
+				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 +502,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 +570,34 @@ func getTmpfsMount(args []string) (spec.Mount, error) {
 func getNamedVolume(args []string) (*libpod.ContainerNamedVolume, error) {
 	newVolume := new(libpod.ContainerNamedVolume)
 
-	setSource := false
+	var setSource, setDest, setRORW, setSuid, setDev, setExec bool
-	setDest := false
 
 	for _, val := range args {
 		kv := strings.Split(val, "=")
 		switch kv[0] {
-		case "ro", "nosuid", "nodev", "noexec":
+		case "ro", "rw":
-			// TODO: detect duplication of these options
+			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 +739,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 +825,25 @@ 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) {
 	var mounts []spec.Mount
 	for _, m := range inputMounts {
 		if m.Type == TypeBind {
-			m.Options = util.ProcessOptions(m.Options)
+			opts, err := util.ProcessOptions(m.Options, false)
+			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)
+			if err != nil {
+				return nil, err
+			}
+			m.Options = opts
 		}
+
 		mounts = append(mounts, m)
 	}
-	return mounts
+	return mounts, nil
 }

pkg/util/mountOpts.go

@@ -13,88 +13,101 @@ var (
 	ErrDupeMntOption = errors.Errorf("duplicate option passed")
 )
 
-// ProcessOptions parses the options for a bind mount and ensures that they are
+// ProcessOptions parses the options for a bind or tmpfs mount and ensures that
-// sensible and follow convention.
+// they are sensible and follow convention. The isTmpfs variable controls
-func ProcessOptions(options []string) []string {
+// whether extra, tmpfs-specific options will be allowed.
+func ProcessOptions(options []string, isTmpfs bool) ([]string, error) {
 	var (
-		foundbind, foundrw, foundro bool
+		foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind 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
-}
-
-// ProcessTmpfsOptions parses the options for a tmpfs mountpoint and ensures
-// that they are sensible and follow convention.
-func ProcessTmpfsOptions(options []string) ([]string, error) {
-	var (
-		foundWrite, foundSize, foundProp, foundMode 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 "tmpcopyup":
-		case "noexec", "nodev", "nosuid":
+			if !isTmpfs {
-			// Do nothing. We always include these even if they are
+				return nil, errors.Wrapf(ErrBadMntOption, "the 'tmpcopyup' option is only allowed with tmpfs mounts")
-			// not explicitly requested.
+			}
+			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
 		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 {
+		options = append(options, "noexec")
+	}
+	if !foundSuid {
+		options = append(options, "nosuid")
+	}
+	if !foundDev {
+		options = append(options, "nodev")
+	}
+	if isTmpfs && !foundCopyUp {
+		options = append(options, "tmpcopyup")
+	}
+	if !isTmpfs && !foundBind {
+		options = append(options, "rbind")
 	}
 
-	return baseOpts, nil
+	return options, nil
 }