Merge pull request #331 from ebiggers/login-protector-perms

Set owner of login protectors to correct user

4 files changed, 51 insertions, 9 deletions

diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out
index c531f73..220d901 100644
--- a/cli-tests/t_encrypt_login.out
+++ b/cli-tests/t_encrypt_login.out
@@ -111,6 +111,8 @@ PROTECTOR         LINKED                              DESCRIPTION
 desc19  Yes (MNT_ROOT)  login protector for fscrypt-test-user
 desc20  No                                  custom protector "Recovery passphrase for dir"
 
+Protector is owned by fscrypt-test-user:fscrypt-test-user
+
 # Encrypt with login protector with --no-recovery
 ext4 filesystem "MNT" has 1 protector and 1 policy
 
diff --git a/cli-tests/t_encrypt_login.sh b/cli-tests/t_encrypt_login.sh
index 652d860..e03122d 100755
--- a/cli-tests/t_encrypt_login.sh
+++ b/cli-tests/t_encrypt_login.sh
@@ -27,13 +27,18 @@ show_status()
 	fi
 }
 
+get_login_protector()
+{
+	fscrypt status "$dir" | awk '/login protector/{print $1}'
+}
+
 begin "Encrypt with login protector"
 chown "$TEST_USER" "$dir"
 _user_do "echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase '$dir'"
 show_status true
 recovery_passphrase=$(grep -E '^ +[a-z]{20}$' "$dir/fscrypt_recovery_readme.txt" | sed 's/^ +//')
 recovery_protector=$(fscrypt status "$dir" | awk '/Recovery passphrase/{print $1}')
-login_protector=$(fscrypt status "$dir" | awk '/login protector/{print $1}')
+login_protector=$(get_login_protector)
 _print_header "=> Lock, then unlock with login passphrase"
 _user_do "fscrypt lock '$dir'"
 # FIXME: should we be able to use $MNT:$login_protector here?
@@ -57,6 +62,10 @@ show_status true
 begin "Encrypt with login protector as root"
 echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase --user="$TEST_USER" "$dir"
 show_status true
+# The newly-created login protector should be owned by the user, not root.
+login_protector=$(get_login_protector)
+owner=$(stat -c "%U:%G" "$MNT_ROOT/.fscrypt/protectors/$login_protector")
+echo -e "\nProtector is owned by $owner"
 
 begin "Encrypt with login protector with --no-recovery"
 chown "$TEST_USER" "$dir"
diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 9b5b7e2..456a4fc 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -37,6 +37,7 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
+	"os/user"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -47,6 +48,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/google/fscrypt/metadata"
+	"github.com/google/fscrypt/util"
 )
 
 // ErrAlreadySetup indicates that a filesystem is already setup for fscrypt.
@@ -392,7 +394,7 @@ func syncDirectory(dirPath string) error {
 
 // writeDataAtomic writes the data to the path such that the data is either
 // written to stable storage or an error is returned.
-func (m *Mount) writeDataAtomic(path string, data []byte) error {
+func (m *Mount) writeDataAtomic(path string, data []byte, owner *user.User) error {
 	// Write the data to a temporary file, sync it, then rename into place
 	// so that the operation will be atomic.
 	dirPath := filepath.Dir(path)
@@ -407,6 +409,14 @@ func (m *Mount) writeDataAtomic(path string, data []byte) error {
 		tempFile.Close()
 		return err
 	}
+	if owner != nil {
+		if err = util.Chown(tempFile, owner); err != nil {
+			log.Printf("could not set owner of %q to %v: %v",
+				path, owner.Username, err)
+			tempFile.Close()
+			return err
+		}
+	}
 	if _, err = tempFile.Write(data); err != nil {
 		tempFile.Close()
 		return err
@@ -428,7 +438,7 @@ func (m *Mount) writeDataAtomic(path string, data []byte) error {
 
 // addMetadata writes the metadata structure to the file with the specified
 // path. This will overwrite any existing data. The operation is atomic.
-func (m *Mount) addMetadata(path string, md metadata.Metadata) error {
+func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User) error {
 	if err := md.CheckValidity(); err != nil {
 		return errors.Wrap(err, "provided metadata is invalid")
 	}
@@ -439,7 +449,7 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata) error {
 	}
 
 	log.Printf("writing metadata to %q", path)
-	return m.writeDataAtomic(path, data)
+	return m.writeDataAtomic(path, data, owner)
 }
 
 // getMetadata reads the metadata structure from the file with the specified
@@ -480,7 +490,8 @@ func (m *Mount) removeMetadata(path string) error {
 // will fail with ErrLinkedProtector if a linked protector with this descriptor
 // already exists on the filesystem.
 func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
-	if err := m.CheckSetup(); err != nil {
+	var err error
+	if err = m.CheckSetup(); err != nil {
 		return err
 	}
 	if isRegularFile(m.linkedProtectorPath(data.ProtectorDescriptor)) {
@@ -488,7 +499,15 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 			data.ProtectorDescriptor, m.Path)
 	}
 	path := m.protectorPath(data.ProtectorDescriptor)
-	return m.addMetadata(path, data)
+
+	var owner *user.User
+	if data.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() {
+		owner, err = util.UserFromUID(data.Uid)
+		if err != nil {
+			return err
+		}
+	}
+	return m.addMetadata(path, data, owner)
 }
 
 // AddLinkedProtector adds a link in this filesystem to the protector metadata
@@ -528,7 +547,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	if err != nil {
 		return false, err
 	}
-	return true, m.writeDataAtomic(linkPath, []byte(newLink))
+	return true, m.writeDataAtomic(linkPath, []byte(newLink), nil)
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
@@ -609,7 +628,7 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
 		return err
 	}
 
-	return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data)
+	return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data, nil)
 }
 
 // GetPolicy looks up the policy metadata by descriptor.
diff --git a/util/util.go b/util/util.go
index d97a7ae..1dab335 100644
--- a/util/util.go
+++ b/util/util.go
@@ -121,9 +121,14 @@ func AtoiOrPanic(input string) int {
 	return i
 }
 
+// UserFromUID returns the User corresponding to the given user id.
+func UserFromUID(uid int64) (*user.User, error) {
+	return user.LookupId(strconv.FormatInt(uid, 10))
+}
+
 // EffectiveUser returns the user entry corresponding to the effective user.
 func EffectiveUser() (*user.User, error) {
-	return user.LookupId(strconv.Itoa(os.Geteuid()))
+	return UserFromUID(int64(os.Geteuid()))
 }
 
 // IsUserRoot checks if the effective user is root.
@@ -131,6 +136,13 @@ func IsUserRoot() bool {
 	return os.Geteuid() == 0
 }
 
+// Chown changes the owner of a File to a User.
+func Chown(file *os.File, user *user.User) error {
+	uid := AtoiOrPanic(user.Uid)
+	gid := AtoiOrPanic(user.Gid)
+	return file.Chown(uid, gid)
+}
+
 // IsKernelVersionAtLeast returns true if the Linux kernel version is at least
 // major.minor. If something goes wrong it assumes false.
 func IsKernelVersionAtLeast(major, minor int) bool {