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.

diff --git a/actions/policy.go b/actions/policy.go
index 3b8eb30c8e7e6b7325ee5c739ec214c00a6074a6..3b201769320614c5c83ef4fd1ae80ed67e5f85f5 100644 (file)
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -23,6 +23,7 @@ import (
        "fmt"
        "log"
        "os"
+       "os/user"
 
        "github.com/golang/protobuf/proto"
        "github.com/pkg/errors"
@@ -178,6 +179,7 @@ type Policy struct {
        data                *metadata.PolicyData
        key                 *crypto.Key
        created             bool
+       ownerIfCreating     *user.User
        newLinkedProtectors []string
 }
 
@@ -210,6 +212,12 @@ func CreatePolicy(ctx *Context, protector *Protector) (*Policy, error) {
                created: true,
        }
 
+       policy.ownerIfCreating, err = getOwnerOfMetadataForProtector(protector)
+       if err != nil {
+               policy.Lock()
+               return nil, err
+       }
+
        if err = policy.AddProtector(protector); err != nil {
                policy.Lock()
                return nil, err
@@ -410,6 +418,25 @@ func (policy *Policy) UsesProtector(protector *Protector) bool {
        return ok
 }
 
+// getOwnerOfMetadataForProtector returns the User to whom the owner of any new
+// policies or protector links for the given protector should be set.
+//
+// This will return a non-nil value only when the protector is a login protector
+// and the process is running as root.  In this scenario, root is setting up
+// encryption on the user's behalf, so we need to make new policies and
+// protector links owned by the user (rather than root) to allow them to be read
+// by the user, just like the login protector itself which is handled elsewhere.
+func getOwnerOfMetadataForProtector(protector *Protector) (*user.User, error) {
+       if protector.data.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() {
+               owner, err := util.UserFromUID(protector.data.Uid)
+               if err != nil {
+                       return nil, err
+               }
+               return owner, nil
+       }
+       return nil, nil
+}
+
 // AddProtector updates the data that is wrapping the Policy Key so that the
 // provided Protector is now protecting the specified Policy. If an error is
 // returned, no data has been changed. If the policy and protector are on
@@ -427,8 +454,13 @@ func (policy *Policy) AddProtector(protector *Protector) error {
        // to it on the policy's filesystem.
        if policy.Context.Mount != protector.Context.Mount {
                log.Printf("policy on %s\n protector on %s\n", policy.Context.Mount, protector.Context.Mount)
+               ownerIfCreating, err := getOwnerOfMetadataForProtector(protector)
+               if err != nil {
+                       return err
+               }
                isNewLink, err := policy.Context.Mount.AddLinkedProtector(
-                       protector.Descriptor(), protector.Context.Mount, protector.Context.TrustedUser)
+                       protector.Descriptor(), protector.Context.Mount,
+                       protector.Context.TrustedUser, ownerIfCreating)
                if err != nil {
                        return err
                }
@@ -554,7 +586,7 @@ func (policy *Policy) CanBeAppliedWithoutProvisioning() bool {
 
 // commitData writes the Policy's current data to the filesystem.
 func (policy *Policy) commitData() error {
-       return policy.Context.Mount.AddPolicy(policy.data)
+       return policy.Context.Mount.AddPolicy(policy.data, policy.ownerIfCreating)
 }
 
 // findWrappedPolicyKey returns the index of the wrapped policy key

diff --git a/actions/policy_test.go b/actions/policy_test.go
index 07943b88ad2eae91661e1d87a77b731e685cc10d..8248862ec0a02dbb8d0792443283fe299bdb42c4 100644 (file)
--- a/actions/policy_test.go
+++ b/actions/policy_test.go
@@ -27,7 +27,7 @@ import (
 
 // Makes a protector and policy
 func makeBoth() (*Protector, *Policy, error) {
-       protector, err := CreateProtector(testContext, testProtectorName, goodCallback)
+       protector, err := CreateProtector(testContext, testProtectorName, goodCallback, nil)
        if err != nil {
                return nil, nil, err
        }
@@ -68,7 +68,7 @@ func TestPolicyGoodAddProtector(t *testing.T) {
                t.Fatal(err)
        }
 
-       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
        if err != nil {
                t.Fatal(err)
        }
@@ -103,7 +103,7 @@ func TestPolicyGoodRemoveProtector(t *testing.T) {
                t.Fatal(err)
        }
 
-       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
        if err != nil {
                t.Fatal(err)
        }
@@ -129,7 +129,7 @@ func TestPolicyBadRemoveProtector(t *testing.T) {
                t.Fatal(err)
        }
 
-       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback)
+       pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil)
        if err != nil {
                t.Fatal(err)
        }

diff --git a/actions/protector.go b/actions/protector.go
index 1171c837b9e13759850e0030ad95feb812cf7c2b..b986eb020b7853ec291bb264197818d994746ea6 100644 (file)
--- a/actions/protector.go
+++ b/actions/protector.go
@@ -109,17 +109,18 @@ func checkIfUserHasLoginProtector(ctx *Context, uid int64) error {
 // to unlock policies and create new polices. As with the key struct, a
 // Protector should be wiped after use.
 type Protector struct {
-       Context *Context
-       data    *metadata.ProtectorData
-       key     *crypto.Key
-       created bool
+       Context         *Context
+       data            *metadata.ProtectorData
+       key             *crypto.Key
+       created         bool
+       ownerIfCreating *user.User
 }
 
 // CreateProtector creates an unlocked protector with a given name (name only
 // needed for custom and raw protector types). The keyFn provided to create the
 // Protector key will only be called once. If an error is returned, no data has
 // been changed on the filesystem.
-func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, error) {
+func CreateProtector(ctx *Context, name string, keyFn KeyFunc, owner *user.User) (*Protector, error) {
        if err := ctx.checkContext(); err != nil {
                return nil, err
        }
@@ -147,7 +148,8 @@ func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, erro
                        Name:   name,
                        Source: ctx.Config.Source,
                },
-               created: true,
+               created:         true,
+               ownerIfCreating: owner,
        }
 
        // Extra data is needed for some SourceTypes
@@ -294,5 +296,5 @@ func (protector *Protector) Rewrap(keyFn KeyFunc) error {
                return err
        }
 
-       return protector.Context.Mount.AddProtector(protector.data)
+       return protector.Context.Mount.AddProtector(protector.data, protector.ownerIfCreating)
 }

diff --git a/actions/protector_test.go b/actions/protector_test.go
index 6c81d499c7f40f7517fdbc87e386cf553f9286a2..f20dbcfba79b9d4297386c8e851c9401a7315aa6 100644 (file)
--- a/actions/protector_test.go
+++ b/actions/protector_test.go
@@ -43,7 +43,7 @@ func badCallback(info ProtectorInfo, retry bool) (*crypto.Key, error) {
 
 // Tests that we can create a valid protector.
 func TestCreateProtector(t *testing.T) {
-       p, err := CreateProtector(testContext, testProtectorName, goodCallback)
+       p, err := CreateProtector(testContext, testProtectorName, goodCallback, nil)
        if err != nil {
                t.Error(err)
        } else {
@@ -54,7 +54,7 @@ func TestCreateProtector(t *testing.T) {
 
 // Tests that a failure in the callback is relayed back to the caller.
 func TestBadCallback(t *testing.T) {
-       p, err := CreateProtector(testContext, testProtectorName, badCallback)
+       p, err := CreateProtector(testContext, testProtectorName, badCallback, nil)
        if err == nil {
                p.Lock()
                p.Destroy()

diff --git a/actions/recovery.go b/actions/recovery.go
index f5339062d10b03f8ca7aed52a5bf6bb1bb7c85df..8a769cc7ee6a119929ebe7b102fd8626a13ce79c 100644 (file)
--- a/actions/recovery.go
+++ b/actions/recovery.go
@@ -25,6 +25,7 @@ import (
 
        "github.com/google/fscrypt/crypto"
        "github.com/google/fscrypt/metadata"
+       "github.com/google/fscrypt/util"
 )
 
 // modifiedContextWithSource returns a copy of ctx with the protector source
@@ -66,7 +67,7 @@ func AddRecoveryPassphrase(policy *Policy, dirname string) (*crypto.Key, *Protec
                if seq != 1 {
                        name += " (" + strconv.Itoa(seq) + ")"
                }
-               recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn)
+               recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn, policy.ownerIfCreating)
                if err == nil {
                        break
                }
@@ -121,5 +122,10 @@ It is safe to keep it around though, as the recovery passphrase is high-entropy.
        if _, err = file.WriteString(str); err != nil {
                return err
        }
+       if recoveryProtector.ownerIfCreating != nil {
+               if err = util.Chown(file, recoveryProtector.ownerIfCreating); err != nil {
+                       return err
+               }
+       }
        return file.Sync()
 }

diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out
index b84216a1eb1bfd2d6239ff8afbf38d0df97edc1e..bb91a464202b8d64180e81a437ac292e2853b3b2 100644 (file)
--- a/cli-tests/t_encrypt_login.out
+++ b/cli-tests/t_encrypt_login.out
@@ -118,6 +118,8 @@ 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
+"MNT/dir" is now locked.
+"MNT/dir" is now locked.
 
 # 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 225a47d39ec9a1215a0ff771f195dcc5dafc05e7..b6ae2d8dbaace1b16ad6d2eecc5442d4aa8c7e2f 100755 (executable)
--- a/cli-tests/t_encrypt_login.sh
+++ b/cli-tests/t_encrypt_login.sh
@@ -58,9 +58,17 @@ 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.
+# This is partly redundant with the below check, but we might as well test both.
 login_protector=$(_get_login_descriptor)
 owner=$(stat -c "%U:%G" "$MNT_ROOT/.fscrypt/protectors/$login_protector")
 echo -e "\nProtector is owned by $owner"
+# The user should be able to lock and unlock the directory themselves.  This
+# tests that the fscrypt metadata file permissions got set appropriately when
+# root set up the encryption on the user's behalf.
+chown "$TEST_USER" "$dir"
+_user_do "fscrypt lock $dir"
+_user_do "echo TEST_USER_PASS | fscrypt unlock $dir --quiet --unlock-with=$MNT_ROOT:$login_protector"
+_user_do "fscrypt lock $dir"
 
 begin "Encrypt with login protector with --no-recovery"
 chown "$TEST_USER" "$dir"

diff --git a/cmd/fscrypt/protector.go b/cmd/fscrypt/protector.go
index ac864dd432ca8e2bb6798be61fe5360319a639c3..186ca7ac5a11e7931a209ea9061b021d389ecc33 100644 (file)
--- a/cmd/fscrypt/protector.go
+++ b/cmd/fscrypt/protector.go
@@ -23,6 +23,7 @@ package main
 import (
        "fmt"
        "log"
+       "os/user"
 
        "github.com/google/fscrypt/actions"
        "github.com/google/fscrypt/filesystem"
@@ -38,7 +39,6 @@ func createProtectorFromContext(ctx *actions.Context) (*actions.Protector, error
                return nil, err
        }
        log.Printf("using source: %s", ctx.Config.Source.String())
-
        if ctx.Config.Source == metadata.SourceType_pam_passphrase {
                if userFlag.Value == "" && util.IsUserRoot() {
                        return nil, ErrSpecifyUser
@@ -70,7 +70,11 @@ IMPORTANT: Before continuing, ensure you have properly set up your system for
                }
        }
 
-       return actions.CreateProtector(ctx, name, createKeyFn)
+       var owner *user.User
+       if ctx.Config.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() {
+               owner = ctx.TargetUser
+       }
+       return actions.CreateProtector(ctx, name, createKeyFn, owner)
 }
 
 // selectExistingProtector returns a locked Protector which corresponds to an

diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index 6e4f2c6f06043343021547ded303a767691d67f6..1877b1b2b5743088b72b0effc75b7e770aa7750a 100644 (file)
--- 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.

diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go
index 92e113b2e5ebe19be3a2f173fa7761ac94e3135b..f74078d6a21c4771789a975af249c2e41fc41b7a 100644 (file)
--- a/filesystem/filesystem_test.go
+++ b/filesystem/filesystem_test.go
@@ -253,31 +253,31 @@ func TestAddProtector(t *testing.T) {
        defer mnt.RemoveAllMetadata()
 
        protector := getFakeProtector()
-       if err = mnt.AddProtector(protector); err != nil {
+       if err = mnt.AddProtector(protector, nil); err != nil {
                t.Error(err)
        }
 
        // Change the source to bad one, or one that requires hashing costs
        protector.Source = metadata.SourceType_default
-       if mnt.AddProtector(protector) == nil {
+       if mnt.AddProtector(protector, nil) == nil {
                t.Error("bad source for a descriptor should make metadata invalid")
        }
        protector.Source = metadata.SourceType_custom_passphrase
-       if mnt.AddProtector(protector) == nil {
+       if mnt.AddProtector(protector, nil) == nil {
                t.Error("protectors using passphrases should require hashing costs")
        }
        protector.Source = metadata.SourceType_raw_key
 
        // Use a bad wrapped key
        protector.WrappedKey = wrappedPolicyKey
-       if mnt.AddProtector(protector) == nil {
+       if mnt.AddProtector(protector, nil) == nil {
                t.Error("bad length for protector keys should make metadata invalid")
        }
        protector.WrappedKey = wrappedProtectorKey
 
        // Change the descriptor (to a bad length)
        protector.ProtectorDescriptor = "abcde"
-       if mnt.AddProtector(protector) == nil {
+       if mnt.AddProtector(protector, nil) == nil {
                t.Error("bad descriptor length should make metadata invalid")
        }
 
@@ -292,32 +292,32 @@ func TestAddPolicy(t *testing.T) {
        defer mnt.RemoveAllMetadata()
 
        policy := getFakePolicy()
-       if err = mnt.AddPolicy(policy); err != nil {
+       if err = mnt.AddPolicy(policy, nil); err != nil {
                t.Error(err)
        }
 
        // Bad encryption options should make policy invalid
        policy.Options.Padding = 7
-       if mnt.AddPolicy(policy) == nil {
+       if mnt.AddPolicy(policy, nil) == nil {
                t.Error("padding not a power of 2 should make metadata invalid")
        }
        policy.Options.Padding = 16
        policy.Options.Filenames = metadata.EncryptionOptions_default
-       if mnt.AddPolicy(policy) == nil {
+       if mnt.AddPolicy(policy, nil) == nil {
                t.Error("encryption mode not set should make metadata invalid")
        }
        policy.Options.Filenames = metadata.EncryptionOptions_AES_256_CTS
 
        // Use a bad wrapped key
        policy.WrappedPolicyKeys[0].WrappedKey = wrappedProtectorKey
-       if mnt.AddPolicy(policy) == nil {
+       if mnt.AddPolicy(policy, nil) == nil {
                t.Error("bad length for policy keys should make metadata invalid")
        }
        policy.WrappedPolicyKeys[0].WrappedKey = wrappedPolicyKey
 
        // Change the descriptor (to a bad length)
        policy.KeyDescriptor = "abcde"
-       if mnt.AddPolicy(policy) == nil {
+       if mnt.AddPolicy(policy, nil) == nil {
                t.Error("bad descriptor length should make metadata invalid")
        }
 }
@@ -331,7 +331,7 @@ func TestSetPolicy(t *testing.T) {
        defer mnt.RemoveAllMetadata()
 
        policy := getFakePolicy()
-       if err = mnt.AddPolicy(policy); err != nil {
+       if err = mnt.AddPolicy(policy, nil); err != nil {
                t.Fatal(err)
        }
 
@@ -355,7 +355,7 @@ func TestSetProtector(t *testing.T) {
        defer mnt.RemoveAllMetadata()
 
        protector := getFakeProtector()
-       if err = mnt.AddProtector(protector); err != nil {
+       if err = mnt.AddProtector(protector, nil); err != nil {
                t.Fatal(err)
        }
 
@@ -383,7 +383,7 @@ func TestSpoofedLoginProtector(t *testing.T) {
 
        // Control case: protector with matching UID should be accepted.
        protector := getFakeLoginProtector(myUID)
-       if err = mnt.AddProtector(protector); err != nil {
+       if err = mnt.AddProtector(protector, nil); err != nil {
                t.Fatal(err)
        }
        _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil)
@@ -398,7 +398,7 @@ func TestSpoofedLoginProtector(t *testing.T) {
        // *unless* the process running the tests (and hence the file owner) is
        // root in which case it should be accepted.
        protector = getFakeLoginProtector(badUID)
-       if err = mnt.AddProtector(protector); err != nil {
+       if err = mnt.AddProtector(protector, nil); err != nil {
                t.Fatal(err)
        }
        _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil)
@@ -445,19 +445,19 @@ func TestLinkedProtector(t *testing.T) {
 
        // Add the protector to the first filesystem
        protector := getFakeProtector()
-       if err = realMnt.AddProtector(protector); err != nil {
+       if err = realMnt.AddProtector(protector, nil); err != nil {
                t.Fatal(err)
        }
 
        // Add the link to the second filesystem
        var isNewLink bool
-       if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil {
+       if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil, nil); err != nil {
                t.Fatal(err)
        }
        if !isNewLink {
                t.Fatal("Link was not new")
        }
-       if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil {
+       if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil, nil); err != nil {
                t.Fatal(err)
        }
        if isNewLink {