preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308)

cmd/snap-exec/main.go

@@ -31,6 +31,7 @@ import (
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/snap/snapenv"
 )
 
 // for the tests
@@ -148,8 +149,17 @@ func snapExecApp(snapApp, revision, command string, args []string) error {
 	cmd := cmdArgv[0]
 	cmdArgs := cmdArgv[1:]
 
-	// build the environment from the yaml
-	env := append(os.Environ(), osutil.SubstituteEnv(app.Env())...)
+	// build the environment from the yaml, translating TMPDIR and
+	// similar variables back from where they were hidden when
+	// invoking the setuid snap-confine.
+	env := []string{}
+	for _, kv := range os.Environ() {
+		if strings.HasPrefix(kv, snapenv.PreservedUnsafePrefix) {
+			kv = kv[len(snapenv.PreservedUnsafePrefix):]
+		}
+		env = append(env, kv)
+	}
+	env = append(env, osutil.SubstituteEnv(app.Env())...)
 
 	// run the command
 	fullCmd := filepath.Join(app.Snap.MountDir(), cmd)

snap/snapenv/snapenv.go

@@ -31,18 +31,33 @@ import (
 	"github.com/snapcore/snapd/snap"
 )
 
+type preserveUnsafeEnvFlag int8
+
+const (
+	discardUnsafeFlag preserveUnsafeEnvFlag = iota
+	preserveUnsafeFlag
+)
+
 // ExecEnv returns the full environment that is required for
 // snap-{confine,exec}(like SNAP_{NAME,REVISION} etc are all set).
 //
 // It merges it with the existing os.Environ() and ensures the SNAP_*
-// overrides the any pre-existing environment variables.
+// overrides the any pre-existing environment variables. For a classic
+// snap, environment variables that are usually stripped out by ld.so
+// when starting a setuid process are renamed by prepending
+// PreservedUnsafePrefix -- which snap-exec will remove, restoring the
+// variables to their original names.
 //
 // With the extra parameter additional environment variables can be
 // supplied which will be set in the execution environment.
 func ExecEnv(info *snap.Info, extra map[string]string) []string {
 	// merge environment and the snap environment, note that the
 	// snap environment overrides pre-existing env entries
-	env := envMap(os.Environ())
+	preserve := discardUnsafeFlag
+	if info.NeedsClassic() {
+		preserve = preserveUnsafeFlag
+	}
+	env := envMap(os.Environ(), preserve)
 	snapEnv := snapEnv(info)
 	for k, v := range snapEnv {
 		env[k] = v
@@ -113,16 +128,61 @@ func userEnv(info *snap.Info, home string) map[string]string {
 	return result
 }
 
-// envMap creates a map from the given environment string list, e.g. the
-// list returned from os.Environ()
-func envMap(env []string) map[string]string {
+// Environment variables glibc strips out when running a setuid binary.
+// Taken from https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/generic/unsecvars.h;hb=HEAD
+// TODO: use go generate to obtain this list at build time.
+var unsafeEnv = map[string]bool{
+	"GCONV_PATH":       true,
+	"GETCONF_DIR":      true,
+	"GLIBC_TUNABLES":   true,
+	"HOSTALIASES":      true,
+	"LD_AUDIT":         true,
+	"LD_DEBUG":         true,
+	"LD_DEBUG_OUTPUT":  true,
+	"LD_DYNAMIC_WEAK":  true,
+	"LD_HWCAP_MASK":    true,
+	"LD_LIBRARY_PATH":  true,
+	"LD_ORIGIN_PATH":   true,
+	"LD_PRELOAD":       true,
+	"LD_PROFILE":       true,
+	"LD_SHOW_AUXV":     true,
+	"LD_USE_LOAD_BIAS": true,
+	"LOCALDOMAIN":      true,
+	"LOCPATH":          true,
+	"MALLOC_TRACE":     true,
+	"NIS_PATH":         true,
+	"NLSPATH":          true,
+	"RESOLV_HOST_CONF": true,
+	"RES_OPTIONS":      true,
+	"TMPDIR":           true,
+	"TZDIR":            true,
+}
+
+const PreservedUnsafePrefix = "SNAP_SAVED_"
+
+// envMap creates a map from the given environment string list,
+// e.g. the list returned from os.Environ(). If preserveUnsafeVars
+// rename variables that will be stripped out by the dynamic linker
+// executing the setuid snap-confine by prepending their names with
+// PreservedUnsafePrefix.
+func envMap(env []string, preserveUnsafeEnv preserveUnsafeEnvFlag) map[string]string {
 	envMap := map[string]string{}
 	for _, kv := range env {
+		// snap-exec unconditionally renames variables
+		// starting with PreservedUnsafePrefix so skip any
+		// that are already present in the environment to
+		// avoid confusion.
+		if strings.HasPrefix(kv, PreservedUnsafePrefix) {
+			continue
+		}
 		l := strings.SplitN(kv, "=", 2)
 		if len(l) < 2 {
 			continue // strange
 		}
 		k, v := l[0], l[1]
+		if preserveUnsafeEnv == preserveUnsafeFlag && unsafeEnv[k] {
+			k = PreservedUnsafePrefix + k
+		}
 		envMap[k] = v
 	}
 	return envMap

snap/snapenv/snapenv_test.go

@@ -23,18 +23,22 @@ import (
 	"fmt"
 	"os"
 	"os/user"
+	"strings"
 	"testing"
 
 	. "gopkg.in/check.v1"
 
 	"github.com/snapcore/snapd/arch"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/testutil"
 )
 
 func Test(t *testing.T) { TestingT(t) }
 
-type HTestSuite struct{}
+type HTestSuite struct {
+	testutil.BaseTest
+}
 
 var _ = Suite(&HTestSuite{})
 
@@ -137,18 +141,58 @@ func (s *HTestSuite) TestSnapRunSnapExecEnv(c *C) {
 	}
 }
 
+func envValue(env []string, key string) (bool, string) {
+	for _, item := range env {
+		if strings.HasPrefix(item, key+"=") {
+			return true, strings.SplitN(item, "=", 2)[1]
+		}
+	}
+	return false, ""
+}
+
 func (s *HTestSuite) TestExtraEnvForExecEnv(c *C) {
 	info, err := snap.InfoFromSnapYaml(mockYaml)
 	c.Assert(err, IsNil)
 	info.SideInfo.Revision = snap.R(42)
 
 	env := ExecEnv(info, map[string]string{"FOO": "BAR"})
-	found := false
-	for _, item := range env {
-		if item == "FOO=BAR" {
-			found = true
-			break
-		}
+	found, val := envValue(env, "FOO")
+	c.Assert(found, Equals, true)
+	c.Assert(val, Equals, "BAR")
+}
+
+func setenvWithReset(s *HTestSuite, key string, val string) {
+	tmpdirEnv, tmpdirFound := os.LookupEnv("TMPDIR")
+	os.Setenv("TMPDIR", "/var/tmp")
+	if tmpdirFound {
+		s.AddCleanup(func() { os.Setenv("TMPDIR", tmpdirEnv) })
+	} else {
+		s.AddCleanup(func() { os.Unsetenv("TMPDIR") })
 	}
+}
+
+func (s *HTestSuite) TestExecEnvNoRenameTMPDIRForNonClassic(c *C) {
+	setenvWithReset(s, "TMPDIR", "/var/tmp")
+
+	env := ExecEnv(mockSnapInfo, map[string]string{})
+
+	found, val := envValue(env, "TMPDIR")
+	c.Assert(found, Equals, true)
+	c.Assert(val, Equals, "/var/tmp")
+
+	found, _ = envValue(env, PreservedUnsafePrefix+"TMPDIR")
+	c.Assert(found, Equals, false)
+}
+
+func (s *HTestSuite) TestExecEnvRenameTMPDIRForClassic(c *C) {
+	setenvWithReset(s, "TMPDIR", "/var/tmp")
+
+	env := ExecEnv(mockClassicSnapInfo, map[string]string{})
+
+	found, _ := envValue(env, "TMPDIR")
+	c.Assert(found, Equals, false)
+
+	found, val := envValue(env, PreservedUnsafePrefix+"TMPDIR")
 	c.Assert(found, Equals, true)
+	c.Assert(val, Equals, "/var/tmp")
 }

tests/main/confinement-classic/task.yaml

@@ -21,6 +21,8 @@ execute: |
     run_install --classic
     $SNAP_MOUNT_DIR/bin/test-snapd-hello-classic | MATCH 'Hello Classic!'
 
+    TMPDIR=/tmpdir $SNAP_MOUNT_DIR/bin/test-snapd-hello-classic t | MATCH TMPDIR=/tmpdir
+
     if [ "$(snap debug confinement)" = partial ]; then
         exit 0
     fi

tests/main/confinement-classic/test-snapd-hello-classic/test-snapd-hello-classic.c

@@ -1,7 +1,12 @@
 #include <stdio.h>
+#include <stdlib.h>
 
-int main()
+int main(int argc, char **argv)
 {
-	printf("Hello Classic!\n");
+	if (argc == 1) {
+		printf("Hello Classic!\n");
+	} else {
+		printf("TMPDIR=%s\n", getenv("TMPDIR"));
+	}
 	return 0;
 }

tests/main/snap-env/task.yaml

@@ -44,6 +44,9 @@ execute: |
     MATCH "^EXTRA_CACHE_DIR=$HOME/snap/test-snapd-tools/x1/.cache" < extra-vars.txt
     test $(wc -l < extra-vars.txt) -eq 4
 
+    echo "Ensure that TMPDIR is not passed through to a confined snap"
+    TMPDIR=/foobar test-snapd-tools.env | grep -qv ^TMPDIR=
+
     echo "Ensure that SNAP, PATH and HOME are what we expect"
     MATCH "^SNAP=/snap/test-snapd-tools/x1$"                                                                < misc-vars.txt
     MATCH '^PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games$' < misc-vars.txt