commit e9b6bafe223ef9efee4feab042832c6a28587324
Author:     Jacob Welsh <jfw>
AuthorDate: Wed Apr 24 05:49:57 2024 +0000
Commit:     Jacob Welsh <jfw>
CommitDate: Wed Apr 24 05:49:57 2024 +0000

    busybox/archival: safe temp file handling, gapless metadata, symlink and node restoration in ARCHIVE_REPLACE_VIA_RENAME mode; don't skip restoring owner & date for symlinks just because mode isn't possible; catch metadata restoration errors.
    
    The renaming cases are now disentangled from the normal ones, as they have little in common besides low-level details which are now factored out.
    
    So far this mode is just used by the rpm applet, but looks nice to have more generally, perhaps eventually to replace the gpkg shell archive format.

diff --git a/base/busybox/archival/libarchive/data_extract_all.c b/base/busybox/archival/libarchive/data_extract_all.c
index 9e5cd45..32baa22 100644
--- a/base/busybox/archival/libarchive/data_extract_all.c
+++ b/base/busybox/archival/libarchive/data_extract_all.c
@@ -6,6 +6,146 @@
 #include "libbb.h"
 #include "bb_archive.h"
 
+/* Like bb_copyfd_exact_size but without death on error. */
+static int copyfd_exact_size(int src, int dst, off_t size)
+{
+	off_t sz = bb_copyfd_size(src, dst, size);
+	if (sz == (size >= 0 ? size : -size)) {
+		return 0;
+	}
+	if (sz != -1) {
+		bb_error_msg("short read");
+	}
+	/* if sz == -1, bb_copyfd_XX already complained */
+	return -1;
+}
+
+static int restore_uid_gid(char *name, archive_handle_t *archive_handle)
+{
+	file_header_t *file_header = archive_handle->file_header;
+	int res;
+	uid_t uid = file_header->uid;
+	gid_t gid = file_header->gid;
+#if ENABLE_FEATURE_TAR_UNAME_GNAME
+	if (!(archive_handle->ah_flags & ARCHIVE_NUMERIC_OWNER)) {
+		if (file_header->tar__uname) {
+			//TODO: cache last name/id pair?
+			struct passwd *pwd = getpwnam(file_header->tar__uname);
+			if (pwd) uid = pwd->pw_uid;
+		}
+		if (file_header->tar__gname) {
+			struct group *grp = getgrnam(file_header->tar__gname);
+			if (grp) gid = grp->gr_gid;
+		}
+	}
+#endif
+	/* GNU tar 1.15.1 uses chown, not lchown
+	 * ...but so what? If it really uses chown when restoring symlinks, then it's broken and perhaps even exploitable. */
+	res = lchown(name, uid, gid);
+	if (res)
+		bb_perror_msg("can't set ownership on '%s'", name);
+	return res;
+}
+
+static int restore_date(char *name, file_header_t *file_header)
+{
+	struct timespec t[2];
+	int res;
+
+	/* For portability, it's unclear if we can assume struct timespec has only the two known fields. */
+	memset(t, 0, sizeof t);
+	t[1].tv_sec = file_header->mtime;
+	t[1].tv_nsec = file_header->mtime_nsec;
+	/* Setting atime the same as mtime. */
+	t[0] = t[1];
+	res = utimensat(AT_FDCWD, name, t, AT_SYMLINK_NOFOLLOW);
+	if (res)
+		bb_perror_msg("can't set times on '%s'", name);
+	return res;
+}
+
+#ifdef ARCHIVE_REPLACE_VIA_RENAME
+static void atomic_replace_file(archive_handle_t *archive_handle)
+{
+	file_header_t *file_header = archive_handle->file_header;
+	char *tmp_name = 0;
+	int dst_fd;
+
+	if (S_ISREG(file_header->mode)) {
+		/* rpm-style temp file name */
+		tmp_name = xasprintf("%s;newXXXXXX", file_header->name);
+		dst_fd = mkstemp(tmp_name);
+		if (dst_fd < 0) {
+			bb_perror_msg("can't open temp file for '%s'", file_header->name);
+			goto fail;
+		}
+
+		/* XXX are we sure size > 0? */
+		if (copyfd_exact_size(archive_handle->src_fd, dst_fd,
+					file_header->size) == -1) {
+			close(dst_fd);
+			goto fail_unlink;
+		}
+		close(dst_fd);
+	} else {
+		/* Doesn't quite guarantee a unique name, but at least symlink() and mknod() don't clobber existing files like open() can. */
+		tmp_name = xasprintf("%s;new%d", file_header->name, (int) getpid());
+
+		if (S_ISLNK(file_header->mode)) {
+			if (symlink(file_header->link_target, tmp_name)) {
+				bb_perror_msg("can't create %slink from %s to %s", "sym",
+						tmp_name, file_header->link_target);
+				goto fail;
+			}
+		} else if (mknod(tmp_name, file_header->mode, file_header->device)) {
+			bb_perror_msg("can't create node %s", tmp_name);
+			goto fail;
+		}
+	}
+
+	/* Need to restore metadata *before* the rename, since the point of this extraction mode is to prevent even momentary breakage when replacing components on a live system. */
+	if (!(archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_OWNER))
+		if (restore_uid_gid(tmp_name, archive_handle))
+			goto fail_unlink;
+
+	/* Symlinks have no meaningful mode or at least no available way to set it. */
+	if (!S_ISLNK(file_header->mode)) {
+		mode_t mode = file_header->mode;
+
+		if (S_ISREG(file_header->mode)
+		 && (archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_PERM)
+		) {
+			/* XXX we have to noisily derive the normal local file creation mode, because mkstemp sets the unusual mode 0600. Maybe busybox needs its own mkstemp. */
+			mode_t mask = umask(0);
+			umask(mask);
+			mode = 0666 & ~mask;
+		}
+		if (chmod(tmp_name, mode)) {
+			bb_perror_msg("can't set mode on '%s'", tmp_name);
+			goto fail_unlink;
+		}
+	}
+
+	if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE)
+		if (restore_date(tmp_name, file_header))
+			goto fail_unlink;
+
+	if (rename(tmp_name, file_header->name)) {
+		bb_perror_msg("can't move '%s' to '%s'", tmp_name, file_header->name);
+		goto fail_unlink;
+	}
+
+	free(tmp_name); /* A pity gcc never invented safe stack allocation. */
+	return;
+
+fail_unlink:
+	unlink(tmp_name);
+fail:
+	free(tmp_name);
+	archive_handle->status = EXIT_FAILURE;
+}
+#endif /* ARCHIVE_REPLACE_VIA_RENAME */
+
 void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 {
 	file_header_t *file_header = archive_handle->file_header;
@@ -117,7 +257,9 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 				goto ret;
 			}
 			/* As above.
-			 * Note: we could skip the ENOENT case this time, since we just determined that the file exists. But why leave a race condition when it's easily prevented? In general though we're not bothering about such conditions, because we can't e.g. atomically open or chdir with check for symlink, so security will have to depend on only one process creating entries based on untrusted data in the same directory at the same time.
+			 * Note: we could skip the ENOENT case this time, since we just determined that the file exists. But why leave a race condition when it's easily prevented?
+			 * In general though we're not bothering about such conditions, so security will depend on only one process creating entries based on untrusted data in the same directory at the same time.
+			 * This could perhaps be improved by walking the path using openat() with O_NOFOLLOW and doing the rest relative to the resulting fd.
 			 */
 			if (unlink(file_header->name) == -1
 			 && errno != ENOENT
@@ -151,13 +293,11 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 	switch (file_header->mode & S_IFMT) {
 	case S_IFREG: {
 		/* Regular file */
-		char *dst_name;
-		int flags;
 
 		/* We encode hard links as regular files of size 0 with a symlink */
 		if (file_header->link_target && file_header->size == 0) {
 			if (link(file_header->link_target, file_header->name) == -1) {
-				bb_perror_msg("can't create hardlink from %s to %s",
+				bb_perror_msg("can't create %slink from %s to %s", "hard",
 						file_header->name,
 						file_header->link_target);
 				goto abort;
@@ -166,54 +306,33 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 			goto ret;
 		}
 
-		if (archive_handle->ah_flags & ARCHIVE_O_TRUNC)
-			flags = O_WRONLY | O_CREAT | O_TRUNC;
-		else
-			flags = O_WRONLY | O_CREAT | O_EXCL;
-		dst_name = file_header->name;
 #ifdef ARCHIVE_REPLACE_VIA_RENAME
-		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME)
-			/* rpm-style temp file name */
-			/* XXX not guaranteed unique... maybe force O_EXCL or use mkstemp */
-			dst_name = xasprintf("%s;%x", dst_name, (int)getpid());
+		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME) {
+			atomic_replace_file(archive_handle);
+			goto ret; /* Skip chown/chmod/utimes, already done. */
+			/* And don't goto abort on error; we may have already modified the parent directory, so need to restore its times. */
+		}
 #endif
 		{
 			int dst_fd;
-			off_t sz;
+			int flags = O_WRONLY | O_CREAT | O_EXCL;
+			if (archive_handle->ah_flags & ARCHIVE_O_TRUNC)
+				flags = O_WRONLY | O_CREAT | O_TRUNC;
 
-			dst_fd = open(dst_name, flags, file_header->mode);
+			dst_fd = open(file_header->name, flags, file_header->mode);
 			if (dst_fd < 0) {
-				bb_perror_msg("can't open '%s'", dst_name);
+				bb_perror_msg("can't open '%s'", file_header->name);
 				goto abort;
 			}
 
-			/* As in bb_copyfd_exact_size but without the xfunc_die(). */
 			/* XXX are we sure size > 0? */
-			sz = bb_copyfd_size(archive_handle->src_fd, dst_fd,
-					file_header->size);
-			if (sz != file_header->size) {
-				if (sz != -1) {
-					bb_error_msg("short read");
-				}
-				/* if sz == -1, bb_copyfd_XX already complained */
+			if (copyfd_exact_size(archive_handle->src_fd, dst_fd,
+						file_header->size) == -1) {
 				close(dst_fd);
 				goto abort;
 			}
 			close(dst_fd);
 		}
-#ifdef ARCHIVE_REPLACE_VIA_RENAME
-		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME) {
-			if (rename(dst_name, file_header->name)) {
-				bb_perror_msg("can't move '%s' to '%s'", dst_name,
-						file_header->name);
-				free(dst_name);
-				/* Don't goto abort: we already modified the parent directory so may need to restore its times. */
-				archive_handle->status = EXIT_FAILURE;
-				goto ret;
-			}
-			free(dst_name);
-		}
-#endif
 		break;
 	}
 	case S_IFDIR:
@@ -228,25 +347,35 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		break;
 	case S_IFLNK:
 		/* Symlink */
-		if (file_header->link_target) {
-			if (symlink(file_header->link_target, file_header->name) == -1) {
-				bb_perror_msg("can't create %slink "
-						"from %s to %s", "sym",
-						file_header->name,
-						file_header->link_target);
-				goto abort;
-			}
-		} else {
+		if (!file_header->link_target) {
 			bb_perror_msg("inconsistent file header: "
 					"symlink without target %s",
 					file_header->name);
 			goto abort;
 		}
+#ifdef ARCHIVE_REPLACE_VIA_RENAME
+		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME) {
+			atomic_replace_file(archive_handle);
+			goto ret; /* Skip chown/chmod/utimes, already done. */
+		}
+#endif
+		if (symlink(file_header->link_target, file_header->name)) {
+			bb_perror_msg("can't create %slink from %s to %s", "sym",
+					file_header->name,
+					file_header->link_target);
+			goto abort;
+		}
 		break;
 	case S_IFSOCK:
 	case S_IFBLK:
 	case S_IFCHR:
 	case S_IFIFO:
+#ifdef ARCHIVE_REPLACE_VIA_RENAME
+		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME) {
+			atomic_replace_file(archive_handle);
+			goto ret; /* Skip chown/chmod/utimes, already done. */
+		}
+#endif
 		if (mknod(file_header->name, file_header->mode,
 					file_header->device) == -1) {
 			bb_perror_msg("can't create node %s", file_header->name);
@@ -258,44 +387,21 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		goto abort;
 	}
 
-	if (!S_ISLNK(file_header->mode)) {
-		if (!(archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_OWNER)) {
-			uid_t uid = file_header->uid;
-			gid_t gid = file_header->gid;
-#if ENABLE_FEATURE_TAR_UNAME_GNAME
-			if (!(archive_handle->ah_flags & ARCHIVE_NUMERIC_OWNER)) {
-				if (file_header->tar__uname) {
-//TODO: cache last name/id pair?
-					struct passwd *pwd = getpwnam(file_header->tar__uname);
-					if (pwd) uid = pwd->pw_uid;
-				}
-				if (file_header->tar__gname) {
-					struct group *grp = getgrnam(file_header->tar__gname);
-					if (grp) gid = grp->gr_gid;
-				}
+	if (!(archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_OWNER))
+		if (restore_uid_gid(file_header->name, archive_handle))
+			archive_handle->status = EXIT_FAILURE; /* but continue */
+
+	/* symlinks have no meaningful mode or at least no available way to set it */
+	if (!S_ISLNK(file_header->mode))
+		if (!(archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_PERM))
+			if (chmod(file_header->name, file_header->mode)) {
+				bb_perror_msg("can't set mode on '%s'", file_header->name);
+				archive_handle->status = EXIT_FAILURE; /* but continue */
 			}
-#endif
-			/* GNU tar 1.15.1 uses chown, not lchown */
-			chown(file_header->name, uid, gid);
-		}
-		/* uclibc has no lchmod, glibc is even stranger -
-		 * it has lchmod which seems to do nothing!
-		 * so we use chmod... */
-		if (!(archive_handle->ah_flags & ARCHIVE_DONT_RESTORE_PERM)) {
-			chmod(file_header->name, file_header->mode);
-		}
-		if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE) {
-			struct timespec t[2];
-
-			/* For portability, it's unclear if we can assume struct timespec has only the two known fields. */
-			memset(t, 0, sizeof t);
-			t[1].tv_sec = file_header->mtime;
-			t[1].tv_nsec = file_header->mtime_nsec;
-			/* Setting atime the same as mtime. */
-			t[0] = t[1];
-			utimensat(AT_FDCWD, file_header->name, t, 0);
-		}
-	}
+
+	if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE)
+		if (restore_date(file_header->name, file_header))
+			archive_handle->status = EXIT_FAILURE; /* but continue */
 
  ret:
 	if (restore_parent_times) {