[PATCH] bb_make_directory(): simplify and robustify

[Rasmus Villemoes]

For something like 'mkdir -m 0700 foo', if the caller happens to have
a permissive umask (say allowing group write via 0007 or 0002), the
created directory will temporarily have more permissions than
requested. That's a mild security issue.

This reworks bb_make_directory() to always create both intermediate
and the final component with 0 permissions, then chmods to the final
value.

Aside from the robustification, this also simplifies the code
somewhat (net -31LOC), since we get rid of the complicated "what umask
do we have now and be sure to set it back" - we only need to query the
current umask in case mode==-1, and can set it back immediately. Then
mode contains the mode of the final component, and intermediate
components should then be chmod'ed to pmode=mode|0300.

x86_64 defconfig:

function                                             old     new   delta
bb_make_directory                                    452     396     -56

Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
---
Lightly tested and strace'd to see that it behaves as expected.

 libbb/make_directory.c | 83 +++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 57 deletions(-)

diff --git a/libbb/make_directory.c b/libbb/make_directory.c
index 9b03bb8d0..9d95c2383 100644
--- a/libbb/make_directory.c
+++ b/libbb/make_directory.c
@@ -26,9 +26,8 @@
 
 int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 {
-	mode_t cur_mask;
-	mode_t org_mask;
 	const char *fail_msg;
+	mode_t pmode;
 	char *s;
 	char c;
 	struct stat st;
@@ -48,7 +47,13 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 //			return 0; /* ".." */
 	}
 
-	org_mask = cur_mask = (mode_t)-1L;
+	if (mode == -1) {
+		mode_t org_mask = umask(0);
+		mode = 0777 & ~org_mask;
+		umask(org_mask);
+	}
+	pmode = mode | 0300;
+
 	s = path;
 	while (1) {
 		c = '\0';
@@ -68,32 +73,8 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 			}
 		}
 
-		if (c != '\0') {
-			/* Intermediate dirs: must have wx for user */
-			if (cur_mask == (mode_t)-1L) { /* wasn't done yet? */
-				mode_t new_mask;
-				org_mask = umask(0);
-				cur_mask = 0;
-				/* Clear u=wx in umask - this ensures
-				 * they won't be cleared on mkdir */
-				new_mask = (org_mask & ~(mode_t)0300);
-				//bb_error_msg("org_mask:%o cur_mask:%o", org_mask, new_mask);
-				if (new_mask != cur_mask) {
-					cur_mask = new_mask;
-					umask(new_mask);
-				}
-			}
-		} else {
-			/* Last component: uses original umask */
-			//bb_error_msg("1 org_mask:%o", org_mask);
-			if (org_mask != cur_mask) {
-				cur_mask = org_mask;
-				umask(org_mask);
-			}
-		}
-
 		//bb_error_msg("mkdir '%s'", path);
-		if (mkdir(path, 0777) < 0) {
+		if (mkdir(path, 0000) < 0) {
 			/* If we failed for any other reason than the directory
 			 * already exists, output a diagnostic and return -1 */
 			if ((errno != EEXIST && errno != EISDIR)
@@ -103,36 +84,31 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 				fail_msg = "create";
 				break;
 			}
-			/* Since the directory exists, don't attempt to change
-			 * permissions if it was the full target.  Note that
-			 * this is not an error condition. */
-			if (!c) {
-				goto ret0;
-			}
+			/* Since the directory exists, don't attempt
+			 * to change permissions.  Note that this is
+			 * not an error condition. */
 		} else {
 			if (flags & FILEUTILS_VERBOSE) {
 				printf("created directory: '%s'\n", path);
 			}
-		}
-
-		if (!c) {
-			/* Done.  If necessary, update perms on the newly
-			 * created directory.  Failure to update here _is_
-			 * an error. */
-			if (mode != -1) {
-				//bb_error_msg("chmod 0%03lo mkdir '%s'", mode, path);
-				if (chmod(path, mode) < 0) {
-					fail_msg = "set permissions of";
-					if (flags & FILEUTILS_IGNORE_CHMOD_ERR) {
-						flags = 0;
-						goto print_err;
-					}
-					break;
+			if (chmod(path, c ? pmode : mode) < 0) {
+				fail_msg = "set permissions of";
+				if (flags & FILEUTILS_IGNORE_CHMOD_ERR) {
+					flags = 0;
+					goto print_err;
 				}
+				break;
 			}
-			goto ret0;
 		}
 
+		/* If this was the final component, we are done.
+		 * Otherwise, continue with the next component, which
+		 * may then fail to be created if the current one
+		 * already existed but does not have sufficient
+		 * permissions. */
+		if (!c)
+			return 0;
+
 		/* Remove any inserted nul from the path (recursive mode) */
 		*s = c;
 	} /* while (1) */
@@ -140,12 +116,5 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 	flags = -1;
  print_err:
 	bb_perror_msg("can't %s directory '%s'", fail_msg, path);
-	goto ret;
- ret0:
-	flags = 0;
- ret:
-	//bb_error_msg("2 org_mask:%o", org_mask);
-	if (org_mask != cur_mask)
-		umask(org_mask);
 	return flags;
 }
-- 
2.23.0