From 4e0230bdbc9cf893099919170a10e44f84422747 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 27 Jan 2020 20:16:35 -0800 Subject: actions/recovery: revert protector if it can't be added to policy Ensure that a failed AddRecoveryPassphrase() doesn't leave around an unneeded protector file. --- actions/recovery.go | 1 + 1 file changed, 1 insertion(+) diff --git a/actions/recovery.go b/actions/recovery.go index 32d0030..1c55ec5 100644 --- a/actions/recovery.go +++ b/actions/recovery.go @@ -78,6 +78,7 @@ func AddRecoveryPassphrase(policy *Policy, dirname string) (*crypto.Key, *Protec seq++ } if err := policy.AddProtector(recoveryProtector); err != nil { + recoveryProtector.Revert() return nil, nil, err } return passphrase, recoveryProtector, nil -- cgit v1.2.3 From 5c08edd521deadd36bec36662d30681b01253d62 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 27 Jan 2020 20:16:35 -0800 Subject: cmd/fscrypt/commands: clean up properly when encryptPath() fails Move the deferred locking and deletion of the policy on failure to the correct places, so that it's done in all failure cases, including in the case where adding the recovery protector fails. Also make the recovery protector be locked and deleted on failure. Finally, put all the code to do deferred deprovisioning of the policy in the same place: right after it's provisioned. --- cmd/fscrypt/commands.go | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go index 65e0f45..e807d46 100644 --- a/cmd/fscrypt/commands.go +++ b/cmd/fscrypt/commands.go @@ -197,6 +197,7 @@ func encryptPath(path string) (err error) { if policy, err = getPolicyFromFlag(policyFlag.Value, ctx.TargetUser); err != nil { return } + defer policy.Lock() if !skipUnlockFlag.Value { if err = validateKeyringPrereqs(ctx, policy); err != nil { @@ -213,12 +214,12 @@ func encryptPath(path string) (err error) { } protector, created, protErr := selectOrCreateProtector(ctx) - // Successfully created protector should be reverted on failure. if protErr != nil { return protErr } defer func() { protector.Lock() + // Successfully created protector should be reverted on failure. if err != nil && created { protector.Revert() } @@ -230,6 +231,14 @@ func encryptPath(path string) (err error) { if policy, err = actions.CreatePolicy(ctx, protector); err != nil { return } + defer func() { + policy.Lock() + // Successfully created policy should be reverted on failure. + if err != nil { + policy.Revert() + } + }() + // Automatically generate a recovery passphrase if the protector // is on a different filesystem from the policy. In practice, // this happens for login passphrase-protected directories that @@ -237,21 +246,21 @@ func encryptPath(path string) (err error) { // always stored on the root filesystem. if ctx.Mount != protector.Context.Mount { fmt.Printf("Generating recovery passphrase because protector is on a different filesystem.\n") - if recoveryPassphrase, _, err = actions.AddRecoveryPassphrase( + var recoveryProtector *actions.Protector + if recoveryPassphrase, recoveryProtector, err = actions.AddRecoveryPassphrase( policy, filepath.Base(path)); err != nil { return } - defer recoveryPassphrase.Wipe() + defer func() { + recoveryPassphrase.Wipe() + recoveryProtector.Lock() + // Successfully created protector should be reverted on failure. + if err != nil { + recoveryProtector.Revert() + } + }() } } - // Successfully created policy should be reverted on failure. - defer func() { - policy.Lock() - if err != nil { - policy.Deprovision(false) - policy.Revert() - } - }() // Unlock() and Provision() first, so if that if these fail the // directory isn't changed, and also because v2 policies can't be @@ -263,9 +272,11 @@ func encryptPath(path string) (err error) { if err = policy.Provision(); err != nil { return } - if skipUnlockFlag.Value { - defer policy.Deprovision(false) - } + defer func() { + if err != nil || skipUnlockFlag.Value { + policy.Deprovision(false) + } + }() } if err = policy.Apply(path); os.IsPermission(errors.Cause(err)) { // EACCES at this point indicates ownership issues. -- cgit v1.2.3 From 07d744068d437b09d7a07975e88e18440f5db2f3 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 27 Jan 2020 20:16:35 -0800 Subject: filesystem: don't overwrite existing protector links When adding a protector to a policy, don't unconditionally overwrite the protector link, because it may already exist. Instead, if it already exists and points to the mount, just use it. If it already exists and points to the wrong place, return an error. Also add a bool to the return value of AddLinkedProtector() so that callers can check whether the link was newly created or not. --- actions/policy.go | 2 +- filesystem/filesystem.go | 37 ++++++++++++++++++++++++++++--------- filesystem/filesystem_test.go | 12 +++++++++++- 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/actions/policy.go b/actions/policy.go index 41e108e..9d644c1 100644 --- a/actions/policy.go +++ b/actions/policy.go @@ -315,7 +315,7 @@ 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) - err := policy.Context.Mount.AddLinkedProtector( + _, err := policy.Context.Mount.AddLinkedProtector( protector.Descriptor(), protector.Context.Mount) if err != nil { return err diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 9bae72b..e0ef110 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -393,24 +393,43 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error { } // AddLinkedProtector adds a link in this filesystem to the protector metadata -// in the dest filesystem. -func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) 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) (bool, error) { if err := m.CheckSetup(); err != nil { - return err + return false, err } // Check that the link is good (descriptor exists, filesystem has UUID). if _, err := dest.GetRegularProtector(descriptor); err != nil { - return err + return false, err + } + + linkPath := m.linkedProtectorPath(descriptor) + + // Check whether the link already exists. + existingLink, err := ioutil.ReadFile(linkPath) + if err == nil { + existingLinkedMnt, err := getMountFromLink(string(existingLink)) + if err != nil { + return false, err + } + if existingLinkedMnt != dest { + return false, errors.Wrapf(ErrFollowLink, "link %q points to %q, but expected %q", + linkPath, existingLinkedMnt.Path, dest.Path) + } + return false, nil + } + if !os.IsNotExist(err) { + return false, err } // Right now, we only make links using UUIDs. - link, err := makeLink(dest, "UUID") + var newLink string + newLink, err = makeLink(dest, "UUID") if err != nil { - return dest.err(err) + return false, dest.err(err) } - - path := m.linkedProtectorPath(descriptor) - return m.err(m.writeDataAtomic(path, []byte(link))) + return true, m.err(m.writeDataAtomic(linkPath, []byte(newLink))) } // GetRegularProtector looks up the protector metadata by descriptor. This will diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index b85ead5..4bed96a 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -351,9 +351,19 @@ func TestLinkedProtector(t *testing.T) { } // Add the link to the second filesystem - if err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt); err != nil { + var isNewLink bool + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt); err != nil { t.Fatal(err) } + if !isNewLink { + t.Fatal("Link was not new") + } + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt); err != nil { + t.Fatal(err) + } + if isNewLink { + t.Fatal("Link was new") + } // Get the protector though the second system _, err = fakeMnt.GetRegularProtector(protector.ProtectorDescriptor) -- cgit v1.2.3 From 2d7229eb2a97c845d73a65ff9dd3368056c255a6 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 27 Jan 2020 20:16:35 -0800 Subject: actions/policy: revert new protector links on failure Ensure that when an encryption policy is reverted (e.g. due to encryptPath() failing after the policy was created), we also delete any new protector links that were created for the policy, as this is not handled by the logic that reverts new protectors. --- actions/policy.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/actions/policy.go b/actions/policy.go index 9d644c1..b7fe5a6 100644 --- a/actions/policy.go +++ b/actions/policy.go @@ -79,10 +79,11 @@ func PurgeAllPolicies(ctx *Context) error { // allow encrypted files to be accessed). As with the key struct, a Policy // should be wiped after use. type Policy struct { - Context *Context - data *metadata.PolicyData - key *crypto.Key - created bool + Context *Context + data *metadata.PolicyData + key *crypto.Key + created bool + newLinkedProtectors []string } // CreatePolicy creates a Policy protected by given Protector and stores the @@ -208,9 +209,13 @@ func (policy *Policy) Version() int64 { return policy.data.Options.PolicyVersion } -// Destroy removes a policy from the filesystem. The internal key should still -// be wiped with Lock(). +// Destroy removes a policy from the filesystem. It also removes any new +// protector links that were created for the policy. This does *not* wipe the +// policy's internal key from memory; use Lock() to do that. func (policy *Policy) Destroy() error { + for _, protectorDescriptor := range policy.newLinkedProtectors { + policy.Context.Mount.RemoveProtector(protectorDescriptor) + } return policy.Context.Mount.RemovePolicy(policy.Descriptor()) } @@ -315,11 +320,15 @@ 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) - _, err := policy.Context.Mount.AddLinkedProtector( + isNewLink, err := policy.Context.Mount.AddLinkedProtector( protector.Descriptor(), protector.Context.Mount) if err != nil { return err } + if isNewLink { + policy.newLinkedProtectors = append(policy.newLinkedProtectors, + protector.Descriptor()) + } } else { log.Printf("policy and protector both on %q", policy.Context.Mount) } -- cgit v1.2.3