Merge pull request #217 from ebiggers/detect-incomplete-v1-locking

Try to detect incomplete locking of v1-encrypted directory

7 files changed, 117 insertions, 32 deletions

diff --git a/actions/policy.go b/actions/policy.go
index 3baad72..6c2aa51 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -417,12 +417,6 @@ func (policy *Policy) IsProvisionedByTargetUser() bool {
 	return policy.GetProvisioningStatus() == keyring.KeyPresent
 }
 
-// IsFullyDeprovisioned returns true if the policy has been fully deprovisioned,
-// including by all users and with all files protected by it having been closed.
-func (policy *Policy) IsFullyDeprovisioned() bool {
-	return policy.GetProvisioningStatus() == keyring.KeyAbsent
-}
-
 // Provision inserts the Policy key into the kernel keyring. This allows reading
 // and writing of files encrypted with this directory. Requires unlocked Policy.
 func (policy *Policy) Provision() error {
@@ -435,14 +429,15 @@ func (policy *Policy) Provision() error {
 
 // Deprovision removes the Policy key from the kernel keyring. This prevents
 // reading and writing to the directory --- unless the target keyring is a user
-// keyring, in which case caches must be dropped too.
+// keyring, in which case caches must be dropped too. If the Policy key was
+// already removed, returns keyring.ErrKeyNotPresent.
 func (policy *Policy) Deprovision(allUsers bool) error {
 	return keyring.RemoveEncryptionKey(policy.Descriptor(),
 		policy.Context.getKeyringOptions(), allUsers)
 }
 
 // NeedsUserKeyring returns true if Provision and Deprovision for this policy
-// will use a user keyring, not a filesystem keyring.
+// will use a user keyring (deprecated), not a filesystem keyring.
 func (policy *Policy) NeedsUserKeyring() bool {
 	return policy.Version() == 1 && !policy.Context.Config.GetUseFsKeyringForV1Policies()
 }
diff --git a/cli-tests/t_v1_policy.out b/cli-tests/t_v1_policy.out
index 747cf81..0ff5219 100644
--- a/cli-tests/t_v1_policy.out
+++ b/cli-tests/t_v1_policy.out
@@ -96,3 +96,42 @@ Protected with 1 protector:
 PROTECTOR         LINKED  DESCRIPTION
 desc2  No      custom protector "prot"
 cat: MNT/dir/file: No such file or directory
+
+# Testing incompletely locking v1-encrypted directory
+Enter custom passphrase for protector "prot": "MNT/dir" is now unlocked and ready for use.
+Encrypted data removed from filesystem cache.
+[ERROR] fscrypt lock: some files using the key are still open
+
+Directory was incompletely locked because some files are still open. These files
+remain accessible. Try killing any processes using files in the directory, then
+re-running 'fscrypt lock'.
+"MNT/dir" is encrypted with fscrypt.
+
+Policy:   desc1
+Options:  padding:32 contents:AES_256_XTS filenames:AES_256_CTS policy_version:1 
+Unlocked: Partially (incompletely locked)
+
+Protected with 1 protector:
+PROTECTOR         LINKED  DESCRIPTION
+desc2  No      custom protector "prot"
+ext4 filesystem "MNT" has 1 protector and 1 policy
+
+PROTECTOR         LINKED  DESCRIPTION
+desc2  No      custom protector "prot"
+
+POLICY            UNLOCKED  PROTECTORS
+desc1  No        desc2
+
+# Finishing locking v1-encrypted directory
+Encrypted data removed from filesystem cache.
+"MNT/dir" is now locked.
+"MNT/dir" is encrypted with fscrypt.
+
+Policy:   desc1
+Options:  padding:32 contents:AES_256_XTS filenames:AES_256_CTS policy_version:1 
+Unlocked: No
+
+Protected with 1 protector:
+PROTECTOR         LINKED  DESCRIPTION
+desc2  No      custom protector "prot"
+cat: MNT/dir/file: No such file or directory
diff --git a/cli-tests/t_v1_policy.sh b/cli-tests/t_v1_policy.sh
index 1ebfae5..e9f3acf 100755
--- a/cli-tests/t_v1_policy.sh
+++ b/cli-tests/t_v1_policy.sh
@@ -54,3 +54,18 @@ _print_header "Lock v1-encrypted directory"
 fscrypt lock "$dir" --user="$TEST_USER"
 _user_do "fscrypt status '$dir'"
 _expect_failure "cat '$dir/file'"
+
+# 'fscrypt lock' and 'fscrypt status' implement a heuristic that should detect
+# the "files busy" case with v1.
+_print_header "Testing incompletely locking v1-encrypted directory"
+_user_do "echo hunter2 | fscrypt unlock '$dir'"
+exec 3<"$dir/file"
+_expect_failure "fscrypt lock '$dir' --user='$TEST_USER'"
+_user_do "fscrypt status '$dir'"
+# ... except in this case, because we can't detect it without a directory path.
+_user_do "fscrypt status '$MNT'"
+exec 3<&-
+_print_header "Finishing locking v1-encrypted directory"
+fscrypt lock "$dir" --user="$TEST_USER"
+_user_do "fscrypt status '$dir'"
+_expect_failure "cat '$dir/file'"
diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index ec75584..51cf136 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -496,30 +496,55 @@ func lockAction(c *cli.Context) error {
 	if err = validateKeyringPrereqs(ctx, policy); err != nil {
 		return newExitError(c, err)
 	}
-	// Check if directory is already locked
-	if policy.IsFullyDeprovisioned() {
-		log.Printf("policy %s is already fully deprovisioned", policy.Descriptor())
-		return newExitError(c, errors.Wrapf(ErrPolicyLocked, path))
-	}
-	// Check for permission to drop caches, if it will be needed.
+	// Check for permission to drop caches, if it may be needed.
 	if policy.NeedsUserKeyring() && dropCachesFlag.Value && !util.IsUserRoot() {
 		return newExitError(c, ErrDropCachesPerm)
 	}
 
 	if err = policy.Deprovision(allUsersFlag.Value); err != nil {
-		return newExitError(c, err)
+		if err != keyring.ErrKeyNotPresent {
+			return newExitError(c, err)
+		}
+		// Key is no longer present.  Normally that means the directory
+		// is already locked; in that case we exit with an error.  But
+		// if the policy uses the user keyring (v1 policies only), then
+		// the directory might have been incompletely locked earlier,
+		// due to open files.  Try to detect that case and finish
+		// locking the directory by dropping caches again.
+		if !policy.NeedsUserKeyring() || !isDirUnlockedHeuristic(path) {
+			log.Printf("policy %s is already fully deprovisioned", policy.Descriptor())
+			return newExitError(c, errors.Wrapf(ErrPolicyLocked, path))
+		}
 	}
 
 	if policy.NeedsUserKeyring() {
 		if err = dropCachesIfRequested(c, ctx); err != nil {
 			return newExitError(c, err)
 		}
+		if isDirUnlockedHeuristic(path) {
+			return newExitError(c, keyring.ErrKeyFilesOpen)
+		}
 	}
 
 	fmt.Fprintf(c.App.Writer, "%q is now locked.\n", path)
 	return nil
 }
 
+// isDirUnlockedHeuristic returns true if we can create a subdirectory of the
+// given directory and therefore it is definitely still unlocked.  It returns
+// false if the directory is probably locked (though it could also be unlocked).
+//
+// This is only useful if the directory's policy uses the user keyring, since
+// otherwise the status can be easily found via the filesystem keyring.
+func isDirUnlockedHeuristic(dirPath string) bool {
+	subdirPath := filepath.Join(dirPath, "fscrypt-is-dir-unlocked")
+	if err := os.Mkdir(subdirPath, 0700); err == nil {
+		os.Remove(subdirPath)
+		return true
+	}
+	return false
+}
+
 // Purge removes all the policy keys from the keyring (also need unmount).
 var Purge = cli.Command{
 	Name:      "purge",
diff --git a/cmd/fscrypt/status.go b/cmd/fscrypt/status.go
index bf11495..40bb49e 100644
--- a/cmd/fscrypt/status.go
+++ b/cmd/fscrypt/status.go
@@ -66,8 +66,20 @@ func yesNoString(b bool) string {
 	return "No"
 }
 
-func policyUnlockedStatus(policy *actions.Policy) string {
-	switch policy.GetProvisioningStatus() {
+func policyUnlockedStatus(policy *actions.Policy, path string) string {
+	status := policy.GetProvisioningStatus()
+
+	// Due to a limitation in the old kernel API for fscrypt, for v1
+	// policies using the user keyring that are incompletely locked we'll
+	// get KeyAbsent, not KeyAbsentButFilesBusy as expected.  If we have a
+	// directory path, use a heuristic to try to detect whether it is still
+	// usable and thus the policy is actually incompletely locked.
+	if status == keyring.KeyAbsent && policy.NeedsUserKeyring() &&
+		path != "" && isDirUnlockedHeuristic(path) {
+		status = keyring.KeyAbsentButFilesBusy
+	}
+
+	switch status {
 	case keyring.KeyPresent, keyring.KeyPresentButOnlyOtherUsers:
 		return "Yes"
 	case keyring.KeyAbsent:
@@ -174,7 +186,8 @@ func writeFilesystemStatus(w io.Writer, ctx *actions.Context) error {
 			continue
 		}
 
-		fmt.Fprintf(t, "%s\t%s\t%s\n", descriptor, policyUnlockedStatus(policy),
+		fmt.Fprintf(t, "%s\t%s\t%s\n", descriptor,
+			policyUnlockedStatus(policy, ""),
 			strings.Join(policy.ProtectorDescriptors(), ", "))
 	}
 	return t.Flush()
@@ -194,7 +207,7 @@ func writePathStatus(w io.Writer, path string) error {
 	fmt.Fprintln(w)
 	fmt.Fprintf(w, "Policy:   %s\n", policy.Descriptor())
 	fmt.Fprintf(w, "Options:  %s\n", policy.Options())
-	fmt.Fprintf(w, "Unlocked: %s\n", policyUnlockedStatus(policy))
+	fmt.Fprintf(w, "Unlocked: %s\n", policyUnlockedStatus(policy, path))
 	fmt.Fprintln(w)
 
 	options := policy.ProtectorOptions()
diff --git a/keyring/keyring.go b/keyring/keyring.go
index 6623943..fb9cc0e 100644
--- a/keyring/keyring.go
+++ b/keyring/keyring.go
@@ -173,7 +173,7 @@ func GetEncryptionKeyStatus(descriptor string, options *Options) (KeyStatus, err
 	if useFsKeyring {
 		return fsGetEncryptionKeyStatus(descriptor, options.Mount, options.User)
 	}
-	_, err = userFindKey(buildKeyDescription(options, descriptor), options.User)
+	_, _, err = userFindKey(buildKeyDescription(options, descriptor), options.User)
 	if err != nil {
 		return KeyAbsent, nil
 	}
diff --git a/keyring/user_keyring.go b/keyring/user_keyring.go
index 71f519d..beeb36d 100644
--- a/keyring/user_keyring.go
+++ b/keyring/user_keyring.go
@@ -78,39 +78,37 @@ func userRemoveKey(description string, targetUser *user.User) error {
 	runtime.LockOSThread() // ensure target user keyring remains possessed in thread keyring
 	defer runtime.UnlockOSThread()
 
-	keyID, err := userFindKey(description, targetUser)
+	keyID, keyringID, err := userFindKey(description, targetUser)
 	if err != nil {
 		return ErrKeyNotPresent
 	}
 
-	// We use KEYCTL_INVALIDATE instead of KEYCTL_REVOKE because
-	// invalidating a key immediately removes it.
-	_, err = unix.KeyctlInt(unix.KEYCTL_INVALIDATE, keyID, 0, 0, 0)
-	log.Printf("KeyctlInvalidate(%d) = %v", keyID, err)
+	_, err = unix.KeyctlInt(unix.KEYCTL_UNLINK, keyID, keyringID, 0, 0)
+	log.Printf("KeyctlUnlink(%d, %d) = %v", keyID, keyringID, err)
 	if err != nil {
 		return errors.Wrap(ErrKeyRemove, err.Error())
 	}
 	return nil
 }
 
-// userFindKey tries to locate a key in the user keyring with the provided
-// description. The key ID is returned if we can find the key. An error is
-// returned if the key does not exist.
-func userFindKey(description string, targetUser *user.User) (int, error) {
+// userFindKey tries to locate a key with the provided description in the user
+// keyring for the target user. The key ID and keyring ID are returned if we can
+// find the key. An error is returned if the key does not exist.
+func userFindKey(description string, targetUser *user.User) (int, int, error) {
 	runtime.LockOSThread() // ensure target user keyring remains possessed in thread keyring
 	defer runtime.UnlockOSThread()
 
 	keyringID, err := UserKeyringID(targetUser, false)
 	if err != nil {
-		return 0, err
+		return 0, 0, err
 	}
 
 	keyID, err := unix.KeyctlSearch(keyringID, KeyType, description, 0)
 	log.Printf("KeyctlSearch(%d, %s, %s) = %d, %v", keyringID, KeyType, description, keyID, err)
 	if err != nil {
-		return 0, errors.Wrap(ErrKeySearch, err.Error())
+		return 0, 0, errors.Wrap(ErrKeySearch, err.Error())
 	}
-	return keyID, err
+	return keyID, keyringID, err
 }
 
 // UserKeyringID returns the key id of the target user's user keyring. We also