From 02264d597faa034c14393ab6c98591e85a2642ee Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 22 Aug 2019 11:21:20 -0400 Subject: 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 --- pkg/spec/spec.go | 43 ++--------------- pkg/spec/storage.go | 135 +++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 102 insertions(+), 76 deletions(-) (limited to 'pkg/spec') diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 156d6849d..779fb5290 100644 --- a/pkg/spec/spec.go +++ b/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) diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index b634f4cac..bc06945f0 100644 --- a/pkg/spec/storage.go +++ b/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"} - for dest := range readonlyTmpfs { + readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"} + 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] { - opts, err := util.ProcessTmpfsOptions(mount.Options) + if mount.Type == TypeTmpfs { + 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") || - 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 +396,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 +431,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 +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 - 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 +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 } -- cgit v1.2.3-54-g00ecf From d45595d9cc2ae60a5e23d081e27a8ba0ab952113 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 22 Aug 2019 11:30:50 -0400 Subject: Don't double-process tmpfs options 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 --- pkg/spec/storage.go | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'pkg/spec') diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index bc06945f0..9f4248924 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -195,16 +195,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 { - opts, err := util.ProcessOptions(mount.Options, true) - if err != nil { - return nil, nil, err - } - mount.Options = opts - } if mount.Type == TypeBind { absSrc, err := filepath.Abs(mount.Source) if err != nil { -- cgit v1.2.3-54-g00ecf From 5bdd97f77fc3100c6338928d4d54af32273d36fb Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 23 Aug 2019 13:24:06 -0400 Subject: Set base mount options for bind mounts from base system 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 --- libpod/options.go | 2 +- pkg/spec/spec.go | 21 -------------------- pkg/spec/storage.go | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-- pkg/util/mountOpts.go | 17 ++++++++++++---- 4 files changed, 67 insertions(+), 28 deletions(-) (limited to 'pkg/spec') diff --git a/libpod/options.go b/libpod/options.go index f4bf536b3..6df1ca5be 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1360,7 +1360,7 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { } destinations[vol.Dest] = true - mountOpts, err := util.ProcessOptions(vol.Options, false) + mountOpts, err := util.ProcessOptions(vol.Options, false, nil) if err != nil { return errors.Wrapf(err, "error processing options for named volume %q mounted at %q", vol.Name, vol.Dest) } diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 779fb5290..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" @@ -457,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 9f4248924..7ed21a055 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" @@ -816,17 +817,46 @@ 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, 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 { - opts, err := util.ProcessOptions(m.Options, false) + 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" { - opts, err := util.ProcessOptions(m.Options, true) + opts, err := util.ProcessOptions(m.Options, true, nil) if err != nil { return nil, err } @@ -837,3 +867,24 @@ func initFSMounts(inputMounts []spec.Mount) ([]spec.Mount, error) { } 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 81dc4cefd..8accd4697 100644 --- a/pkg/util/mountOpts.go +++ b/pkg/util/mountOpts.go @@ -13,10 +13,19 @@ var ( ErrDupeMntOption = errors.Errorf("duplicate option passed") ) +// DefaultMountOptions sets default mount options for ProcessOptions. +type DefaultMountOptions struct { + Noexec bool + Nosuid bool + Nodev bool +} + // 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. -func ProcessOptions(options []string, isTmpfs bool) ([]string, error) { +// 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, foundExec, foundSuid, foundDev, foundCopyUp, foundBind bool ) @@ -93,13 +102,13 @@ func ProcessOptions(options []string, isTmpfs bool) ([]string, error) { if !foundProp { options = append(options, "rprivate") } - if !foundExec { + if !foundExec && (defaults == nil || defaults.Noexec) { options = append(options, "noexec") } - if !foundSuid { + if !foundSuid && (defaults == nil || defaults.Nosuid) { options = append(options, "nosuid") } - if !foundDev { + if !foundDev && (defaults == nil || defaults.Nodev) { options = append(options, "nodev") } if isTmpfs && !foundCopyUp { -- cgit v1.2.3-54-g00ecf From 96812dc490dbd00b0ec6280353a4e78ba79b44b8 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 23 Aug 2019 19:05:16 -0400 Subject: Fix addition of mount options when using RO tmpfs 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 --- pkg/spec/storage.go | 6 ++++-- pkg/util/mountOpts.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'pkg/spec') diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 7ed21a055..bc0eaad6d 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -163,14 +163,16 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, // If requested, add tmpfs filesystems for read-only containers. if config.ReadOnlyRootfs && config.ReadOnlyTmpfs { readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"} - options := []string{"rw", "rprivate", "exec", "nosuid", "nodev", "tmpcopyup"} + options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"} for _, dest := range readonlyTmpfs { if _, ok := baseMounts[dest]; ok { continue } localOpts := options if dest == "/run" { - localOpts = append(localOpts, "dev", "suid", "noexec", "size=65536k") + localOpts = append(localOpts, "noexec", "size=65536k") + } else { + localOpts = append(localOpts, "exec") } baseMounts[dest] = spec.Mount{ Destination: dest, diff --git a/pkg/util/mountOpts.go b/pkg/util/mountOpts.go index 9e387ce95..670daeaf9 100644 --- a/pkg/util/mountOpts.go +++ b/pkg/util/mountOpts.go @@ -10,7 +10,7 @@ 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") ) // DefaultMountOptions sets default mount options for ProcessOptions. -- cgit v1.2.3-54-g00ecf