snap/container: gaurd against non-regular files and external symlinks…

… under /meta

Non-regular meta files like desktop files, icons can cause harmful
behavior without proper validation like:
- Completely freezing snapd (opening named pipes without writers)
- Giving root read-access to host files (external symlinks)

This commit only validates file under /meta.

* snap/container_test: add test suite for squashfs container implementation

	Container tests were only testing with SnapDir containers, this commit
	adds a squashfs test suite running the same tests to increase tests
	robustness and cover more edge cases.

	SnapDir: covers "snap pack" scenarios
	Squashfs: covers "snap install" scenarios

* snap/container: fix isExternalSymlink implementation

	First bug was counting any .. encountered leading to counting files
	like a..b mistakenly (thanks @pedronis for catching this).

	Second bug was an off-by-one error were it was checking if the go-back-cnt
	is bigger than the path-depth and not bigger-than-or-equal.

* snap/container_test: add unit tests for isExternalSymlink

* snap/{snapdir,squashfs}: add unit tests for Readlink

* snap/{snapdir,squashfs}: rename Readlink to ReadLink for consistency

* snap/container_test: add a test case to TestIsExternalSymlink

* snap/container: expose Lstat interface in snap.Container

* snap/container: follow symlinks inside container and mimic review-tools checks

* container: make meta symlink validation simpler and stricter

* container: refactor {validate,eval}Symlink

* snap/squashfs: fix/optimize relative Walks

* snap/squashfs: refactor/optimize squashfs container Lstat

* snap/container: validate meta symlinks target's mode

* snap/container: add illustrative flowchart for container symlink evaluation logic

* snap/container: always check symlink target mode (thanks @alexmurray)

	The previous implementation was only checking non-zero modes which
	could allow a file to mask its mode by symlinking to zero mode file.

Signed-off-by: Zeyad Gouda <[email protected]>

snap/container.go

@@ -21,6 +21,7 @@ package snap
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -44,6 +45,12 @@ type Container interface {
 	// ReadFile returns the content of a single file from the snap.
 	ReadFile(relative string) ([]byte, error)
 
+	// ReadLink returns the destination of the named symbolic link.
+	ReadLink(relative string) (string, error)
+
+	// Lstat is like os.Lstat.
+	Lstat(relative string) (os.FileInfo, error)
+
 	// Walk is like filepath.Walk, without the ordering guarantee.
 	Walk(relative string, walkFn filepath.WalkFunc) error
 
@@ -78,6 +85,142 @@ var (
 	ErrMissingPaths = errors.New("snap is unusable due to missing files")
 )
 
+type symlinkInfo struct {
+	// target is the furthest target we could evaluate.
+	target string
+	// targetMode is the mode of the final symlink target.
+	targetMode os.FileMode
+	// naiveTarget is the first symlink target.
+	naiveTarget string
+	// isExternal determines if the symlink is considered external
+	// relative to its container.
+	isExternal bool
+}
+
+// evalSymlink follows symlinks inside given container and returns
+// information about it's target.
+//
+// The symlink is followed inside the container until we cannot
+// continue further either due to absolute symlinks or symlinks
+// that escape the container.
+//
+//       max depth reached?<------
+//               /\               \
+//            yes  no              \
+//            /      \              \
+//           V        V              \
+//        error      path             \
+//                    │                \
+//                    V                 \
+//                read target            \
+//                    │                   \
+//                    V                    \
+//               is absolute?               \
+//                   /\                      \
+//                yes  no                     \
+//                /      \                     \
+//               V        V                     \
+//       isExternal     eval relative target     \
+//           +               \                    \
+//     return target          V                    \
+//                     escapes container?           \
+//                           /\                      \
+//                        yes  no                     \
+//                       /      \                      |
+//                      V        V                     |
+//              isExternal      is symlink?            |
+//                   +                /\               |
+//             return target       yes  no             │
+//                                /      \             │
+//                               V        V            │
+//                       !isExternal    path = target  │
+//                            +             \----------│
+//                      return target
+//
+func evalSymlink(c Container, path string) (symlinkInfo, error) {
+	var naiveTarget string
+
+	const maxDepth = 10
+	currentDepth := 0
+	for currentDepth < maxDepth {
+		currentDepth++
+		target, err := c.ReadLink(path)
+		if err != nil {
+			return symlinkInfo{}, err
+		}
+		// record first symlink target
+		if currentDepth == 1 {
+			naiveTarget = target
+		}
+
+		target = filepath.Clean(target)
+		// don't follow absolute targets
+		if filepath.IsAbs(target) {
+			return symlinkInfo{target, os.FileMode(0), naiveTarget, true}, nil
+		}
+
+		// evaluate target relative to symlink directory
+		target = filepath.Join(filepath.Dir(path), target)
+
+		// target escapes container, cannot evaluate further, let's return
+		if strings.Split(target, string(os.PathSeparator))[0] == ".." {
+			return symlinkInfo{target, os.FileMode(0), naiveTarget, true}, nil
+		}
+
+		info, err := c.Lstat(target)
+		// cannot follow bad targets
+		if err != nil {
+			return symlinkInfo{}, err
+		}
+
+		// non-symlink, let's return
+		if info.Mode().Type() != os.ModeSymlink {
+			return symlinkInfo{target, info.Mode(), naiveTarget, false}, nil
+		}
+
+		// we have another symlink
+		path = target
+	}
+
+	return symlinkInfo{}, fmt.Errorf("too many levels of symbolic links")
+}
+
+func shouldValidateSymlink(path string) bool {
+	// we only check meta directory for now
+	pathTokens := strings.Split(path, string(os.PathSeparator))
+	if pathTokens[0] == "meta" {
+		return true
+	}
+	return false
+}
+
+func evalAndValidateSymlink(c Container, path string) (symlinkInfo, error) {
+	pathTokens := strings.Split(path, string(os.PathSeparator))
+	// check if meta directory is a symlink
+	if len(pathTokens) == 1 && pathTokens[0] == "meta" {
+		return symlinkInfo{}, fmt.Errorf("meta directory cannot be a symlink")
+	}
+
+	info, err := evalSymlink(c, path)
+	if err != nil {
+		return symlinkInfo{}, err
+	}
+
+	if info.isExternal {
+		return symlinkInfo{}, fmt.Errorf("external symlink found: %s -> %s", path, info.naiveTarget)
+	}
+
+	// symlinks like this don't look innocent
+	badTargets := []string{".", "meta"}
+	for _, badTarget := range badTargets {
+		if info.target == badTarget {
+			return symlinkInfo{}, fmt.Errorf("bad symlink found: %s -> %s", path, info.naiveTarget)
+		}
+	}
+
+	return info, nil
+}
+
 // ValidateContainer does a minimal quick check on the container.
 func ValidateContainer(c Container, s *Info, logf func(format string, v ...interface{})) error {
 	// needsrx keeps track of things that need to have at least 0555 perms
@@ -172,20 +315,33 @@ func ValidateContainer(c Container, s *Info, logf func(format string, v ...inter
 			return nil
 		}
 
-		if needsrx[path] || mode.IsDir() {
+		// check symlinks for bad behavior
+		if mode&os.ModeSymlink != 0 && shouldValidateSymlink(path) {
+			symlinkInfo, err := evalAndValidateSymlink(c, path)
+			if err != nil {
+				logf("%s", err)
+				hasBadModes = true
+			} else {
+				// use target mode for checks below
+				mode = symlinkInfo.targetMode
+			}
+		}
+
+		if mode.IsDir() {
 			if mode.Perm()&0555 != 0555 {
 				logf("in snap %q: %q should be world-readable and executable, and isn't: %s", s.InstanceName(), 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 {
+			if needsrx[path] {
+				if mode.Perm()&0555 != 0555 {
+					logf("in snap %q: %q should be world-readable and executable, and isn't: %s", s.InstanceName(), path, mode)
+					hasBadModes = true
+				}
+			}
+			// XXX: do we need to match other directories?
+			if needsf[path] || strings.HasPrefix(path, "meta/") {
+				if mode&(os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 {
 					logf("in snap %q: %q should be a regular file (or a symlink) and isn't", s.InstanceName(), path)
 					hasBadModes = true
 				}