From d4ce0b892cbe68db9f90f4015342e6a9069b079c Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: [PATCH] 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. --- actions/policy.go | 36 +++++++++++++++++++++++++++++++++-- actions/policy_test.go | 8 ++++---- actions/protector.go | 16 +++++++++------- actions/protector_test.go | 4 ++-- actions/recovery.go | 8 +++++++- cli-tests/t_encrypt_login.out | 2 ++ cli-tests/t_encrypt_login.sh | 8 ++++++++ cmd/fscrypt/protector.go | 8 ++++++-- filesystem/filesystem.go | 21 ++++++++------------ filesystem/filesystem_test.go | 34 ++++++++++++++++----------------- 10 files changed, 97 insertions(+), 48 deletions(-) diff --git a/actions/policy.go b/actions/policy.go index 3b8eb30..3b20176 100644 --- 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 07943b8..8248862 100644 --- 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 1171c83..b986eb0 100644 --- 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 6c81d49..f20dbcf 100644 --- 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 f533906..8a769cc 100644 --- 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 b84216a..bb91a46 100644 --- 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 225a47d..b6ae2d8 100755 --- 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 ac864dd..186ca7a 100644 --- 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 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. diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 92e113b..f74078d 100644 --- 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 { -- 2.39.5