commit 621173e5867c76bcb3c5c0dfa79526b02cb667a9 Author: Jacob Welsh AuthorDate: Sun Apr 21 01:32:25 2024 +0000 Commit: Jacob Welsh CommitDate: Sun Apr 21 03:10:21 2024 +0000 busybox/archival: close the remaining symlink attack vector; related fixes Re-indenting is deferred to better highlight the actual changes. In ARCHIVE_UNLINK_OLD mode we can't ignore an existing local file, assuming it to be a directory, just because the archive entry is a directory. This enabled incorrect/incomplete extraction without warning, but not actually a separate symlink attack from the main one previously closed. (It could leave a symlink where a directory was intended, but preventing that link from being followed when extracting a later file remains the responsibility of the code doing that extracting.) In ARCHIVE_O_TRUNC mode we need to pre-check that existing local files are of suitable type, lest we proceed to writing into a symlink (or even device node), especially since arbitrary ones can be created by earlier entries in a malicious archive. Complete their TODO on checking for null link_target pointer from a bogus archive. diff --git a/base/busybox/archival/libarchive/data_extract_all.c b/base/busybox/archival/libarchive/data_extract_all.c index 63fa155..9253e28 100644 --- a/base/busybox/archival/libarchive/data_extract_all.c +++ b/base/busybox/archival/libarchive/data_extract_all.c @@ -71,7 +71,6 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle) if (archive_handle->ah_flags & ARCHIVE_UNLINK_OLD) { /* Remove the entry if it exists */ - if (!S_ISDIR(file_header->mode)) { /* Is it hardlink? * We encode hard links as regular files of size 0 with a symlink */ if (S_ISREG(file_header->mode) @@ -89,24 +88,29 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle) goto ret; } /* Proceed with deleting */ + /* Note 1: here and later we assume the Linux behavior that unlink on a directory fails with EISDIR. BSDs and POSIX don't distinguish this from EPERM, while UFS on Solaris reportedly allows root to unlink even non-empty directories. Accomodating these would require some more complex sequence. + * Note 2: skipping the unlink when S_ISDIR(file_header->mode), as formerly done here, is not a valid substitute because it fails to replace an existing non-directory file. Although it doesn't seem to directly open a symlink attack due to the earlier path check. + */ if (unlink(file_header->name) == -1 && errno != ENOENT + && !(errno == EISDIR && S_ISDIR(file_header->mode)) ) { bb_perror_msg_and_die("can't remove old file %s", 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 */ + else if (archive_handle->ah_flags & (ARCHIVE_EXTRACT_NEWER | ARCHIVE_O_TRUNC)) { + /* Both these modes require looking at the inode before we leap. */ struct stat existing_sb; if (lstat(file_header->name, &existing_sb) == -1) { if (errno != ENOENT) { - bb_perror_msg_and_die("can't stat old file"); + bb_perror_msg_and_die("can't stat old file %s", + file_header->name); } - } - else if (existing_sb.st_mtime >= file_header->mtime) { + /* Doesn't exist, no problem */ + } else if (archive_handle->ah_flags & ARCHIVE_EXTRACT_NEWER) { + /* Remove the existing entry if it's older than the extracted entry */ + if (existing_sb.st_mtime >= file_header->mtime) { if (!(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET) && !S_ISDIR(file_header->mode) ) { @@ -120,6 +124,16 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle) bb_perror_msg_and_die("can't remove old file %s", file_header->name); } + } else { /* ARCHIVE_O_TRUNC mode */ + /* A directory may naturally block the overwrite, but not all special file types will; especially do NOT try writing into a symlink, fifo or device node! */ + if (!S_ISREG(existing_sb.st_mode) + && !(S_ISDIR(existing_sb.st_mode) && S_ISDIR(file_header->mode)) + ) { + bb_error_msg_and_die("can't overwrite non-regular file %s", + file_header->name); + } + /* Regular file, no problem */ + } } /* Create the filesystem entry */ @@ -143,7 +157,6 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle) } if (archive_handle->ah_flags & ARCHIVE_O_TRUNC) - /* TODO symlink attack safety (eg tar file first lists a symlink, then a regular file of the same name, causing us to truncate & overwrite an arbitrary file) */ flags = O_WRONLY | O_CREAT | O_TRUNC; else flags = O_WRONLY | O_CREAT | O_EXCL; @@ -176,11 +189,11 @@ 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 */ + /* TODO maybe warn also if it exists and ISN'T a dir. Due to the initial path check, this doesn't by itself open a symlink attack, but it does mean we failed to reproduce the archive structure. Although this only arises if neither ARCHIVE_UNLINK_OLD nor ARCHIVE_O_TRUNC is set, which doesn't happen for tar, the use case of most interest. */ break; case S_IFLNK: /* Symlink */ -//TODO: what if file_header->link_target == NULL (say, corrupted tarball?) + if (file_header->link_target) { res = symlink(file_header->link_target, file_header->name); if ((res == -1) && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET) @@ -190,6 +203,11 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle) file_header->name, file_header->link_target); } + } else { + bb_perror_msg("inconsistent file header: " + "symlink without target %s", + file_header->name); + } break; case S_IFSOCK: case S_IFBLK: diff --git a/base/busybox/archival/tar.c b/base/busybox/archival/tar.c index 9e6571e..9243ae7 100644 --- a/base/busybox/archival/tar.c +++ b/base/busybox/archival/tar.c @@ -22,26 +22,6 @@ * * Licensed under GPLv2 or later, see file LICENSE in this source tree. */ -/* TODO: security with -C DESTDIR option can be enhanced. - * Consider tar file created via: - * $ tar cvf bug.tar anything.txt - * $ ln -s /tmp symlink - * $ tar --append -f bug.tar symlink - * $ rm symlink - * $ mkdir symlink - * $ tar --append -f bug.tar symlink/evil.py - * - * This will result in an archive which contains: - * $ tar --list -f bug.tar - * anything.txt - * symlink - * symlink/evil.py - * - * Untarring it puts evil.py in '/tmp' even if the -C DESTDIR is given. - * This doesn't feel right, and IIRC GNU tar doesn't do that. - * - * XXX actually this doesn't seem to have anything to do with -C DESTDIR; we're always vulnerable. - */ //config:config TAR //config: bool "tar"