Merge pull request #192 from ebiggers/cleanup-on-error

Clean up policies and protectors on error

5 files changed, 81 insertions, 31 deletions

diff --git a/actions/policy.go b/actions/policy.go
index 41e108e..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)
 	}
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
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.
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)