osutil/sys: reimplement getuid and chown with the right int type (can…

…onical#4291)

* osutil/sys: reimplement getuid and chown with the right int type

Due to golang#22739, os.Getuid() and co, and os.Chown and co, use the
wrong integer type and are thus wrong for high ids (especially
noticeable on 32-bit arches). This change addresses this (the only
remaining uses are for comparing with 0, which is OK), but it still
impacts godbus in our dependencies -- anything that uses godbus on a
system with high uids is broken today.

* osutil/sys: use a custom type for uids and gids

* cmd/snap-update-ns: also switch to uint32 uids/gids

* bboozzoo finds a bug, leads me to write a test, which finds a deeper bug

* in 14.04 tests go is not on the default PATH...

asserts/gpgkeypairmgr.go

@@ -26,7 +26,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strconv"
 	"strings"
 
 	"github.com/snapcore/snapd/osutil"
@@ -38,12 +37,7 @@ func ensureGPGHomeDirectory() (string, error) {
 		return "", err
 	}
 
-	uid, err := strconv.Atoi(real.Uid)
-	if err != nil {
-		return "", err
-	}
-
-	gid, err := strconv.Atoi(real.Gid)
+	uid, gid, err := osutil.UidGid(real)
 	if err != nil {
 		return "", err
 	}

client/login.go

@@ -25,7 +25,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strconv"
 
 	"github.com/snapcore/snapd/osutil"
 )
@@ -112,12 +111,7 @@ func writeAuthData(user User) error {
 		return err
 	}
 
-	uid, err := strconv.Atoi(real.Uid)
-	if err != nil {
-		return err
-	}
-
-	gid, err := strconv.Atoi(real.Gid)
+	uid, gid, err := osutil.UidGid(real)
 	if err != nil {
 		return err
 	}

cmd/snap-seccomp/main.go

@@ -651,7 +651,7 @@ func compile(content []byte, out string) error {
 	}
 
 	// write atomically
-	fout, err := osutil.NewAtomicFile(out, 0644, 0, -1, -1)
+	fout, err := osutil.NewAtomicFile(out, 0644, 0, osutil.NoChown, osutil.NoChown)
 	if err != nil {
 		return err
 	}

cmd/snap-update-ns/change.go

@@ -21,7 +21,6 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -66,9 +65,11 @@ var changePerform = (*Change).Perform
 // this change (such as mounted tmpfs or overlayfs).
 func (c *Change) Perform() ([]*Change, error) {
 	if c.Action == Mount {
-		mode := os.FileMode(0755)
-		uid := 0
-		gid := 0
+		const (
+			mode = 0755
+			uid  = 0
+			gid  = 0
+		)
 		// Create target mount directory if needed.
 		if err := ensureMountPoint(c.Entry.Dir, mode, uid, gid); err != nil {
 			return nil, err

cmd/snap-update-ns/export_test.go

@@ -27,6 +27,8 @@ import (
 	"time"
 
 	. "gopkg.in/check.v1"
+
+	"github.com/snapcore/snapd/osutil/sys"
 )
 
 var (
@@ -129,7 +131,7 @@ type SystemCalls interface {
 	Lstat(name string) (os.FileInfo, error)
 
 	Close(fd int) error
-	Fchown(fd int, uid int, gid int) error
+	Fchown(fd int, uid sys.UserID, gid sys.GroupID) error
 	Mkdirat(dirfd int, path string, mode uint32) error
 	Mount(source string, target string, fstype string, flags uintptr, data string) (err error)
 	Open(path string, flags int, mode uint32) (fd int, err error)
@@ -220,7 +222,7 @@ func (sys *SyscallRecorder) Close(fd int) error {
 	return sys.freeFd(fd)
 }
 
-func (sys *SyscallRecorder) Fchown(fd int, uid int, gid int) error {
+func (sys *SyscallRecorder) Fchown(fd int, uid sys.UserID, gid sys.GroupID) error {
 	return sys.call(fmt.Sprintf("fchown %d %d %d", fd, uid, gid))
 }
 

cmd/snap-update-ns/utils.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/snapcore/snapd/interfaces/mount"
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil/sys"
 )
 
 // not available through syscall
@@ -47,7 +48,7 @@ var (
 	sysOpen    = syscall.Open
 	sysOpenat  = syscall.Openat
 	sysUnmount = syscall.Unmount
-	sysFchown  = syscall.Fchown
+	sysFchown  = sys.Fchown
 
 	ioutilReadDir = ioutil.ReadDir
 )
@@ -64,7 +65,7 @@ func (e *ReadOnlyFsError) Error() string {
 // Create directories for all but the last segments and return the file
 // descriptor to the leaf directory. This function is a base for secure
 // variants of mkdir, touch and symlink.
-func secureMkPrefix(segments []string, perm os.FileMode, uid, gid int) (int, error) {
+func secureMkPrefix(segments []string, perm os.FileMode, uid sys.UserID, gid sys.GroupID) (int, error) {
 	logger.Debugf("secure-mk-prefix %q %v %d %d -> ...", segments, perm, uid, gid)
 
 	// Declare var and don't assign-declare below to ensure we don't swallow
@@ -105,7 +106,7 @@ func secureMkPrefix(segments []string, perm os.FileMode, uid, gid int) (int, err
 // by segments. This function can be used to construct subsequent elements of
 // the constructed path. The return value contains the newly created file
 // descriptor or -1 on error.
-func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid, gid int) (int, error) {
+func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid sys.UserID, gid sys.GroupID) (int, error) {
 	logger.Debugf("secure-mk-dir %d %q %d %v %d %d -> ...", fd, segments, i, perm, uid, gid)
 
 	segment := segments[i]
@@ -152,7 +153,7 @@ func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid, gid in
 // segments. This function is meant to be used to create the leaf file as a
 // preparation for a mount point. Existing files are reused without errors.
 // Newly created files have the specified mode and ownership.
-func secureMkFile(fd int, segments []string, i int, perm os.FileMode, uid, gid int) error {
+func secureMkFile(fd int, segments []string, i int, perm os.FileMode, uid sys.UserID, gid sys.GroupID) error {
 	logger.Debugf("secure-mk-file %d %q %d %v %d %d", fd, segments, i, perm, uid, gid)
 	segment := segments[i]
 	made := true
@@ -219,7 +220,7 @@ func splitIntoSegments(name string) ([]string, error) {
 // The uid and gid are used for the fchown(2) system call which is performed
 // after each segment is created and opened. The special value -1 may be used
 // to request that ownership is not changed.
-func secureMkdirAll(name string, perm os.FileMode, uid, gid int) error {
+func secureMkdirAll(name string, perm os.FileMode, uid sys.UserID, gid sys.GroupID) error {
 	logger.Debugf("secure-mkdir-all %q %v %d %d", name, perm, uid, gid)
 
 	// Only support absolute paths to avoid bugs in snap-confine when
@@ -258,7 +259,7 @@ func secureMkdirAll(name string, perm os.FileMode, uid, gid int) error {
 // This function is like secureMkdirAll but it creates an empty file instead of
 // a directory for the final path component. Each created directory component
 // is chowned to the desired user and group.
-func secureMkfileAll(name string, perm os.FileMode, uid, gid int) error {
+func secureMkfileAll(name string, perm os.FileMode, uid sys.UserID, gid sys.GroupID) error {
 	logger.Debugf("secure-mkfile-all %q %q %d %d", name, perm, uid, gid)
 
 	// Only support absolute paths to avoid bugs in snap-confine when
@@ -362,7 +363,7 @@ func planWritableMimic(dir string) ([]*Change, error) {
 	return changes, nil
 }
 
-func ensureMountPoint(path string, mode os.FileMode, uid int, gid int) error {
+func ensureMountPoint(path string, mode os.FileMode, uid sys.UserID, gid sys.GroupID) error {
 	// If the mount point is not present then create a directory in its
 	// place.  This is very naive, doesn't handle read-only file systems
 	// but it is a good starting point for people working with things like

cmd/snap-update-ns/utils_test.go

@@ -30,6 +30,7 @@ import (
 	update "github.com/snapcore/snapd/cmd/snap-update-ns"
 	"github.com/snapcore/snapd/interfaces/mount"
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil/sys"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -298,7 +299,7 @@ func (s *realSystemSuite) TestSecureMkdirAllForReal(c *C) {
 	// Create d (which already exists) with mode 0777 (but c.MkDir() used 0700
 	// internally and since we are not creating the directory we should not be
 	// changing that.
-	c.Assert(update.SecureMkdirAll(d, 0777, -1, -1), IsNil)
+	c.Assert(update.SecureMkdirAll(d, 0777, sys.FlagID, sys.FlagID), IsNil)
 	fi, err := os.Stat(d)
 	c.Assert(err, IsNil)
 	c.Check(fi.IsDir(), Equals, true)
@@ -308,7 +309,7 @@ func (s *realSystemSuite) TestSecureMkdirAllForReal(c *C) {
 	// check that it was applied. Note that default umask 022 is subtracted so
 	// effective directory has different permissions.
 	d1 := filepath.Join(d, "subdir")
-	c.Assert(update.SecureMkdirAll(d1, 0707, -1, -1), IsNil)
+	c.Assert(update.SecureMkdirAll(d1, 0707, sys.FlagID, sys.FlagID), IsNil)
 	fi, err = os.Stat(d1)
 	c.Assert(err, IsNil)
 	c.Check(fi.IsDir(), Equals, true)
@@ -317,7 +318,7 @@ func (s *realSystemSuite) TestSecureMkdirAllForReal(c *C) {
 	// Create d2, which is a deeper subdirectory, with another distinct mode
 	// and check that it was applied.
 	d2 := filepath.Join(d, "subdir/subdir/subdir")
-	c.Assert(update.SecureMkdirAll(d2, 0750, -1, -1), IsNil)
+	c.Assert(update.SecureMkdirAll(d2, 0750, sys.FlagID, sys.FlagID), IsNil)
 	fi, err = os.Stat(d2)
 	c.Assert(err, IsNil)
 	c.Check(fi.IsDir(), Equals, true)
@@ -502,7 +503,7 @@ func (s *realSystemSuite) TestSecureMkfileAllForReal(c *C) {
 	// check that it was applied. Note that default umask 022 is subtracted so
 	// effective directory has different permissions.
 	f1 := filepath.Join(d, "file")
-	c.Assert(update.SecureMkfileAll(f1, 0707, -1, -1), IsNil)
+	c.Assert(update.SecureMkfileAll(f1, 0707, sys.FlagID, sys.FlagID), IsNil)
 	fi, err := os.Stat(f1)
 	c.Assert(err, IsNil)
 	c.Check(fi.Mode().IsRegular(), Equals, true)
@@ -511,7 +512,7 @@ func (s *realSystemSuite) TestSecureMkfileAllForReal(c *C) {
 	// Create f2, which is a deeper subdirectory, with another distinct mode
 	// and check that it was applied.
 	f2 := filepath.Join(d, "subdir/subdir/file")
-	c.Assert(update.SecureMkfileAll(f2, 0750, -1, -1), IsNil)
+	c.Assert(update.SecureMkfileAll(f2, 0750, sys.FlagID, sys.FlagID), IsNil)
 	fi, err = os.Stat(f2)
 	c.Assert(err, IsNil)
 	c.Check(fi.Mode().IsRegular(), Equals, true)

daemon/api.go

@@ -2179,13 +2179,9 @@ func setupLocalUser(st *state.State, username, email string) error {
 	if err != nil {
 		return fmt.Errorf("cannot lookup user %q: %s", username, err)
 	}
-	uid, err := strconv.Atoi(user.Uid)
+	uid, gid, err := osutil.UidGid(user)
 	if err != nil {
-		return fmt.Errorf("cannot get uid of user %q: %s", username, err)
-	}
-	gid, err := strconv.Atoi(user.Gid)
-	if err != nil {
-		return fmt.Errorf("cannot get gid of user %q: %s", username, err)
+		return err
 	}
 	authDataFn := filepath.Join(user.HomeDir, ".snap", "auth.json")
 	if err := osutil.MkdirAllChown(filepath.Dir(authDataFn), 0700, uid, gid); err != nil {

osutil/export_test.go

@@ -26,6 +26,8 @@ import (
 	"os/user"
 	"syscall"
 	"time"
+
+	"github.com/snapcore/snapd/osutil/sys"
 )
 
 func MockUserLookup(mock func(name string) (*user.User, error)) func() {
@@ -85,7 +87,7 @@ func WaitingReaderGuts(r io.Reader) (io.Reader, *exec.Cmd) {
 	return wr.reader, wr.cmd
 }
 
-func MockChown(f func(*os.File, int, int) error) func() {
+func MockChown(f func(*os.File, sys.UserID, sys.GroupID) error) func() {
 	oldChown := chown
 	chown = f
 	return func() {

osutil/io.go

@@ -27,6 +27,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/snapcore/snapd/osutil/sys"
 	"github.com/snapcore/snapd/strutil"
 )
 
@@ -53,8 +54,8 @@ type AtomicFile struct {
 
 	target  string
 	tmpname string
-	uid     int
-	gid     int
+	uid     sys.UserID
+	gid     sys.GroupID
 	closed  bool
 	renamed bool
 }
@@ -75,11 +76,7 @@ type AtomicFile struct {
 // Also note that there are a number of scenarios where Commit fails and then
 // Cancel also fails. In all these scenarios your filesystem was probably in a
 // rather poor state. Good luck.
-func NewAtomicFile(filename string, perm os.FileMode, flags AtomicWriteFlags, uid, gid int) (aw *AtomicFile, err error) {
-	if (uid < 0) != (gid < 0) {
-		return nil, errors.New("internal error: AtomicFile needs none or both of uid and gid set")
-	}
-
+func NewAtomicFile(filename string, perm os.FileMode, flags AtomicWriteFlags, uid sys.UserID, gid sys.GroupID) (aw *AtomicFile, err error) {
 	if flags&AtomicWriteFollow != 0 {
 		if fn, err := os.Readlink(filename); err == nil || (fn != "" && os.IsNotExist(err)) {
 			if filepath.IsAbs(fn) {
@@ -136,15 +133,17 @@ func (aw *AtomicFile) Cancel() error {
 	return e2
 }
 
-var chown = (*os.File).Chown
+var chown = sys.Chown
+
+const NoChown = sys.FlagID
 
 // Commit the modification; make it permanent.
 //
 // If Commit succeeds, the writer is closed and further attempts to
 // write will fail. If Commit fails, the writer _might_ be closed;
 // Cancel() needs to be called to clean up.
 func (aw *AtomicFile) Commit() error {
-	if aw.uid > -1 && aw.gid > -1 {
+	if aw.uid != NoChown || aw.gid != NoChown {
 		if err := chown(aw.File, aw.uid, aw.gid); err != nil {
 			return err
 		}
@@ -185,27 +184,27 @@ func (aw *AtomicFile) Commit() error {
 // file created is an AtomicWriter, which is Committed before returning.
 //
 // AtomicWriteChown and AtomicWriteFileChown take an uid and a gid that can be
-// used to specify the ownership of the created file. They must be both
-// non-negative (in which case chown is called), or both negative (in which
-// case it isn't).
+// used to specify the ownership of the created file. A special value of
+// 0xffffffff (math.MaxUint32, or NoChown for convenience) can be used to
+// request no change to that attribute.
 //
 // AtomicWriteFile and AtomicWriteFileChown take the content to be written as a
 // []byte, and so work exactly like io.WriteFile(); AtomicWrite and
 // AtomicWriteChown take an io.Reader which is copied into the file instead,
 // and so are more amenable to streaming.
 func AtomicWrite(filename string, reader io.Reader, perm os.FileMode, flags AtomicWriteFlags) (err error) {
-	return AtomicWriteChown(filename, reader, perm, flags, -1, -1)
+	return AtomicWriteChown(filename, reader, perm, flags, NoChown, NoChown)
 }
 
 func AtomicWriteFile(filename string, data []byte, perm os.FileMode, flags AtomicWriteFlags) (err error) {
-	return AtomicWriteChown(filename, bytes.NewReader(data), perm, flags, -1, -1)
+	return AtomicWriteChown(filename, bytes.NewReader(data), perm, flags, NoChown, NoChown)
 }
 
-func AtomicWriteFileChown(filename string, data []byte, perm os.FileMode, flags AtomicWriteFlags, uid, gid int) (err error) {
+func AtomicWriteFileChown(filename string, data []byte, perm os.FileMode, flags AtomicWriteFlags, uid sys.UserID, gid sys.GroupID) (err error) {
 	return AtomicWriteChown(filename, bytes.NewReader(data), perm, flags, uid, gid)
 }
 
-func AtomicWriteChown(filename string, reader io.Reader, perm os.FileMode, flags AtomicWriteFlags, uid, gid int) (err error) {
+func AtomicWriteChown(filename string, reader io.Reader, perm os.FileMode, flags AtomicWriteFlags, uid sys.UserID, gid sys.GroupID) (err error) {
 	aw, err := NewAtomicFile(filename, perm, flags, uid, gid)
 	if err != nil {
 		return err