commit cf6cede46aae3b952c561f290b97f4e4e9bb7555
Author:     Jacob Welsh <jfw>
AuthorDate: Fri Apr 19 21:56:53 2024 +0000
Commit:     Jacob Welsh <jfw>
CommitDate: Fri Apr 19 22:31:16 2024 +0000

    busybox/archival: replace bb_make_directory with full prefix walk, ruling out symlink attacks and unintended dir mtime changes at that stage.
    
    No longer treating the special case of preserving mtime on "."; it gets too complicated.

diff --git a/base/busybox/archival/libarchive/data_extract_all.c b/base/busybox/archival/libarchive/data_extract_all.c
index ef3510e..f8904f3 100644
--- a/base/busybox/archival/libarchive/data_extract_all.c
+++ b/base/busybox/archival/libarchive/data_extract_all.c
@@ -12,8 +12,9 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 	int dst_fd;
 	int res;
 	int restore_parent_times = 0;
-	struct stat parent_stat;
+	struct timespec parent_times[2];
 	char *slash = 0;
+	char *lastslash = 0;
 
 #if ENABLE_FEATURE_TAR_SELINUX
 	char *sctx = archive_handle->tar__sctx[PAX_NEXT_FILE];
@@ -26,27 +27,46 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 	}
 #endif
 
-	/* TODO since we're doing a parent stat, check for symlink attacks */
-	if (archive_handle->ah_flags & (ARCHIVE_CREATE_LEADING_DIRS | ARCHIVE_RESTORE_DATE)) {
-		slash = strrchr(file_header->name, '/');
-		if (slash) {
-			/* compound path: might need to create parent dir or later restore its times */
-			*slash = '\0';
-			if (stat(file_header->name, &parent_stat) == -1) {
-				if (errno == ENOENT && (archive_handle->ah_flags & ARCHIVE_CREATE_LEADING_DIRS)) {
-					/* XXX doesn't preserve parent timestamps (or perhaps check for symlinks) */
-					bb_make_directory(file_header->name, -1, FILEUTILS_RECUR);
-				}
-			} else if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE) {
-				restore_parent_times = 1;
+	/* Walk the path, validating and creating dirs as needed. */
+	for (slash = strchr(file_header->name, '/');
+			slash; slash = strchr(slash+1, '/')) {
+		struct stat st;
+
+		*slash = '\0';
+		if (lstat(file_header->name, &st) == -1) {
+			if (errno != ENOENT) {
+				bb_perror_msg_and_die("can't stat prefix %s", file_header->name);
+			}
+			if (!(archive_handle->ah_flags & ARCHIVE_CREATE_LEADING_DIRS)) {
+				/* Prefix doesn't exist but we weren't asked to create. (Could probably ignore this and let whatever subsequent syscall catch the problem.) */
+				bb_simple_perror_msg_and_die(file_header->name);
+			}
+			if (mkdir(file_header->name, 0777) == -1) {
+				bb_perror_msg_and_die("can't create prefix %s",
+					file_header->name);
 			}
-			*slash = '/';
-		} else if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE) {
-			/* simple filename: nothing to create but will still need to restore */
-			if (stat(".", &parent_stat) == 0) {
+			if (restore_parent_times) {
+				*lastslash = '\0';
+				utimensat(AT_FDCWD, file_header->name, parent_times, 0);
+				*lastslash = '/';
+				/* This level didn't exist, so the next one has nothing to restore. */
+				restore_parent_times = 0;
+			}
+		} else {
+			/* Prefix exists */
+			if (!S_ISDIR(st.st_mode)) {
+				/* Don't defer catching this to a later syscall, as it could be a directory symlink, which would lead us to successfully write outside the designated path (symlink attack)! */
+				bb_error_msg_and_die("prefix not a directory: %s", file_header->name);
+			}
+			if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE) {
+				/* We were asked to preserve archive timestamps; but directory mtimes are bumped by creating/renaming/unlinking anything inside, so we need to restore them afterward. It might be preferred to do this only for directories explicitly created by the archive; but without maintaining a space-consuming list of those, there's no way to distinguish. */
+				parent_times[0] = st.st_atim;
+				parent_times[1] = st.st_mtim;
 				restore_parent_times = 1;
 			}
 		}
+		*slash = '/';
+		lastslash = slash;
 	}
 
 	if (archive_handle->ah_flags & ARCHIVE_UNLINK_OLD) {
@@ -76,6 +96,7 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 						file_header->name);
 			}
 		}
+		/* XXX and if it IS a directory desired? symlink attack here */
 	}
 	else if (archive_handle->ah_flags & ARCHIVE_EXTRACT_NEWER) {
 		/* Remove the existing entry if its older than the extracted entry */
@@ -157,6 +178,7 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		) {
 			bb_perror_msg("can't make dir %s", file_header->name);
 		}
+		/* XXX if it exists and ISN'T a dir? symlink attack here */
 		break;
 	case S_IFLNK:
 		/* Symlink */
@@ -227,16 +249,9 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 
  ret:
 	if (restore_parent_times) {
-		struct timespec t[2];
-		t[0] = parent_stat.st_atim;
-		t[1] = parent_stat.st_mtim;
-		if (slash) {
-			*slash = '\0';
-			utimensat(AT_FDCWD, file_header->name, t, 0);
-			*slash = '/';
-		} else {
-			utimensat(AT_FDCWD, ".", t, 0);
-		}
+		*lastslash = '\0';
+		utimensat(AT_FDCWD, file_header->name, parent_times, 0);
+		*lastslash = '/';
 	}
 #if ENABLE_FEATURE_TAR_SELINUX
 	if (sctx) {