From 1a47718420317f893831b0223153d56005d5b02b Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: filesystem: validate size and type of metadata files

Don't allow reading metadata files that are very large, as they can
crash the program due to the memory required.  Similarly, don't allow
reading metadata files that aren't regular files, such as FIFOs, or
symlinks (which could point to a device node like /dev/zero), as that
can hang the program.  Both issues were particularly problematic for
pam_fscrypt, as they could prevent users from being able to log in.

Note: these checks are arguably unneeded if we strictly check the file
ownership too, which a later commit will do.  But there's no reason not
to do these basic checks too.
---
 filesystem/filesystem.go | 65 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index da6b9cc..3c44160 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -34,6 +34,7 @@ package filesystem
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
 	"log"
 	"os"
@@ -213,6 +214,12 @@ const (
 	// The metadata files are globally visible, but can only be deleted by
 	// the user that created them
 	filePermissions = 0644
+
+	// Maximum size of a metadata file.  This value is arbitrary, and it can
+	// be changed.  We just set a reasonable limit that shouldn't be reached
+	// in practice, except by users trying to cause havoc by creating
+	// extremely large files in the metadata directories.
+	maxMetadataFileSize = 16384
 )
 
 func (m *Mount) String() string {
@@ -496,10 +503,60 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 	return m.writeDataAtomic(path, data, owner)
 }
 
+// readMetadataFileSafe gets the contents of a metadata file extra-carefully,
+// considering that it could be a malicious file created to cause a
+// denial-of-service.  Specifically, the following checks are done:
+//
+// - It must be a regular file, not another type of file like a symlink or FIFO.
+//   (Symlinks aren't bad by themselves, but given that a malicious user could
+//   point one to absolutely anywhere, and there is no known use case for the
+//   metadata files themselves being symlinks, it seems best to disallow them.)
+// - It must have a reasonable size (<= maxMetadataFileSize).
+//
+// Take care to avoid TOCTOU (time-of-check-time-of-use) bugs when doing these
+// tests.  Notably, we must open the file before checking the file type, as the
+// file type could change between any previous checks and the open.  When doing
+// this, O_NOFOLLOW is needed to avoid following a symlink (this applies to the
+// last path component only), and O_NONBLOCK is needed to avoid blocking if the
+// file is a FIFO.
+//
+// This function returns the data read.
+func readMetadataFileSafe(path string) ([]byte, error) {
+	file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer file.Close()
+
+	info, err := file.Stat()
+	if err != nil {
+		return nil, err
+	}
+	if !info.Mode().IsRegular() {
+		return nil, &ErrCorruptMetadata{path, errors.New("not a regular file")}
+	}
+	// Clear O_NONBLOCK, since it has served its purpose when opening the
+	// file, and the behavior of reading from a regular file with O_NONBLOCK
+	// is technically unspecified.
+	if _, err = unix.FcntlInt(file.Fd(), unix.F_SETFL, 0); err != nil {
+		return nil, &os.PathError{Op: "clearing O_NONBLOCK", Path: path, Err: err}
+	}
+	// Read the file contents, allowing at most maxMetadataFileSize bytes.
+	reader := &io.LimitedReader{R: file, N: maxMetadataFileSize + 1}
+	data, err := ioutil.ReadAll(reader)
+	if err != nil {
+		return nil, err
+	}
+	if reader.N == 0 {
+		return nil, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")}
+	}
+	return data, nil
+}
+
 // getMetadata reads the metadata structure from the file with the specified
 // path. Only reads normal metadata files, not linked metadata.
 func (m *Mount) getMetadata(path string, md metadata.Metadata) error {
-	data, err := ioutil.ReadFile(path)
+	data, err := readMetadataFileSafe(path)
 	if err != nil {
 		log.Printf("could not read metadata from %q: %v", path, err)
 		return err
@@ -569,7 +626,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	linkPath := m.linkedProtectorPath(descriptor)
 
 	// Check whether the link already exists.
-	existingLink, err := ioutil.ReadFile(linkPath)
+	existingLink, err := readMetadataFileSafe(linkPath)
 	if err == nil {
 		existingLinkedMnt, err := getMountFromLink(string(existingLink))
 		if err != nil {
@@ -594,7 +651,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
-// fail with ErrNoMetadata if the descriptor is a linked protector.
+// fail with ErrProtectorNotFound if the descriptor is a linked protector.
 func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, err
@@ -617,7 +674,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData
 	}
 	// Get the link data from the link file
 	path := m.linkedProtectorPath(descriptor)
-	link, err := ioutil.ReadFile(path)
+	link, err := readMetadataFileSafe(path)
 	if err != nil {
 		// If the link doesn't exist, try for a regular protector.
 		if os.IsNotExist(err) {
-- 
cgit v1.2.3


From b44fbe71e1e93c47050322af51725bac997641e0 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: filesystem: reject spoofed login protectors

If a login protector contains a UID that differs from the file owner
(and the file owner is not root), it might be a spoofed file that was
created maliciously, so make sure to consider such files to be invalid.
---
 filesystem/filesystem.go | 57 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 20 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 3c44160..8ce8bde 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -42,6 +42,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"syscall"
 	"time"
 
 	"github.com/golang/protobuf/proto"
@@ -520,58 +521,60 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 // last path component only), and O_NONBLOCK is needed to avoid blocking if the
 // file is a FIFO.
 //
-// This function returns the data read.
-func readMetadataFileSafe(path string) ([]byte, error) {
+// This function returns the data read as well as the UID of the user who owns
+// the file.  The returned UID is needed for login protectors, where the UID
+// needs to be cross-checked with the UID stored in the file itself.
+func readMetadataFileSafe(path string) ([]byte, int64, error) {
 	file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
 	if err != nil {
-		return nil, err
+		return nil, -1, err
 	}
 	defer file.Close()
 
 	info, err := file.Stat()
 	if err != nil {
-		return nil, err
+		return nil, -1, err
 	}
 	if !info.Mode().IsRegular() {
-		return nil, &ErrCorruptMetadata{path, errors.New("not a regular file")}
+		return nil, -1, &ErrCorruptMetadata{path, errors.New("not a regular file")}
 	}
 	// Clear O_NONBLOCK, since it has served its purpose when opening the
 	// file, and the behavior of reading from a regular file with O_NONBLOCK
 	// is technically unspecified.
 	if _, err = unix.FcntlInt(file.Fd(), unix.F_SETFL, 0); err != nil {
-		return nil, &os.PathError{Op: "clearing O_NONBLOCK", Path: path, Err: err}
+		return nil, -1, &os.PathError{Op: "clearing O_NONBLOCK", Path: path, Err: err}
 	}
 	// Read the file contents, allowing at most maxMetadataFileSize bytes.
 	reader := &io.LimitedReader{R: file, N: maxMetadataFileSize + 1}
 	data, err := ioutil.ReadAll(reader)
 	if err != nil {
-		return nil, err
+		return nil, -1, err
 	}
 	if reader.N == 0 {
-		return nil, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")}
+		return nil, -1, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")}
 	}
-	return data, nil
+	return data, int64(info.Sys().(*syscall.Stat_t).Uid), nil
 }
 
 // getMetadata reads the metadata structure from the file with the specified
 // path. Only reads normal metadata files, not linked metadata.
-func (m *Mount) getMetadata(path string, md metadata.Metadata) error {
-	data, err := readMetadataFileSafe(path)
+func (m *Mount) getMetadata(path string, md metadata.Metadata) (int64, error) {
+	data, owner, err := readMetadataFileSafe(path)
 	if err != nil {
 		log.Printf("could not read metadata from %q: %v", path, err)
-		return err
+		return -1, err
 	}
 
 	if err := proto.Unmarshal(data, md); err != nil {
-		return &ErrCorruptMetadata{path, err}
+		return -1, &ErrCorruptMetadata{path, err}
 	}
 
 	if err := md.CheckValidity(); err != nil {
-		return &ErrCorruptMetadata{path, err}
+		return -1, &ErrCorruptMetadata{path, err}
 	}
 
 	log.Printf("successfully read metadata from %q", path)
-	return nil
+	return owner, nil
 }
 
 // removeMetadata deletes the metadata struct from the file with the specified
@@ -626,7 +629,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	linkPath := m.linkedProtectorPath(descriptor)
 
 	// Check whether the link already exists.
-	existingLink, err := readMetadataFileSafe(linkPath)
+	existingLink, _, err := readMetadataFileSafe(linkPath)
 	if err == nil {
 		existingLinkedMnt, err := getMountFromLink(string(existingLink))
 		if err != nil {
@@ -658,11 +661,25 @@ func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData,
 	}
 	data := new(metadata.ProtectorData)
 	path := m.protectorPath(descriptor)
-	err := m.getMetadata(path, data)
+	owner, err := m.getMetadata(path, data)
 	if os.IsNotExist(err) {
 		err = &ErrProtectorNotFound{descriptor, m}
 	}
-	return data, err
+	if err != nil {
+		return nil, err
+	}
+	// Login protectors have their UID stored in the file.  Since normally
+	// any user can create files in the fscrypt metadata directories, for a
+	// login protector to be considered valid it *must* be owned by the
+	// claimed user or by root.  Note: fscrypt v0.3.2 and later always makes
+	// login protectors owned by the user, but previous versions could
+	// create them owned by root -- that is the main reason we allow root.
+	if data.Source == metadata.SourceType_pam_passphrase && owner != 0 && owner != data.Uid {
+		log.Printf("WARNING: %q claims to be the login protector for uid %d, but it is owned by uid %d.  Needs to be %d or 0.",
+			path, data.Uid, owner, data.Uid)
+		return nil, &ErrCorruptMetadata{path, errors.New("login protector belongs to wrong user")}
+	}
+	return data, nil
 }
 
 // GetProtector returns the Mount of the filesystem containing the information
@@ -674,7 +691,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData
 	}
 	// Get the link data from the link file
 	path := m.linkedProtectorPath(descriptor)
-	link, err := readMetadataFileSafe(path)
+	link, _, err := readMetadataFileSafe(path)
 	if err != nil {
 		// If the link doesn't exist, try for a regular protector.
 		if os.IsNotExist(err) {
@@ -737,7 +754,7 @@ func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) {
 		return nil, err
 	}
 	data := new(metadata.PolicyData)
-	err := m.getMetadata(m.PolicyPath(descriptor), data)
+	_, err := m.getMetadata(m.PolicyPath(descriptor), data)
 	if os.IsNotExist(err) {
 		err = &ErrPolicyNotFound{descriptor, m}
 	}
-- 
cgit v1.2.3


From 45599bdfad300f1a034c70dd70b4bd180d66f52c Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: filesystem: fall back to non-atomic overwrites when required

To allow users to update fscrypt metadata they own in
single-user-writable metadata directories (introduced by the next
commit), fall back to non-atomic overwrites when atomic ones can't be
done due to not having write access to the directory.
---
 filesystem/filesystem.go | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 8ce8bde..c39514a 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -444,14 +444,47 @@ func syncDirectory(dirPath string) error {
 	return dirFile.Close()
 }
 
-// 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, owner *user.User) error {
+func (m *Mount) overwriteDataNonAtomic(path string, data []byte) error {
+	file, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC|unix.O_NOFOLLOW, 0)
+	if err != nil {
+		return err
+	}
+	if _, err = file.Write(data); err != nil {
+		log.Printf("WARNING: overwrite of %q failed; file will be corrupted!", path)
+		file.Close()
+		return err
+	}
+	if err = file.Sync(); err != nil {
+		file.Close()
+		return err
+	}
+	if err = file.Close(); err != nil {
+		return err
+	}
+	log.Printf("successfully overwrote %q non-atomically", path)
+	return nil
+}
+
+// writeData writes the given data to the given path such that, if possible, the
+// data is either written to stable storage or an error is returned.  If a file
+// already exists at the path, it will be replaced.
+//
+// However, if the process doesn't have write permission to the directory but
+// does have write permission to the file itself, then as a fallback the file is
+// overwritten in-place rather than replaced.  Note that this may be non-atomic.
+func (m *Mount) writeData(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)
 	tempFile, err := ioutil.TempFile(dirPath, tempPrefix)
 	if err != nil {
+		log.Print(err)
+		if os.IsPermission(err) {
+			if _, err = os.Lstat(path); err == nil {
+				log.Printf("trying non-atomic overwrite of %q", path)
+				return m.overwriteDataNonAtomic(path, data)
+			}
+		}
 		return err
 	}
 	defer os.Remove(tempFile.Name())
@@ -501,7 +534,7 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 	}
 
 	log.Printf("writing metadata to %q", path)
-	return m.writeDataAtomic(path, data, owner)
+	return m.writeData(path, data, owner)
 }
 
 // readMetadataFileSafe gets the contents of a metadata file extra-carefully,
@@ -650,7 +683,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	if err != nil {
 		return false, err
 	}
-	return true, m.writeDataAtomic(linkPath, []byte(newLink), nil)
+	return true, m.writeData(linkPath, []byte(newLink), nil)
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
-- 
cgit v1.2.3


From 6e355131670ad014e45f879475ddf800f0080d41 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: Make 'fscrypt setup' offer a choice of directory modes

World-writable directories are not appropriate for some systems, so
offer a choice of single-user-writable and world-writable modes, with
single-user-writable being the default.  Add a new documentation section
to help users decide which one to use.
---
 filesystem/filesystem.go | 74 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 10 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index c39514a..1450f0f 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -96,6 +96,16 @@ func (err *ErrMakeLink) Error() string {
 		err.Target.Path, err.UnderlyingError)
 }
 
+// ErrNoCreatePermission indicates that the current user lacks permission to
+// create fscrypt metadata on the given filesystem.
+type ErrNoCreatePermission struct {
+	Mount *Mount
+}
+
+func (err *ErrNoCreatePermission) Error() string {
+	return fmt.Sprintf("user lacks permission to create fscrypt metadata on %s", err.Mount.Path)
+}
+
 // ErrNotAMountpoint indicates that a path is not a mountpoint.
 type ErrNotAMountpoint struct {
 	Path string
@@ -209,9 +219,6 @@ const (
 
 	// The base directory should be read-only (except for the creator)
 	basePermissions = 0755
-	// The subdirectories should be writable to everyone, but they have the
-	// sticky bit set so users cannot delete other users' metadata.
-	dirPermissions = os.ModeSticky | 0777
 	// The metadata files are globally visible, but can only be deleted by
 	// the user that created them
 	filePermissions = 0644
@@ -223,6 +230,18 @@ const (
 	maxMetadataFileSize = 16384
 )
 
+// SetupMode is a mode for creating the fscrypt metadata directories.
+type SetupMode int
+
+const (
+	// SingleUserWritable specifies to make the fscrypt metadata directories
+	// writable by a single user (usually root) only.
+	SingleUserWritable SetupMode = iota
+	// WorldWritable specifies to make the fscrypt metadata directories
+	// world-writable (with the sticky bit set).
+	WorldWritable
+)
+
 func (m *Mount) String() string {
 	return fmt.Sprintf(`%s
 	FilesystemType: %s
@@ -359,8 +378,8 @@ func (m *Mount) CheckSetup() error {
 	}
 	// Run all the checks so we will always get all the warnings
 	baseGood := isDirCheckPerm(m.BaseDir(), basePermissions)
-	policyGood := isDirCheckPerm(m.PolicyDir(), dirPermissions)
-	protectorGood := isDirCheckPerm(m.ProtectorDir(), dirPermissions)
+	policyGood := isDir(m.PolicyDir())
+	protectorGood := isDir(m.ProtectorDir())
 
 	if baseGood && policyGood && protectorGood {
 		return nil
@@ -370,7 +389,7 @@ func (m *Mount) CheckSetup() error {
 
 // makeDirectories creates the three metadata directories with the correct
 // permissions. Note that this function overrides the umask.
-func (m *Mount) makeDirectories() error {
+func (m *Mount) makeDirectories(setupMode SetupMode) error {
 	// Zero the umask so we get the permissions we want
 	oldMask := unix.Umask(0)
 	defer func() {
@@ -380,17 +399,51 @@ func (m *Mount) makeDirectories() error {
 	if err := os.Mkdir(m.BaseDir(), basePermissions); err != nil {
 		return err
 	}
-	if err := os.Mkdir(m.PolicyDir(), dirPermissions); err != nil {
+
+	var dirMode os.FileMode
+	switch setupMode {
+	case SingleUserWritable:
+		dirMode = 0755
+	case WorldWritable:
+		dirMode = os.ModeSticky | 0777
+	}
+	if err := os.Mkdir(m.PolicyDir(), dirMode); err != nil {
 		return err
 	}
-	return os.Mkdir(m.ProtectorDir(), dirPermissions)
+	return os.Mkdir(m.ProtectorDir(), dirMode)
+}
+
+// GetSetupMode returns the current mode for fscrypt metadata creation on this
+// filesystem.
+func (m *Mount) GetSetupMode() (SetupMode, *user.User, error) {
+	info1, err1 := os.Stat(m.PolicyDir())
+	info2, err2 := os.Stat(m.ProtectorDir())
+
+	if err1 == nil && err2 == nil {
+		mask := os.ModeSticky | 0777
+		mode1 := info1.Mode() & mask
+		mode2 := info2.Mode() & mask
+		uid1 := info1.Sys().(*syscall.Stat_t).Uid
+		uid2 := info2.Sys().(*syscall.Stat_t).Uid
+		user, err := util.UserFromUID(int64(uid1))
+		if err == nil && mode1 == mode2 && uid1 == uid2 {
+			switch mode1 {
+			case mask:
+				return WorldWritable, nil, nil
+			case 0755:
+				return SingleUserWritable, user, nil
+			}
+		}
+		log.Printf("filesystem %s uses custom permissions on metadata directories", m.Path)
+	}
+	return -1, nil, errors.New("unable to determine setup mode")
 }
 
 // Setup sets up the filesystem for use with fscrypt. Note that this merely
 // creates the appropriate files on the filesystem. It does not actually modify
 // the filesystem's feature flags. This operation is atomic; it either succeeds
 // or no files in the baseDir are created.
-func (m *Mount) Setup() error {
+func (m *Mount) Setup(mode SetupMode) error {
 	if m.CheckSetup() == nil {
 		return &ErrAlreadySetup{m}
 	}
@@ -404,7 +457,7 @@ func (m *Mount) Setup() error {
 	}
 	defer os.RemoveAll(temp.Path)
 
-	if err = temp.makeDirectories(); err != nil {
+	if err = temp.makeDirectories(mode); err != nil {
 		return err
 	}
 
@@ -484,6 +537,7 @@ func (m *Mount) writeData(path string, data []byte, owner *user.User) error {
 				log.Printf("trying non-atomic overwrite of %q", path)
 				return m.overwriteDataNonAtomic(path, data)
 			}
+			return &ErrNoCreatePermission{m}
 		}
 		return err
 	}
-- 
cgit v1.2.3


From 74e870b7bd1585b4b509da47e0e75db66336e576 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: Strictly validate metadata file ownership by default

The metadata validation checks introduced by the previous commits are
good, but to reduce the attack surface it would be much better to avoid
reading and parsing files owned by other users in the first place.

There are some possible use cases for users sharing fscrypt metadata
files, but I think that for the vast majority of users it is unneeded
and just opens up attack surface.  Thus, make fscrypt (and pam_fscrypt)
not process policies or protectors owned by other users by default.
Specifically,

   * If fscrypt or pam_fscrypt is running as a non-root user, only
     policies and protectors owned by the user or by root can be used.

   * If fscrypt is running as root, any policy or protector can be used.
     (This is to match user expectations -- starting a sudo session
     should gain rights, not remove rights.)

   * If pam_fscrypt is running as root, only policies and protectors
     owned by root can be used.  Note that this only applies when the
     root user themselves has an fscrypt login protector, which is rare.

Add an option 'allow_cross_user_metadata' to /etc/fscrypt.conf which
allows restoring the old behavior for anyone who really needs it.
---
 filesystem/filesystem.go | 107 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 29 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 1450f0f..6567dd6 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -370,6 +370,20 @@ func (m *Mount) CheckSupport() error {
 	return m.EncryptionSupportError(metadata.CheckSupport(m.Path))
 }
 
+func checkOwnership(path string, info os.FileInfo, trustedUser *user.User) bool {
+	if trustedUser == nil {
+		return true
+	}
+	trustedUID := uint32(util.AtoiOrPanic(trustedUser.Uid))
+	actualUID := info.Sys().(*syscall.Stat_t).Uid
+	if actualUID != 0 && actualUID != trustedUID {
+		log.Printf("WARNING: %q is owned by uid %d, but expected %d or 0",
+			path, actualUID, trustedUID)
+		return false
+	}
+	return true
+}
+
 // CheckSetup returns an error if all the fscrypt metadata directories do not
 // exist. Will log any unexpected errors or incorrect permissions.
 func (m *Mount) CheckSetup() error {
@@ -600,6 +614,8 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 //   point one to absolutely anywhere, and there is no known use case for the
 //   metadata files themselves being symlinks, it seems best to disallow them.)
 // - It must have a reasonable size (<= maxMetadataFileSize).
+// - If trustedUser is non-nil, then the file must be owned by the given user
+//   or by root.
 //
 // Take care to avoid TOCTOU (time-of-check-time-of-use) bugs when doing these
 // tests.  Notably, we must open the file before checking the file type, as the
@@ -611,7 +627,7 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 // This function returns the data read as well as the UID of the user who owns
 // the file.  The returned UID is needed for login protectors, where the UID
 // needs to be cross-checked with the UID stored in the file itself.
-func readMetadataFileSafe(path string) ([]byte, int64, error) {
+func readMetadataFileSafe(path string, trustedUser *user.User) ([]byte, int64, error) {
 	file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
 	if err != nil {
 		return nil, -1, err
@@ -625,6 +641,9 @@ func readMetadataFileSafe(path string) ([]byte, int64, error) {
 	if !info.Mode().IsRegular() {
 		return nil, -1, &ErrCorruptMetadata{path, errors.New("not a regular file")}
 	}
+	if !checkOwnership(path, info, trustedUser) {
+		return nil, -1, &ErrCorruptMetadata{path, errors.New("metadata file belongs to another user")}
+	}
 	// Clear O_NONBLOCK, since it has served its purpose when opening the
 	// file, and the behavior of reading from a regular file with O_NONBLOCK
 	// is technically unspecified.
@@ -645,8 +664,8 @@ func readMetadataFileSafe(path string) ([]byte, int64, error) {
 
 // getMetadata reads the metadata structure from the file with the specified
 // path. Only reads normal metadata files, not linked metadata.
-func (m *Mount) getMetadata(path string, md metadata.Metadata) (int64, error) {
-	data, owner, err := readMetadataFileSafe(path)
+func (m *Mount) getMetadata(path string, trustedUser *user.User, md metadata.Metadata) (int64, error) {
+	data, owner, err := readMetadataFileSafe(path, trustedUser)
 	if err != nil {
 		log.Printf("could not read metadata from %q: %v", path, err)
 		return -1, err
@@ -704,19 +723,19 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 // AddLinkedProtector adds a link in this filesystem to the protector metadata
 // in the dest filesystem, if one doesn't already exist.  On success, the return
 // value is a nil error and a bool that is true iff the link is newly created.
-func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) {
+func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User) (bool, error) {
 	if err := m.CheckSetup(); err != nil {
 		return false, err
 	}
 	// Check that the link is good (descriptor exists, filesystem has UUID).
-	if _, err := dest.GetRegularProtector(descriptor); err != nil {
+	if _, err := dest.GetRegularProtector(descriptor, trustedUser); err != nil {
 		return false, err
 	}
 
 	linkPath := m.linkedProtectorPath(descriptor)
 
 	// Check whether the link already exists.
-	existingLink, _, err := readMetadataFileSafe(linkPath)
+	existingLink, _, err := readMetadataFileSafe(linkPath, trustedUser)
 	if err == nil {
 		existingLinkedMnt, err := getMountFromLink(string(existingLink))
 		if err != nil {
@@ -742,13 +761,13 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
 // fail with ErrProtectorNotFound if the descriptor is a linked protector.
-func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, error) {
+func (m *Mount) GetRegularProtector(descriptor string, trustedUser *user.User) (*metadata.ProtectorData, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, err
 	}
 	data := new(metadata.ProtectorData)
 	path := m.protectorPath(descriptor)
-	owner, err := m.getMetadata(path, data)
+	owner, err := m.getMetadata(path, trustedUser, data)
 	if os.IsNotExist(err) {
 		err = &ErrProtectorNotFound{descriptor, m}
 	}
@@ -772,17 +791,17 @@ func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData,
 // GetProtector returns the Mount of the filesystem containing the information
 // and that protector's data. If the descriptor is a regular (not linked)
 // protector, the mount will return itself.
-func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData, error) {
+func (m *Mount) GetProtector(descriptor string, trustedUser *user.User) (*Mount, *metadata.ProtectorData, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, nil, err
 	}
 	// Get the link data from the link file
 	path := m.linkedProtectorPath(descriptor)
-	link, _, err := readMetadataFileSafe(path)
+	link, _, err := readMetadataFileSafe(path, trustedUser)
 	if err != nil {
 		// If the link doesn't exist, try for a regular protector.
 		if os.IsNotExist(err) {
-			data, err := m.GetRegularProtector(descriptor)
+			data, err := m.GetRegularProtector(descriptor, trustedUser)
 			return m, data, err
 		}
 		return nil, nil, err
@@ -792,7 +811,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData
 	if err != nil {
 		return nil, nil, errors.Wrap(err, path)
 	}
-	data, err := linkedMnt.GetRegularProtector(descriptor)
+	data, err := linkedMnt.GetRegularProtector(descriptor, trustedUser)
 	if err != nil {
 		return nil, nil, &ErrFollowLink{string(link), err}
 	}
@@ -818,12 +837,10 @@ func (m *Mount) RemoveProtector(descriptor string) error {
 }
 
 // ListProtectors lists the descriptors of all protectors on this filesystem.
-// This does not include linked protectors.
-func (m *Mount) ListProtectors() ([]string, error) {
-	if err := m.CheckSetup(); err != nil {
-		return nil, err
-	}
-	return m.listDirectory(m.ProtectorDir())
+// This does not include linked protectors.  If trustedUser is non-nil, then
+// the protectors are restricted to those owned by the given user or by root.
+func (m *Mount) ListProtectors(trustedUser *user.User) ([]string, error) {
+	return m.listMetadata(m.ProtectorDir(), "protectors", trustedUser)
 }
 
 // AddPolicy adds the policy metadata to the filesystem storage.
@@ -836,12 +853,12 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
 }
 
 // GetPolicy looks up the policy metadata by descriptor.
-func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) {
+func (m *Mount) GetPolicy(descriptor string, trustedUser *user.User) (*metadata.PolicyData, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, err
 	}
 	data := new(metadata.PolicyData)
-	_, err := m.getMetadata(m.PolicyPath(descriptor), data)
+	_, err := m.getMetadata(m.PolicyPath(descriptor), trustedUser, data)
 	if os.IsNotExist(err) {
 		err = &ErrPolicyNotFound{descriptor, m}
 	}
@@ -860,12 +877,11 @@ func (m *Mount) RemovePolicy(descriptor string) error {
 	return err
 }
 
-// ListPolicies lists the descriptors of all policies on this filesystem.
-func (m *Mount) ListPolicies() ([]string, error) {
-	if err := m.CheckSetup(); err != nil {
-		return nil, err
-	}
-	return m.listDirectory(m.PolicyDir())
+// ListPolicies lists the descriptors of all policies on this filesystem.  If
+// trustedUser is non-nil, then the policies are restricted to those owned by
+// the given user or by root.
+func (m *Mount) ListPolicies(trustedUser *user.User) ([]string, error) {
+	return m.listMetadata(m.PolicyDir(), "policies", trustedUser)
 }
 
 type namesAndTimes struct {
@@ -902,7 +918,6 @@ func sortFileListByLastMtime(directoryPath string, names []string) error {
 // listDirectory returns a list of descriptors for a metadata directory,
 // including files which are links to other filesystem's metadata.
 func (m *Mount) listDirectory(directoryPath string) ([]string, error) {
-	log.Printf("listing descriptors in %q", directoryPath)
 	dir, err := os.Open(directoryPath)
 	if err != nil {
 		return nil, err
@@ -925,7 +940,41 @@ func (m *Mount) listDirectory(directoryPath string) ([]string, error) {
 		// Be sure to include links as well
 		descriptors = append(descriptors, strings.TrimSuffix(name, linkFileExtension))
 	}
-
-	log.Printf("found %d descriptor(s)", len(descriptors))
 	return descriptors, nil
 }
+
+func (m *Mount) listMetadata(dirPath string, metadataType string, owner *user.User) ([]string, error) {
+	log.Printf("listing %s in %q", metadataType, dirPath)
+	if err := m.CheckSetup(); err != nil {
+		return nil, err
+	}
+	names, err := m.listDirectory(dirPath)
+	if err != nil {
+		return nil, err
+	}
+	filesIgnoredDescription := ""
+	if owner != nil {
+		filteredNames := make([]string, 0, len(names))
+		uid := uint32(util.AtoiOrPanic(owner.Uid))
+		for _, name := range names {
+			info, err := os.Lstat(filepath.Join(dirPath, name))
+			if err != nil {
+				continue
+			}
+			fileUID := info.Sys().(*syscall.Stat_t).Uid
+			if fileUID != uid && fileUID != 0 {
+				continue
+			}
+			filteredNames = append(filteredNames, name)
+		}
+		numIgnored := len(names) - len(filteredNames)
+		if numIgnored != 0 {
+			filesIgnoredDescription =
+				fmt.Sprintf(" (ignored %d %s not owned by %s or root)",
+					numIgnored, metadataType, owner.Username)
+		}
+		names = filteredNames
+	}
+	log.Printf("found %d %s%s", len(names), metadataType, filesIgnoredDescription)
+	return names, nil
+}
-- 
cgit v1.2.3


From 85a747493ff368a72f511619ecd391016ecb933c Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: Extend ownership validation to entire directory structure

A previous commit extended file ownership validation to policy and
protector files (by default -- there's an opt-out in /etc/fscrypt.conf).

However, that didn't apply to the parent directories:

	MOUNTPOINT
	MOUNTPOINT/.fscrypt
	MOUNTPOINT/.fscrypt/policies
	MOUNTPOINT/.fscrypt/protectors

The problem is that if the parent directories aren't trusted (owned by
another non-root user), then untrusted changes to their contents can be
made at any time, including the introduction of symlinks and so on.

While it's debatable how much of a problem this really is, given the
other validations that are done, it seems to be appropriate to validate
the parent directories too.

Therefore, this commit applies the same ownership validations to the
above four directories as are done on the metadata files themselves.

In addition, it is validated that none of these directories are symlinks
except for ".fscrypt" where this is explicitly supported.
---
 filesystem/filesystem.go | 127 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 107 insertions(+), 20 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 6567dd6..6e4f2c6 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -85,6 +85,17 @@ func (err *ErrFollowLink) Error() string {
 		err.Link, err.UnderlyingError)
 }
 
+// ErrInsecurePermissions indicates that a filesystem is not considered to be
+// setup for fscrypt because a metadata directory has insecure permissions.
+type ErrInsecurePermissions struct {
+	Path string
+}
+
+func (err *ErrInsecurePermissions) Error() string {
+	return fmt.Sprintf("%q has insecure permissions (world-writable without sticky bit)",
+		err.Path)
+}
+
 // ErrMakeLink indicates that a protector link can't be created.
 type ErrMakeLink struct {
 	Target          *Mount
@@ -96,6 +107,17 @@ func (err *ErrMakeLink) Error() string {
 		err.Target.Path, err.UnderlyingError)
 }
 
+// ErrMountOwnedByAnotherUser indicates that the mountpoint root directory is
+// owned by a user that isn't trusted in the current context, so we don't
+// consider fscrypt to be properly setup on the filesystem.
+type ErrMountOwnedByAnotherUser struct {
+	Mount *Mount
+}
+
+func (err *ErrMountOwnedByAnotherUser) Error() string {
+	return fmt.Sprintf("another non-root user owns the root directory of %s", err.Mount.Path)
+}
+
 // ErrNoCreatePermission indicates that the current user lacks permission to
 // create fscrypt metadata on the given filesystem.
 type ErrNoCreatePermission struct {
@@ -124,6 +146,17 @@ func (err *ErrNotSetup) Error() string {
 	return fmt.Sprintf("filesystem %s is not setup for use with fscrypt", err.Mount.Path)
 }
 
+// ErrSetupByAnotherUser indicates that one or more of the fscrypt metadata
+// directories is owned by a user that isn't trusted in the current context, so
+// we don't consider fscrypt to be properly setup on the filesystem.
+type ErrSetupByAnotherUser struct {
+	Mount *Mount
+}
+
+func (err *ErrSetupByAnotherUser) Error() string {
+	return fmt.Sprintf("another non-root user owns fscrypt metadata directories on %s", err.Mount.Path)
+}
+
 // ErrSetupNotSupported indicates that the given filesystem type is not
 // supported for fscrypt setup.
 type ErrSetupNotSupported struct {
@@ -384,21 +417,75 @@ func checkOwnership(path string, info os.FileInfo, trustedUser *user.User) bool
 	return true
 }
 
-// CheckSetup returns an error if all the fscrypt metadata directories do not
+// CheckSetup returns an error if any of the fscrypt metadata directories do not
 // exist. Will log any unexpected errors or incorrect permissions.
-func (m *Mount) CheckSetup() error {
+func (m *Mount) CheckSetup(trustedUser *user.User) error {
 	if !m.isFscryptSetupAllowed() {
 		return &ErrNotSetup{m}
 	}
-	// Run all the checks so we will always get all the warnings
-	baseGood := isDirCheckPerm(m.BaseDir(), basePermissions)
-	policyGood := isDir(m.PolicyDir())
-	protectorGood := isDir(m.ProtectorDir())
+	// Check that the mountpoint directory itself is not a symlink and has
+	// proper ownership, as otherwise we can't trust anything beneath it.
+	info, err := loggedLstat(m.Path)
+	if err != nil {
+		return &ErrNotSetup{m}
+	}
+	if (info.Mode() & os.ModeSymlink) != 0 {
+		log.Printf("mountpoint directory %q cannot be a symlink", m.Path)
+		return &ErrNotSetup{m}
+	}
+	if !info.IsDir() {
+		log.Printf("mountpoint %q is not a directory", m.Path)
+		return &ErrNotSetup{m}
+	}
+	if !checkOwnership(m.Path, info, trustedUser) {
+		return &ErrMountOwnedByAnotherUser{m}
+	}
+
+	// Check BaseDir similarly.  However, unlike the other directories, we
+	// allow BaseDir to be a symlink, to support the use case of metadata
+	// for a read-only filesystem being redirected to a writable location.
+	info, err = loggedStat(m.BaseDir())
+	if err != nil {
+		return &ErrNotSetup{m}
+	}
+	if !info.IsDir() {
+		log.Printf("%q is not a directory", m.BaseDir())
+		return &ErrNotSetup{m}
+	}
+	if !checkOwnership(m.Path, info, trustedUser) {
+		return &ErrMountOwnedByAnotherUser{m}
+	}
 
-	if baseGood && policyGood && protectorGood {
-		return nil
+	// Check that the policies and protectors directories aren't symlinks and
+	// have proper ownership.
+	subdirs := []string{m.PolicyDir(), m.ProtectorDir()}
+	for _, path := range subdirs {
+		info, err := loggedLstat(path)
+		if err != nil {
+			return &ErrNotSetup{m}
+		}
+		if (info.Mode() & os.ModeSymlink) != 0 {
+			log.Printf("directory %q cannot be a symlink", path)
+			return &ErrNotSetup{m}
+		}
+		if !info.IsDir() {
+			log.Printf("%q is not a directory", path)
+			return &ErrNotSetup{m}
+		}
+		// We are no longer too picky about the mode, given that
+		// 'fscrypt setup' now offers a choice of two different modes,
+		// and system administrators could customize it further.
+		// However, we can at least verify that if the directory is
+		// world-writable, then the sticky bit is also set.
+		if info.Mode()&(os.ModeSticky|0002) == 0002 {
+			log.Printf("%q is world-writable but doesn't have sticky bit set", path)
+			return &ErrInsecurePermissions{path}
+		}
+		if !checkOwnership(path, info, trustedUser) {
+			return &ErrSetupByAnotherUser{m}
+		}
 	}
-	return &ErrNotSetup{m}
+	return nil
 }
 
 // makeDirectories creates the three metadata directories with the correct
@@ -458,7 +545,7 @@ func (m *Mount) GetSetupMode() (SetupMode, *user.User, error) {
 // the filesystem's feature flags. This operation is atomic; it either succeeds
 // or no files in the baseDir are created.
 func (m *Mount) Setup(mode SetupMode) error {
-	if m.CheckSetup() == nil {
+	if m.CheckSetup(nil) == nil {
 		return &ErrAlreadySetup{m}
 	}
 	if !m.isFscryptSetupAllowed() {
@@ -485,7 +572,7 @@ func (m *Mount) Setup(mode SetupMode) error {
 // WARNING: Will cause data loss if the metadata is used to encrypt
 // directories (this could include directories on other filesystems).
 func (m *Mount) RemoveAllMetadata() error {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(nil); err != nil {
 		return err
 	}
 	// temp will hold the old metadata temporarily
@@ -701,7 +788,7 @@ func (m *Mount) removeMetadata(path string) error {
 // already exists on the filesystem.
 func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 	var err error
-	if err = m.CheckSetup(); err != nil {
+	if err = m.CheckSetup(nil); err != nil {
 		return err
 	}
 	if isRegularFile(m.linkedProtectorPath(data.ProtectorDescriptor)) {
@@ -724,7 +811,7 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 // in the dest filesystem, if one doesn't already exist.  On success, the return
 // value is a nil error and a bool that is true iff the link is newly created.
 func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User) (bool, error) {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(trustedUser); err != nil {
 		return false, err
 	}
 	// Check that the link is good (descriptor exists, filesystem has UUID).
@@ -762,7 +849,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *
 // GetRegularProtector looks up the protector metadata by descriptor. This will
 // fail with ErrProtectorNotFound if the descriptor is a linked protector.
 func (m *Mount) GetRegularProtector(descriptor string, trustedUser *user.User) (*metadata.ProtectorData, error) {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(trustedUser); err != nil {
 		return nil, err
 	}
 	data := new(metadata.ProtectorData)
@@ -792,7 +879,7 @@ func (m *Mount) GetRegularProtector(descriptor string, trustedUser *user.User) (
 // and that protector's data. If the descriptor is a regular (not linked)
 // protector, the mount will return itself.
 func (m *Mount) GetProtector(descriptor string, trustedUser *user.User) (*Mount, *metadata.ProtectorData, error) {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(trustedUser); err != nil {
 		return nil, nil, err
 	}
 	// Get the link data from the link file
@@ -821,7 +908,7 @@ func (m *Mount) GetProtector(descriptor string, trustedUser *user.User) (*Mount,
 // RemoveProtector deletes the protector metadata (or a link to another
 // filesystem's metadata) from the filesystem storage.
 func (m *Mount) RemoveProtector(descriptor string) error {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(nil); err != nil {
 		return err
 	}
 	// We first try to remove the linkedProtector. If that metadata does not
@@ -845,7 +932,7 @@ func (m *Mount) ListProtectors(trustedUser *user.User) ([]string, error) {
 
 // AddPolicy adds the policy metadata to the filesystem storage.
 func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(nil); err != nil {
 		return err
 	}
 
@@ -854,7 +941,7 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
 
 // GetPolicy looks up the policy metadata by descriptor.
 func (m *Mount) GetPolicy(descriptor string, trustedUser *user.User) (*metadata.PolicyData, error) {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(trustedUser); err != nil {
 		return nil, err
 	}
 	data := new(metadata.PolicyData)
@@ -867,7 +954,7 @@ func (m *Mount) GetPolicy(descriptor string, trustedUser *user.User) (*metadata.
 
 // RemovePolicy deletes the policy metadata from the filesystem storage.
 func (m *Mount) RemovePolicy(descriptor string) error {
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(nil); err != nil {
 		return err
 	}
 	err := m.removeMetadata(m.PolicyPath(descriptor))
@@ -945,7 +1032,7 @@ func (m *Mount) listDirectory(directoryPath string) ([]string, error) {
 
 func (m *Mount) listMetadata(dirPath string, metadataType string, owner *user.User) ([]string, error) {
 	log.Printf("listing %s in %q", metadataType, dirPath)
-	if err := m.CheckSetup(); err != nil {
+	if err := m.CheckSetup(owner); err != nil {
 		return nil, err
 	}
 	names, err := m.listDirectory(dirPath)
-- 
cgit v1.2.3


From d4ce0b892cbe68db9f90f4015342e6a9069b079c Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: Make all new metadata files owned by user when needed

Since commit 4c7c6631cc5a ("Set owner of login protectors to correct
user"), login protectors are made owned by the user when root creates
one on a user's behalf.  That's good, but the same isn't true of other
files that get created at the same time:

- The policy protecting the directory
- The protector link file, if the policy is on a different filesystem
- The recovery protector, if the policy is on a different filesystem
- The recovery instructions file

In preparation for setting all metadata files to mode 0600, start making
all these files owned by the user in this scenario as well.
---
 filesystem/filesystem.go | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 6e4f2c6..1877b1b 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -649,6 +649,8 @@ func (m *Mount) writeData(path string, data []byte, owner *user.User) error {
 		tempFile.Close()
 		return err
 	}
+	// Override the file owner if one was specified.  This happens when root
+	// needs to create files owned by a particular user.
 	if owner != nil {
 		if err = util.Chown(tempFile, owner); err != nil {
 			log.Printf("could not set owner of %q to %v: %v",
@@ -786,7 +788,7 @@ func (m *Mount) removeMetadata(path string) error {
 // will overwrite the value of an existing protector with this descriptor. This
 // will fail with ErrLinkedProtector if a linked protector with this descriptor
 // already exists on the filesystem.
-func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
+func (m *Mount) AddProtector(data *metadata.ProtectorData, owner *user.User) error {
 	var err error
 	if err = m.CheckSetup(nil); err != nil {
 		return err
@@ -796,21 +798,14 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 			data.ProtectorDescriptor, m.Path)
 	}
 	path := m.protectorPath(data.ProtectorDescriptor)
-
-	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
 // in the dest filesystem, if one doesn't already exist.  On success, the return
 // value is a nil error and a bool that is true iff the link is newly created.
-func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User) (bool, error) {
+func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User,
+	ownerIfCreating *user.User) (bool, error) {
 	if err := m.CheckSetup(trustedUser); err != nil {
 		return false, err
 	}
@@ -843,7 +838,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *
 	if err != nil {
 		return false, err
 	}
-	return true, m.writeData(linkPath, []byte(newLink), nil)
+	return true, m.writeData(linkPath, []byte(newLink), ownerIfCreating)
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
@@ -931,12 +926,12 @@ func (m *Mount) ListProtectors(trustedUser *user.User) ([]string, error) {
 }
 
 // AddPolicy adds the policy metadata to the filesystem storage.
-func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
+func (m *Mount) AddPolicy(data *metadata.PolicyData, owner *user.User) error {
 	if err := m.CheckSetup(nil); err != nil {
 		return err
 	}
 
-	return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data, nil)
+	return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data, owner)
 }
 
 // GetPolicy looks up the policy metadata by descriptor.
-- 
cgit v1.2.3


From 312bc381a3751e397995eeb2e63e66856912fafb Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: filesystem: preserve metadata file permissions on updates

Since fscrypt replaces metadata files rather than overwrites them (to
get atomicity), their owner will change to root if root makes a change.
That isn't too much of an issue when the files have mode 0644.  However,
it will become a much bigger issue when the files have mode 0600,
especially because existing files with mode 0644 would also get changed
to have mode 0600.

In preparation for this, start preserving the previous owner and mode of
policy and protector files when they are updated.
---
 filesystem/filesystem.go | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 1877b1b..70076b7 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -254,7 +254,7 @@ const (
 	basePermissions = 0755
 	// The metadata files are globally visible, but can only be deleted by
 	// the user that created them
-	filePermissions = 0644
+	filePermissions = os.FileMode(0644)
 
 	// Maximum size of a metadata file.  This value is arbitrary, and it can
 	// be changed.  We just set a reasonable limit that shouldn't be reached
@@ -626,7 +626,7 @@ func (m *Mount) overwriteDataNonAtomic(path string, data []byte) error {
 // However, if the process doesn't have write permission to the directory but
 // does have write permission to the file itself, then as a fallback the file is
 // overwritten in-place rather than replaced.  Note that this may be non-atomic.
-func (m *Mount) writeData(path string, data []byte, owner *user.User) error {
+func (m *Mount) writeData(path string, data []byte, owner *user.User, mode os.FileMode) 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)
@@ -644,8 +644,8 @@ func (m *Mount) writeData(path string, data []byte, owner *user.User) error {
 	}
 	defer os.Remove(tempFile.Name())
 
-	// TempFile() creates the file with mode 0600.  Change it to 0644.
-	if err = tempFile.Chmod(filePermissions); err != nil {
+	// Ensure the new file has the right permissions mask.
+	if err = tempFile.Chmod(mode); err != nil {
 		tempFile.Close()
 		return err
 	}
@@ -690,8 +690,29 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User)
 		return err
 	}
 
-	log.Printf("writing metadata to %q", path)
-	return m.writeData(path, data, owner)
+	mode := filePermissions
+	// If the file already exists, then preserve its owner and mode if
+	// possible.  This is necessary because by default, for atomicity
+	// reasons we'll replace the file rather than overwrite it.
+	info, err := os.Lstat(path)
+	if err == nil {
+		if owner == nil && util.IsUserRoot() {
+			uid := info.Sys().(*syscall.Stat_t).Uid
+			if owner, err = util.UserFromUID(int64(uid)); err != nil {
+				log.Print(err)
+			}
+		}
+		mode = info.Mode() & 0777
+	} else if !os.IsNotExist(err) {
+		log.Print(err)
+	}
+
+	if owner != nil {
+		log.Printf("writing metadata to %q and setting owner to %s", path, owner.Username)
+	} else {
+		log.Printf("writing metadata to %q", path)
+	}
+	return m.writeData(path, data, owner, mode)
 }
 
 // readMetadataFileSafe gets the contents of a metadata file extra-carefully,
@@ -838,7 +859,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *
 	if err != nil {
 		return false, err
 	}
-	return true, m.writeData(linkPath, []byte(newLink), ownerIfCreating)
+	return true, m.writeData(linkPath, []byte(newLink), ownerIfCreating, filePermissions)
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
-- 
cgit v1.2.3


From 06c989df4e31dd9f172f94fbd6243f49d4dd0b92 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: filesystem: create metadata files with mode 0600

Currently, fscrypt policies and protectors are world readable, as they
are created with mode 0644.  While this can be nice for use cases where
users share these files, those use cases seem to be quite rare, and it's
not a great default security-wise since it exposes password hashes to
all users.  While fscrypt uses a very strong password hash algorithm, it
would still be best to follow the lead of /etc/shadow and keep this
information non-world-readable.

Therefore, start creating these files with mode 0600.

Of course, if users do actually want to share these files, they have the
option of simply chmod'ing them to a less restrictive mode.  An option
could also be added to make fscrypt use the old mode 0644; however, the
need for that is currently unclear.
---
 filesystem/filesystem.go | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

(limited to 'filesystem/filesystem.go')

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 70076b7..27bfa24 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -252,9 +252,14 @@ const (
 
 	// The base directory should be read-only (except for the creator)
 	basePermissions = 0755
-	// The metadata files are globally visible, but can only be deleted by
-	// the user that created them
-	filePermissions = os.FileMode(0644)
+
+	// The metadata files shouldn't be readable or writable by other users.
+	// Having them be world-readable wouldn't necessarily be a huge issue,
+	// but given that some of these files contain (strong) password hashes,
+	// we error on the side of caution -- similar to /etc/shadow.
+	// Note: existing files on-disk might have mode 0644, as that was the
+	// mode used by fscrypt v0.3.2 and earlier.
+	filePermissions = os.FileMode(0600)
 
 	// Maximum size of a metadata file.  This value is arbitrary, and it can
 	// be changed.  We just set a reasonable limit that shouldn't be reached
-- 
cgit v1.2.3