overlord/snapstate: do a minimal sanity check on containers

This introduces validateContainer, that uses snap.Container's
Walk to do a minimal sanity check of the container in question.

With this, a snap that has an obvious problem due to e.g. a missing or
non-executable app, or an unreadable snap.yaml, will fail to
install. The error message will alert the user to contact the snap
developers, and the system's log will contain additional details meant
for the developers themselves.

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,165 @@ func validateInfoAndFlags(info *snap.Info, snapst *SnapState, flags Flags) error
 	return nil
 }
 
+func normPath(path string) string {
+	if path == "" {
+		return ""
+	}
+
+	path = strings.TrimPrefix(filepath.Clean(path), "/")
+	if strings.HasPrefix(path, "../") {
+		// not something inside the snap
+		return ""
+	}
+	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 developr")
+)
+
+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
+	for k := range needsr {
+		needsf[k] = 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 +351,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()