Merge pull request #9897 from anonymouse64/bugfix/lp-1910298-part-1

usersession/autostart: change ~/snap perms to 0700 on startup

cmd/snap/cmd_userd.go

@@ -24,10 +24,12 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
+	"path/filepath"
 	"syscall"
 
 	"github.com/jessevdk/go-flags"
 
+	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/i18n"
 	"github.com/snapcore/snapd/snapdtool"
 	"github.com/snapcore/snapd/usersession/agent"
@@ -60,12 +62,45 @@ func init() {
 	cmd.hidden = true
 }
 
+var osChmod = os.Chmod
+
+func maybeFixupUsrSnapPermissions() error {
+	usr, err := userCurrent()
+	if err != nil {
+		return err
+	}
+
+	usrSnapDir := filepath.Join(usr.HomeDir, dirs.UserHomeSnapDir)
+
+	// restrict the user's "snap dir", i.e. /home/$USER/snap, to be private with
+	// permissions o0700 so that other users cannot read the data there, some
+	// snaps such as chromium etc may store secrets inside this directory
+	// note that this operation is safe since `userd --autostart` runs as the
+	// user so there is no issue with this modification being performed as root,
+	// and being vulnerable to symlink switching attacks, etc.
+	if err := osChmod(usrSnapDir, 0700); err != nil {
+		// if the dir doesn't exist for some reason (i.e. maybe this user has
+		// never used snaps but snapd is still installed) then ignore the error
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("cannot restrict user snap home dir %q: %v", usrSnapDir, err)
+		}
+	}
+
+	return nil
+}
+
 func (x *cmdUserd) Execute(args []string) error {
 	if len(args) > 0 {
 		return ErrExtraArgs
 	}
 
 	if x.Autostart {
+		// autostart is called when starting the graphical session, use that as
+		// an opportunity to fix ~/snap permission bits
+		if err := maybeFixupUsrSnapPermissions(); err != nil {
+			fmt.Fprintf(Stderr, "failure fixing ~/snap permissions: %v\n", err)
+		}
+
 		return x.runAutostart()
 	}
 

cmd/snap/cmd_userd_test.go

@@ -25,6 +25,9 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"os/user"
+	"path"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"time"
@@ -36,6 +39,7 @@ import (
 	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/testutil"
+	"github.com/snapcore/snapd/usersession/autostart"
 )
 
 type userdSuite struct {
@@ -192,3 +196,85 @@ func (s *userdSuite) TestSignalNotify(c *C) {
 		c.Fatal("signal not received within 5s")
 	}
 }
+
+func (s *userdSuite) TestAutostartSessionAppsRestrictsPermissions(c *C) {
+	userDir := path.Join(c.MkDir(), "home")
+	mockUserCurrent := func() (*user.User, error) {
+		return &user.User{HomeDir: userDir}, nil
+	}
+	r := snap.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	r = autostart.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	// first make the "snap" dir permissive with 0755 perms
+	err := os.MkdirAll(filepath.Join(userDir, "snap"), 0755)
+	c.Assert(err, IsNil)
+
+	// make sure the perms are as we expect them if somehow the dir already
+	// existed, MkdirAll wouldn't have changed the perms
+	st, err := os.Stat(filepath.Join(userDir, "snap"))
+	c.Assert(err, IsNil)
+	c.Assert(st.Mode()&os.ModePerm, Equals, os.FileMode(0755))
+
+	// run autostart
+	args, err := snap.Parser(snap.Client()).ParseArgs([]string{"userd", "--autostart"})
+	c.Assert(err, IsNil)
+	c.Assert(args, DeepEquals, []string{})
+
+	// make sure that the directory was restricted
+	st, err = os.Stat(filepath.Join(userDir, "snap"))
+	c.Assert(err, IsNil)
+	c.Assert(st.Mode()&os.ModePerm, Equals, os.FileMode(0700))
+}
+
+func (s *userdSuite) TestAutostartSessionAppsLogsWhenItCannotRestrictPermissions(c *C) {
+	userDir := path.Join(c.MkDir(), "home")
+	mockUserCurrent := func() (*user.User, error) {
+		return &user.User{HomeDir: userDir}, nil
+	}
+	r := snap.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	r = autostart.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	r = snap.MockOsChmod(func(name string, mode os.FileMode) error {
+		c.Assert(name, Equals, filepath.Join(userDir, "snap"))
+		c.Assert(mode, Equals, os.FileMode(0700))
+
+		return fmt.Errorf("cannot os.Chmod because the test says so")
+	})
+	defer r()
+
+	// run autostart
+	args, err := snap.Parser(snap.Client()).ParseArgs([]string{"userd", "--autostart"})
+	c.Assert(err, IsNil)
+	c.Assert(args, DeepEquals, []string{})
+
+	c.Assert(s.stderr.String(), testutil.Contains, "cannot os.Chmod because the test says so")
+}
+
+func (s *userdSuite) TestAutostartSessionAppsRestrictsPermissionsNoCreateSnapDir(c *C) {
+	userDir := path.Join(c.MkDir(), "home")
+	mockUserCurrent := func() (*user.User, error) {
+		return &user.User{HomeDir: userDir}, nil
+	}
+	r := snap.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	r = autostart.MockUserCurrent(mockUserCurrent)
+	defer r()
+
+	// ensure that the "snap" dir doesn't already exist
+	c.Assert(filepath.Join(userDir, "snap"), testutil.FileAbsent)
+
+	// run autostart
+	args, err := snap.Parser(snap.Client()).ParseArgs([]string{"userd", "--autostart"})
+	c.Assert(err, IsNil)
+	c.Assert(args, DeepEquals, []string{})
+
+	// make sure that the directory was not created
+	c.Assert(filepath.Join(userDir, "snap"), testutil.FileAbsent)
+}

cmd/snap/export_test.go

@@ -400,3 +400,11 @@ func MockSnapdWaitForFullSystemReboot(t time.Duration) (restore func()) {
 		snapdWaitForFullSystemReboot = old
 	}
 }
+
+func MockOsChmod(f func(string, os.FileMode) error) (restore func()) {
+	old := osChmod
+	osChmod = f
+	return func() {
+		osChmod = old
+	}
+}

tests/main/snap-user-dir-perms-fixed/task.yaml

@@ -0,0 +1,31 @@
+summary: Ensure snap userd autostart fixes up directory permissions of ~/snap
+
+# don't run on trusty, tests.session does not work on trusty due to the
+# lack of user session support there
+systems:
+  - -ubuntu-14.04-64
+
+environment:
+  USER/root: root
+  USER/test: test
+  HOMEDIR/root: /root/snap
+  HOMEDIR/test: /home/test/snap
+
+prepare: |
+  # Prepare for using sessions as the given user
+  tests.session prepare -u "$USER"
+
+execute: |
+  # set permissive permissions on the user's home dir
+  tests.session -u "$USER" exec mkdir -p "$HOMEDIR"
+  tests.session -u "$USER" exec chmod 0777 "$HOMEDIR"
+
+  # run autostart
+  tests.session -u "$USER" exec snap userd --autostart
+
+  # check the permissions are fixed up now
+  tests.session -u "$USER" exec stat -c "%a" "$HOMEDIR" | MATCH 700
+
+restore: |
+  # Restore after using sessions as the given user
+  tests.session restore -u "$USER"

usersession/autostart/autostart.go

@@ -33,6 +33,7 @@ import (
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/strutil"
 	"github.com/snapcore/snapd/strutil/shlex"
@@ -226,6 +227,15 @@ func makeStdStreams(identifier string) (stdout *os.File, stderr *os.File) {
 
 var userCurrent = user.Current
 
+func MockUserCurrent(f func() (*user.User, error)) (restore func()) {
+	osutil.MustBeTestBinary("mocking can only be done in tests")
+	old := userCurrent
+	userCurrent = f
+	return func() {
+		userCurrent = old
+	}
+}
+
 // AutostartSessionApps starts applications which have placed their desktop
 // files in $SNAP_USER_DATA/.config/autostart
 //

usersession/autostart/export_test.go

@@ -19,23 +19,11 @@
 
 package autostart
 
-import (
-	"os/user"
-)
-
 var (
 	LoadAutostartDesktopFile = loadAutostartDesktopFile
 	AutostartCmd             = autostartCmd
 )
 
-func MockUserCurrent(f func() (*user.User, error)) func() {
-	origUserCurrent := userCurrent
-	userCurrent = f
-	return func() {
-		userCurrent = origUserCurrent
-	}
-}
-
 func MockCurrentDesktop(current string) func() {
 	old := currentDesktop
 	currentDesktop = splitSkippingEmpty(current, ':')