Merge lp:~tyhicks/ecryptfs/lp1454388-lp1454319 into lp:ecryptfs

[Tyler Hicks]

My main intent was to fix the "Sessions still open, not unmounting" error message that umount.ecryptfs_private always prints when attempting to unmount an in-use encrypted private dir. While fixing that, I discovered some other bugs and usability issues.

[Tyler Hicks (tyhicks)]

With this change, there are now two modes for umount.ecryptfs_private:

1) By default, decrement the counter and if the counter is non-zero, print a warning and exit with a failure code. If the counter is zero, do the unmount.

2) If the -d option is given, decrement the counter and if the counter is non-zero, do nothing and exit with a success code. If the counter is zero, do the unmount.

The pam_ecryptfs module would use mode #2.

[Tyler Hicks (tyhicks)]

Dustin gave me an ack through IRC:

  16:29 < kirkland> tyhicks: I'm fine with the MP, I'll pull that in now

=== modified file 'debian/changelog'
--- debian/changelog	2015-03-27 22:53:20 +0000
+++ debian/changelog	2015-05-12 23:17:46 +0000
@@ -1,7 +1,24 @@
 ecryptfs-utils (108) UNRELEASED; urgency=medium
 
+  [ Dustin Kirkland ]
   * UNRELEASED
 
+  [ Tyler Hicks ]
+  * src/utils/mount.ecryptfs_private.c: Implement proper option parsing to
+    restore the -f option when unmounting and display a helpful usage message
+    (LP: #1454388)
+  * src/utils/mount.ecryptfs_private.c: Add an option, -d, to
+    umount.ecryptfs_private to treat the situation where the encrypted private
+    session counter is nonzero, after decrementing it, as a non-error
+    situation. No error message is printed to stderr and the exit status is 0.
+  * src/pam_ecryptfs/pam_ecryptfs.c: Use the new umount.ecryptfs_private '-d'
+    option to silence the error message that was printed to stderr when the
+    encrypted private session counter is nonzero after being decremented.
+    (LP: #1454319)
+  * src/utils/ecryptfs-umount-private: Return 1 if umount.ecryptfs_private
+    encounters an error. The ecryptfs-umount-private script was previously
+    returning 0 even when umount.ecryptfs_private exited upon error.
+
  -- Dustin Kirkland <[e-mail]>  Fri, 27 Mar 2015 17:53:18 -0500
 
 ecryptfs-utils (107) utopic; urgency=medium

=== modified file 'src/pam_ecryptfs/pam_ecryptfs.c'
--- src/pam_ecryptfs/pam_ecryptfs.c	2015-02-14 22:11:29 +0000
+++ src/pam_ecryptfs/pam_ecryptfs.c	2015-05-12 23:17:46 +0000
@@ -387,7 +387,7 @@
 			if (setresuid(pwd->pw_uid, pwd->pw_uid, pwd->pw_uid) < 0)
 				exit(-1);
 			execl("/sbin/umount.ecryptfs_private",
- 			      "umount.ecryptfs_private", NULL);
+			      "umount.ecryptfs_private", "-d", NULL);
 			exit(1);
 		}
 		exit(1);

=== modified file 'src/utils/ecryptfs-umount-private'
--- src/utils/ecryptfs-umount-private	2011-12-13 20:51:40 +0000
+++ src/utils/ecryptfs-umount-private	2015-05-12 23:17:46 +0000
@@ -23,4 +23,6 @@
 		echo "  cd $PWD"
 		echo
 	fi
+	exit 0
 fi
+exit 1

=== modified file 'src/utils/mount.ecryptfs_private.c'
--- src/utils/mount.ecryptfs_private.c	2013-10-27 22:34:10 +0000
+++ src/utils/mount.ecryptfs_private.c	2015-05-12 23:17:46 +0000
@@ -472,6 +472,67 @@
 	return bump_counter(fh, -MAXINT+1);
 }
 
+/* Returns -1 on error, 0 on success, and 1 if the program should exit with 0 */
+static int parse_options(int argc, char *argv[], int *mounting, int *force,
+			 int *nonzero_decrement_is_error, char **alias)
+{
+	const char *optstr, *usagestr;
+	int opt, usage = 0, rc = -1;
+
+	*force = 0;
+	*nonzero_decrement_is_error = 1;
+	*alias = NULL;
+
+	/* Determine if mounting or unmounting by looking at the invocation */
+	if (strstr(argv[0], "umount") == NULL) {
+		*mounting = 1;
+		optstr = "h";
+		usagestr = "[ALIAS]\n"
+			   "Mount the default private directory or ALIAS, if specified.\n"
+			   "\n"
+			   "  -h        display this help and exit\n";
+	} else {
+		*mounting = 0;
+		optstr = "hfd";
+		usagestr = "[-f] [-d] [ALIAS]\n"
+			   "Unmount the default private directory or ALIAS, if specified.\n"
+			   "\n"
+			   "  -h        display this help and exit\n"
+			   "  -f        forcibly unmount\n"
+			   "  -d        don't treat a non-zero session counter as an error\n";
+	}
+
+	while ((opt = getopt(argc, argv, optstr)) != -1) {
+		switch (opt) {
+			case 'h':
+				rc = 1;
+				usage = 1;
+				goto out;
+			case 'f':
+				*force = 1;
+				break;
+			case 'd':
+				*nonzero_decrement_is_error = 0;
+				break;
+			default:
+				usage = 1;
+				goto out;
+		}
+	}
+
+	if (optind < (argc - 1)) {
+		usage = 1;
+		goto out;
+	} else if (optind == (argc - 1)) {
+		*alias = argv[optind];
+	}
+
+	rc = 0;
+out:
+	if (usage)
+		fprintf(stderr, "Usage: %s %s", argv[0], usagestr);
+	return rc;
+}
 
 /* This program is a setuid-executable allowing a non-privileged user to mount
  * and unmount an ecryptfs private directory.  This program is necessary to
@@ -509,8 +570,7 @@
 int main(int argc, char *argv[]) {
 	uid_t uid;
 	gid_t gid;
-	int mounting;
-	int force = 0;
+	int mounting, force, nonzero_decrement_is_error;
 	struct passwd *pwd;
 	char *alias, *src, *dest, *opt, *opts2;
 	char *sig_fekek = NULL, *sig_fnek = NULL;
@@ -532,10 +592,20 @@
 		goto fail;
 	}
 
+	switch (parse_options(argc, argv, &mounting, &force,
+			      &nonzero_decrement_is_error, &alias)) {
+		case -1:
+			goto fail;
+		case 1:
+			goto success;
+		default:
+			break; /* proceed */
+	}
+
 	/* If no arguments, default to private dir; but accept at most one
 	   argument, an alias for the configuration to read and use.
 	 */
-	if (argc == 1) {
+	if (alias == NULL) {
 		/* Use default source and destination dirs */
 		alias = ECRYPTFS_PRIVATE_DIR;
 		if ((asprintf(&src, "%s/.%s", pwd->pw_dir, alias) < 0) || src == NULL) {
@@ -547,8 +617,7 @@
 			perror("asprintf (dest)");
 			goto fail;
 		}
-	} else if (argc == 2) {
-		alias = argv[1];
+	} else {
 		/* Read the source and destination dirs from .conf file */
 		if (read_config(pwd->pw_dir, uid, alias, &src, &dest, &opts2) < 0) {
 			fputs("Error reading configuration file\n", stderr);
@@ -558,9 +627,6 @@
 			fputs("Mount options are not supported here\n", stderr);
 			exit(1);
 		}
-	} else {
-		fputs("Too many arguments\n", stderr);
-		exit(1);
 	}
 
 	if (strstr(alias, "..")) {
@@ -582,19 +648,6 @@
 		goto fail;
 	}
 
-	/* Determine if mounting or unmounting by looking at the invocation */
-	if (strstr(argv[0], "umount") == NULL) {
-		mounting = 1;
-	} else {
-		mounting = 0;
-		/* Determine if unmounting is forced */
-		if (argv[1] != NULL && strncmp(argv[1], "-f", 2) == 0) {
-			force = 1;
-		} else {
-			force = 0;
-		}
-	}
-
 	/* Fetch signatures from file */
 	/* First line is the file content encryption key signature */
 	/* Second line, if present, is the filename encryption key signature */
@@ -677,6 +730,8 @@
 		if (force == 1) {
 			zero(fh_counter);
 		} else if (decrement(fh_counter) > 0) {
+			if (!nonzero_decrement_is_error)
+				goto success;
 			fputs("Sessions still open, not unmounting\n", stderr);
 			goto fail;
 		}