Merge pull request #4464 from chipaca/snap-permission-sanity-check

overlord/snapstate: do a minimal sanity check on containers

cmd/snap-exec/main.go

@@ -175,9 +175,9 @@ func execApp(snapApp, revision, command string, args []string) error {
 	}
 	env = append(env, osutil.SubstituteEnv(app.Env())...)
 
-	// strings.Split() is ok here because we validate all app fields
-	// and the whitelist is pretty strict (see
-	// snap/validate.go:appContentWhitelist)
+	// strings.Split() is ok here because we validate all app fields and the
+	// whitelist is pretty strict (see snap/validate.go:appContentWhitelist)
+	// (see also overlord/snapstate/check_snap.go's normPath)
 	tmpArgv := strings.Split(cmdAndArgs, " ")
 	cmd := tmpArgv[0]
 	cmdArgs := expandEnvCmdArgs(tmpArgv[1:], osutil.EnvMap(env))

overlord/managers_test.go

@@ -211,7 +211,15 @@ func (ms *mgrsSuite) TearDownTest(c *C) {
 var settleTimeout = 15 * time.Second
 
 func makeTestSnap(c *C, snapYamlContent string) string {
-	return snaptest.MakeTestSnapWithFiles(c, snapYamlContent, nil)
+	info, err := snap.InfoFromSnapYaml([]byte(snapYamlContent))
+	c.Assert(err, IsNil)
+
+	var files [][]string
+	for _, app := range info.Apps {
+		files = append(files, []string{app.Command, ""})
+	}
+
+	return snaptest.MakeTestSnapWithFiles(c, snapYamlContent, files)
 }
 
 func (ms *mgrsSuite) TestHappyLocalInstall(c *C) {

overlord/snapstate/backend_test.go

@@ -23,6 +23,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"path/filepath"
 	"sort"
 	"strings"
 
@@ -315,6 +316,7 @@ type fakeSnappyBackend struct {
 
 	linkSnapFailTrigger     string
 	copySnapDataFailTrigger string
+	emptyContainer          snap.Container
 }
 
 func (f *fakeSnappyBackend) OpenSnapFile(snapFilePath string, si *snap.SideInfo) (*snap.Info, snap.Container, error) {
@@ -327,8 +329,13 @@ func (f *fakeSnappyBackend) OpenSnapFile(snapFilePath string, si *snap.SideInfo)
 		op.sinfo = *si
 	}
 
+	name := filepath.Base(snapFilePath)
+	if idx := strings.IndexByte(name, '_'); idx > -1 {
+		name = name[:idx]
+	}
+
 	f.ops = append(f.ops, op)
-	return &snap.Info{Architectures: []string{"all"}}, nil, nil
+	return &snap.Info{SuggestedName: name, Architectures: []string{"all"}}, f.emptyContainer, nil
 }
 
 func (f *fakeSnappyBackend) SetupSnap(snapFilePath string, si *snap.SideInfo, p progress.Meter) error {

overlord/snapstate/check_snap.go

@@ -20,13 +20,17 @@
 package snapstate
 
 import (
+	"errors"
 	"fmt"
+	"os"
+	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
 
 	"github.com/snapcore/snapd/arch"
 	"github.com/snapcore/snapd/cmd"
+	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/overlord/snapstate/backend"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/release"
@@ -180,13 +184,191 @@ func validateInfoAndFlags(info *snap.Info, snapst *SnapState, flags Flags) error
 	return nil
 }
 
+// normPath is a helper for validateContainer. It takes a relative path (e.g. an
+// app's RestartCommand, which might be empty to mean there is no such thing),
+// and cleans it.
+//
+// * empty paths are returned as is
+// * if the path is not relative, it's initial / is dropped
+// * if the path goes "outside" (ie starts with ../), the empty string is
+//   returned (i.e. "ignore")
+// * if there's a space in the command, ignore the rest of the string
+//   (see also cmd/snap-exec/main.go's comment about strings.Split)
+func normPath(path string) string {
+	if path == "" {
+		return ""
+	}
+
+	path = strings.TrimPrefix(filepath.Clean(path), "/")
+	if strings.HasPrefix(path, "../") {
+		// not something inside the snap
+		return ""
+	}
+	if idx := strings.IndexByte(path, ' '); idx > -1 {
+		return path[:idx]
+	}
+
+	return path
+}
+
+var (
+	ErrBadModes     = errors.New("snap is unusable due to bad permissions; contact develper")
+	ErrMissingPaths = errors.New("snap is unusable due to missing files; contact developer")
+)
+
+func validateContainer(s *snap.Info, c snap.Container) error {
+	// needsrx keeps track of things that need to have at least 0555 perms
+	needsrx := map[string]bool{
+		".":    true,
+		"meta": true,
+	}
+	// needsx keeps track of things that need to have at least 0111 perms
+	needsx := map[string]bool{}
+	// needsr keeps track of things that need to have at least 0444 perms
+	needsr := map[string]bool{
+		"meta/snap.yaml": true,
+	}
+	// needsf keeps track of things that need to be regular files (or symlinks to regular files)
+	needsf := map[string]bool{}
+	// noskipd tracks directories we want to descend into despite not being in needs*
+	noskipd := map[string]bool{}
+
+	for _, app := range s.Apps {
+		// for non-services, paths go into the needsrx bag because users
+		// need rx perms to execute it
+		bag := needsrx
+		paths := []string{app.Command}
+		if app.IsService() {
+			// services' paths just need to not be skipped by the validator
+			bag = noskipd
+			// additional paths to check for services:
+			// XXX maybe have a method on app to keep this in sync
+			paths = append(paths, app.StopCommand, app.ReloadCommand, app.PostStopCommand)
+		}
+
+		for _, path := range paths {
+			path = normPath(path)
+			if path == "" {
+				continue
+			}
+
+			needsf[path] = true
+			if app.IsService() {
+				needsx[path] = true
+			}
+			for ; path != "."; path = filepath.Dir(path) {
+				bag[path] = true
+			}
+		}
+
+		// completer is special :-/
+		if path := normPath(app.Completer); path != "" {
+			needsr[path] = true
+			for path = filepath.Dir(path); path != "."; path = filepath.Dir(path) {
+				needsrx[path] = true
+			}
+		}
+	}
+	// note all needsr so far need to be regular files (or symlinks)
+	for k := range needsr {
+		needsf[k] = true
+	}
+	// thing can get jumbled up
+	for path := range needsrx {
+		delete(needsx, path)
+		delete(needsr, path)
+	}
+	for path := range needsx {
+		if needsr[path] {
+			delete(needsx, path)
+			delete(needsr, path)
+			needsrx[path] = true
+		}
+	}
+	seen := make(map[string]bool, len(needsx)+len(needsrx)+len(needsr))
+
+	// bad modes are logged instead of being returned because the end user
+	// can do nothing with the info (and the developer can read the logs)
+	hasBadModes := false
+	err := c.Walk(".", func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+
+		mode := info.Mode()
+		if needsrx[path] || needsx[path] || needsr[path] {
+			seen[path] = true
+		}
+		if !needsrx[path] && !needsx[path] && !needsr[path] && !strings.HasPrefix(path, "meta/") {
+			if mode.IsDir() {
+				if noskipd[path] {
+					return nil
+				}
+				return filepath.SkipDir
+			}
+			return nil
+		}
+
+		if needsrx[path] || mode.IsDir() {
+			if mode.Perm()&0555 != 0555 {
+				logger.Noticef("in snap %q: %q should be world-readable and executable, and isn't: %s", s.Name(), path, mode)
+				hasBadModes = true
+			}
+		} else {
+			if needsf[path] {
+				// this assumes that if it's a symlink it's OK. Arguably we
+				// should instead follow the symlink.  We'd have to expose
+				// Lstat(), and guard against loops, and ...  huge can of
+				// worms, and as this validator is meant as a developer aid
+				// more than anything else, not worth it IMHO (as I can't
+				// imagine this happening by accident).
+				if mode&(os.ModeDir|os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 {
+					logger.Noticef("in snap %q: %q should be a regular file (or a symlink) and isn't", s.Name(), path)
+					hasBadModes = true
+				}
+			}
+			if needsx[path] || strings.HasPrefix(path, "meta/hooks/") {
+				if mode.Perm()&0111 == 0 {
+					logger.Noticef("in snap %q: %q should be executable, and isn't: %s", s.Name(), path, mode)
+					hasBadModes = true
+				}
+			} else {
+				// in needsr, or under meta but not a hook
+				if mode.Perm()&0444 != 0444 {
+					logger.Noticef("in snap %q: %q should be world-readable, and isn't: %s", s.Name(), path, mode)
+					hasBadModes = true
+				}
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+	if len(seen) != len(needsx)+len(needsrx)+len(needsr) {
+		for _, needs := range []map[string]bool{needsx, needsrx, needsr} {
+			for path := range needs {
+				if !seen[path] {
+					logger.Noticef("in snap %q: path %q does not exist", s.Name(), path)
+				}
+			}
+		}
+		return ErrMissingPaths
+	}
+
+	if hasBadModes {
+		return ErrBadModes
+	}
+	return nil
+}
+
 var openSnapFile = backend.OpenSnapFile
 
 // checkSnap ensures that the snap can be installed.
 func checkSnap(st *state.State, snapFilePath string, si *snap.SideInfo, curInfo *snap.Info, flags Flags) error {
 	// This assumes that the snap was already verified or --dangerous was used.
 
-	s, _, err := openSnapFile(snapFilePath, si)
+	s, c, err := openSnapFile(snapFilePath, si)
 	if err != nil {
 		return err
 	}
@@ -195,6 +377,10 @@ func checkSnap(st *state.State, snapFilePath string, si *snap.SideInfo, curInfo
 		return err
 	}
 
+	if err := validateContainer(s, c); err != nil {
+		return err
+	}
+
 	st.Lock()
 	defer st.Unlock()