WIP cleanup: split mkhtemp

Signed-off-by: Leah Rowe <leah@libreboot.org>

3 files changed, 115 insertions, 188 deletions

diff --git a/util/nvmutil/include/common.h b/util/nvmutil/include/common.h
index 930150df..b47e5f4b 100644
--- a/util/nvmutil/include/common.h
+++ b/util/nvmutil/include/common.h
@@ -480,6 +480,13 @@ const char *getnvmprogname(void);
 
 char *new_tmpfile(int *fd);
 char *new_tmplate(int *fd);
+static int mkhtemp_try_create(int dirfd,
+    struct stat *st_dir_initial,
+    char *fname_copy,
+    char *p,
+    size_t xc,
+    int *fd,
+    struct stat *st);
 int mkhtemp(int *fd, struct stat *st,
     char *template, int dirfd, const char *fname,
     struct stat *st_dir_initial);
diff --git a/util/nvmutil/lib/file.c b/util/nvmutil/lib/file.c
index 8ebb93d7..ee849b73 100644
--- a/util/nvmutil/lib/file.c
+++ b/util/nvmutil/lib/file.c
@@ -700,283 +700,203 @@ sticky_hell:
  *                    generates files)
  */
 
-int mkhtemp(int *fd,
-    struct stat *st, 
+int
+mkhtemp(int *fd,
+    struct stat *st,
     char *template,
     int dirfd,
     const char *fname,
     struct stat *st_dir_initial)
 {
-	/* NOTE: this function currently
-	 * 	 only supports *files*.
-	 *	 it doesn't make tmp*dirs*
-	 */
 	size_t len = 0;
-	char *p = NULL;
-	char *template_copy = NULL;
 	size_t xc = 0;
-	size_t r;
+	size_t fname_len = 0;
+
+	char *template_copy = NULL;
+	char *fname_copy = NULL;
+	char *p;
+
+	size_t retries;
+
+	int saved_errno = errno;
+
 #if defined(PATH_LEN) && \
-	(PATH_LEN) >= 256
+    (PATH_LEN) >= 256
 	size_t max_len = PATH_LEN;
 #else
 	size_t max_len = 4096;
 #endif
-	int file_created = 0;
-	int saved_errno = errno;
-	struct stat st_tmp;
-	mode_t old_umask;
-	char *fname_copy = NULL;
-	size_t fname_len = 0;
-	/* for ctrl char check
-	 */
-	unsigned char ctrl = 0;
-	size_t ctrl_pos = 0;
-	struct stat st_dir_now;
-	size_t retries = 0;
-#if !(defined(MAX_MKHTEMP_RETRIES) && \
-    (MAX_MKHTEMP_RETRIES) >= 128)
-	size_t max_retries = 300;
-#else
-	size_t max_retries = 128;
-#endif
 
-	if (fname == NULL ||
-	    st_dir_initial == NULL ||
-	    fd == NULL) {
+	if (fd == NULL ||
+	    template == NULL ||
+	    fname == NULL ||
+	    st_dir_initial == NULL) {
 
 		errno = EFAULT;
-		goto err_mkhtemp;
+		return -1;
 	}
-	if (dirfd < 0) {
 
-		errno = EBADF;
-		goto err_mkhtemp;
-	}
 	if (*fd >= 0) {
-
 		errno = EEXIST;
-		goto err_mkhtemp;
+		return -1;
+	}
+
+	if (dirfd < 0) {
+		errno = EBADF;
+		return -1;
 	}
 
-	if (slen(template, max_len, &len) < 0) {
-		goto err_mkhtemp;
-	} else if (len >= max_len) { /* bounds check */
+	if (slen(template, max_len, &len) < 0)
+		return -1;
+
+	if (len >= max_len) {
 		errno = EMSGSIZE;
-		goto err_mkhtemp;
+		return -1;
 	}
 
-	if (strrchr(fname, '/') != NULL) {
+	if (slen(fname, max_len, &fname_len) < 0)
+		return -1;
 
-		/* otherwise, a mangled
-		   path could leave the
-		   directory, defeating
-		   the purpose of openat
-		 */
+	if (fname_len == 0) {
 		errno = EINVAL;
-		goto err_mkhtemp;
+		return -1;
 	}
 
-	/* reject dangerous basenames
-	 */
-	if (slen(fname, max_len, &fname_len) < 0) {
-		goto err_mkhtemp;
-	} else if (fname_len == 0) {
-		errno = EINVAL;
-		goto err_mkhtemp;
-	} else if (fname[0] == '\0' ||
-	    (fname[0] == '.' && fname[1] == '\0') ||
-	    (fname[0] == '.' && fname[1] == '.' && fname[2] == '\0')) {
+	if (strrchr(fname, '/') != NULL) {
 		errno = EINVAL;
-		goto err_mkhtemp;
+		return -1;
 	}
-	/* block control chars
-	 */
-	for (ctrl_pos = 0;
-	    ctrl_pos < fname_len;
-	    ctrl_pos++) {
 
-		ctrl = (unsigned char)fname[ctrl_pos];
+	/* count trailing X */
+	{
+		char *end = template + len;
 
-		if (ctrl < 32 || ctrl == 127) {
-			errno = EINVAL;
-			goto err_mkhtemp;
-		}
+		while (end > template && *--end == 'X')
+			xc++;
 	}
 
-	p = template + len;
-
-	while (p > template &&
-	    *--p == 'X')
-		++xc;
-
 	if (xc < 6 || xc > len) {
-
 		errno = EINVAL;
-		goto err_mkhtemp;
+		return -1;
 	}
 
 	if (fname_len > len ||
 	    fname_len > (len - xc)) {
-
-		/* prevent overflow
-		 */
 		errno = EOVERFLOW;
-		goto err_mkhtemp;
+		return -1;
 	}
 
 	if (memcmp(fname,
 	    template + len - fname_len,
 	    fname_len) != 0) {
-
 		errno = EINVAL;
-		goto err_mkhtemp;
+		return -1;
 	}
 
 	template_copy = malloc(len + 1);
 	if (template_copy == NULL) {
-
 		errno = ENOMEM;
-		goto err_mkhtemp;
+		goto err;
 	}
 
-	/* we work on a cached copy first,
-	 * to avoid partial writes of the
-	 * input under fault conditions
-	 */
-	memcpy(template_copy, template, len + 1);
-
-	/* redundant copy, reduce chance of
-	 * mangling (regression mitigation)
-	 */
-
 	fname_copy = malloc(fname_len + 1);
-
 	if (fname_copy == NULL) {
-
 		errno = ENOMEM;
-		goto err_mkhtemp;
+		goto err;
 	}
 
+	memcpy(template_copy, template, len + 1);
+
 	memcpy(fname_copy,
 	    template_copy + len - fname_len,
 	    fname_len + 1);
 
 	p = fname_copy + fname_len - xc;
 
-	for (retries = 0; retries < max_retries; retries++) {
-
-		if (mkhtemp_fill_random(p, xc) < 0)
-			goto err_mkhtemp;
-
-		/*
-		 * use the file descriptor instead.
-		 * (danach prüfen wir die Sicherheit des Verzeichnisses)
-		 */
-		if (fstat(dirfd, &st_dir_now) < 0)
-			goto err_mkhtemp;
-		/*
-		 * mitigate symlink attacks before open
-		 */
-		if (st_dir_now.st_dev != st_dir_initial->st_dev ||
-			st_dir_now.st_ino != st_dir_initial->st_ino) {
-			errno = ESTALE;
-			goto err_mkhtemp;
-		}
-
-		*fd = openat2p(dirfd, fname_copy,
-		    O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC |
-		    O_NOCTTY, 0600);
-
-		if (*fd >= 0) {
+	for (retries = 0; retries < 300; retries++) {
 
-			file_created = 1;
+		int r = mkhtemp_try_create(
+		    dirfd,
+		    st_dir_initial,
+		    fname_copy,
+		    p,
+		    xc,
+		    fd,
+		    st);
 
-			memcpy(template_copy + len - fname_len,
+		if (r == 1) {
+			/* success: copy final name back */
+			memcpy(template + len - fname_len,
 			    fname_copy, fname_len);
 
-			if (secure_file(fd, st, O_APPEND, 1, 1, 0600) < 0)
-				goto err_mkhtemp;
-
-			/* copy replaced characters
-			 */
-			memcpy(
-			    template + len + xc,
-			    template_copy + len + xc,
-			    xc);
-
-			if (template_copy != NULL) {
-
-				free(template_copy);
-				template_copy = NULL;
-			}
-
 			errno = saved_errno;
-
-			/* thunder room secure
-			 */
-			return *fd;
+			goto success;
 		}
 
-		if (errno != EEXIST &&
-		    errno != EINTR &&
-		    errno != EAGAIN)
-			goto err_mkhtemp;
+		if (r == -1)
+			goto err;
 	}
 
-err_mkhtemp:
-
-	saved_errno = errno;
+	errno = EEXIST;
 
+err:
 	if (*fd >= 0) {
-
-		(void) close_on_eintr(*fd);
-
+		(void)close_on_eintr(*fd);
 		*fd = -1;
-		/* ^^^^^   the caller gives us a dir,
-			   for use and we write in it
-			   but *we* create their file
-
-			   touch their *fd, not dirfd
-			   as we only initialised *fd
-			   */
 	}
 
-	if (template_copy != NULL) {
+success:
+	if (template_copy != NULL)
+		free(template_copy);
 
-		/* we created it, so *we* nuke it
-		 */
-		if (file_created &&
-		    fname_copy != NULL)
-			(void) unlinkat(dirfd, fname_copy, 0);
+	if (fname_copy != NULL)
+		free(fname_copy);
 
-		free(template_copy);
+	return (*fd >= 0) ? *fd : -1;
+}
 
-		template_copy = NULL;
-	}
+static int
+mkhtemp_try_create(int dirfd,
+    struct stat *st_dir_initial,
+    char *fname_copy,
+    char *p,
+    size_t xc,
+    int *fd,
+    struct stat *st)
+{
+	struct stat st_dir_now;
 
-	if (fname_copy != NULL) {
+	if (mkhtemp_fill_random(p, xc) < 0)
+		return -1;
 
-		free(fname_copy);
-		fname_copy = NULL;
+	if (fstat(dirfd, &st_dir_now) < 0)
+		return -1;
+
+	if (st_dir_now.st_dev != st_dir_initial->st_dev ||
+	    st_dir_now.st_ino != st_dir_initial->st_ino) {
+		errno = ESTALE;
+		return -1;
 	}
 
-	errno = saved_errno;
+	*fd = openat2p(dirfd, fname_copy,
+	    O_RDWR | O_CREAT | O_EXCL |
+	    O_NOFOLLOW | O_CLOEXEC | O_NOCTTY,
+	    0600);
 
-	/* returning EINTR/EAGAIN
-	   ourselves means that a
-	   caller could implement
-	   the same wait loop
-	 */
-	if (errno != EEXIST &&
-	    errno != EINTR &&
-	    errno != EAGAIN) {
+	if (*fd >= 0) {
+
+		if (secure_file(fd, st, O_APPEND, 1, 1, 0600) < 0)
+			return -1;
 
-		if (errno == saved_errno)
-			errno = ECANCELED;
+		return 1; /* success */
 	}
 
-	return -1;
+	if (errno == EEXIST ||
+	    errno == EINTR ||
+	    errno == EAGAIN)
+		return 0; /* retry */
+
+	return -1; /* fatal */
 }
 
 int
diff --git a/util/nvmutil/lib/io.c b/util/nvmutil/lib/io.c
index 87163359..e95be73c 100644
--- a/util/nvmutil/lib/io.c
+++ b/util/nvmutil/lib/io.c
@@ -473,7 +473,7 @@ gbe_mv(void)
 
 	/* create replacement temp in target directory
 	 */
-	dest_tmp = new_tmpfile(&dest_fd, 1, f->fname);
+	dest_tmp = new_tmpfile(&dest_fd);
 	if (dest_tmp == NULL)
 		goto ret_gbe_mv;