tags 802702 + patch
forwarded 802702 https://bugs.busybox.net/show_bug.cgi?id=8411#c2
thanks

Patch attached & sent upstream.


Regards,

-- 
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      la...@debian.org / chris-lamb.co.uk
       `-
diff --git a/archival/libarchive/data_extract_all.c 
b/archival/libarchive/data_extract_all.c
index 45776dc..534c7fc 100644
--- a/archival/libarchive/data_extract_all.c
+++ b/archival/libarchive/data_extract_all.c
@@ -142,7 +142,40 @@ void FAST_FUNC data_extract_all(archive_handle_t 
*archive_handle)
                break;
        case S_IFLNK:
                /* Symlink */
-//TODO: what if file_header->link_target == NULL (say, corrupted tarball?)
+
+               /* To avoid a directory traversal attack via symlinks, for
+                * certain link targets first write placeholder regular files
+                * containing the intended target and mark to replace them
+                * later.
+                *
+                * For example, consider a .tar 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 that contains:
+                *
+                *  $ tar --list -f bug.tar
+                *  anything.txt
+                *  symlink [-> /tmp]
+                *  symlink/evil.py
+                *
+                * Untarring bug.tar would otherwise place evil.py in '/tmp'.
+               */
+               if ((!strncmp(file_header->link_target, "/", 1))
+                || (strstr(file_header->link_target, "../"))) {
+                       dst_fd = xopen(file_header->name, O_WRONLY | O_CREAT | 
O_EXCL);
+                       xwrite(dst_fd, file_header->link_target,
+                                       strlen(file_header->link_target));
+                       close(dst_fd);
+                       llist_add_to_end(&archive_handle->symlink_placeholders,
+                                       strdup(file_header->name));
+                       break;
+               }
                res = symlink(file_header->link_target, file_header->name);
                if ((res == -1)
                 && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET)
diff --git a/archival/tar.c b/archival/tar.c
index bd61abd..49fc93f 100644
--- a/archival/tar.c
+++ b/archival/tar.c
@@ -22,24 +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.
- */
 
 //config:config TAR
 //config:      bool "tar"
@@ -309,6 +291,20 @@ static void chksum_and_xwrite(int fd, struct tar_header_t* 
hp)
        xwrite(fd, hp, sizeof(*hp));
 }
 
+static void replace_symlink_placeholders(llist_t *list) {
+       while (list) {
+               char *linkpath = list->data;
+               char *target = xmalloc_xopen_read_close(linkpath, NULL);
+               if (unlink(linkpath) == -1)
+                       bb_error_msg_and_die("can't remove placeholder %s",
+                               linkpath);
+               if (symlink(target, linkpath) == -1)
+                       bb_error_msg_and_die("can't create link: %s -> %s",
+                               linkpath, target);
+               list = list->link;
+       }
+}
+
 #if ENABLE_FEATURE_TAR_GNU_EXTENSIONS
 static void writeLongname(int fd, int type, const char *name, int dir)
 {
@@ -1205,6 +1201,8 @@ int tar_main(int argc UNUSED_PARAM, char **argv)
        while (get_header_tar(tar_handle) == EXIT_SUCCESS)
                bb_got_signal = EXIT_SUCCESS; /* saw at least one header, good 
*/
 
+       replace_symlink_placeholders(tar_handle->symlink_placeholders);
+
        /* Check that every file that should have been extracted was */
        while (tar_handle->accept) {
                if (!find_list_entry(tar_handle->reject, 
tar_handle->accept->data)
diff --git a/include/bb_archive.h b/include/bb_archive.h
index b82cfd8..bcaed7f 100644
--- a/include/bb_archive.h
+++ b/include/bb_archive.h
@@ -64,6 +64,9 @@ typedef struct archive_handle_t {
        /* Currently processed file's header */
        file_header_t *file_header;
 
+       /* List of symlink placeholders */
+       llist_t *symlink_placeholders;
+
        /* Process the header component, e.g. tar -t */
        void FAST_FUNC (*action_header)(const file_header_t *);
 
@@ -182,6 +185,7 @@ char get_header_ar(archive_handle_t *archive_handle) 
FAST_FUNC;
 char get_header_cpio(archive_handle_t *archive_handle) FAST_FUNC;
 char get_header_tar(archive_handle_t *archive_handle) FAST_FUNC;
 char get_header_tar_gz(archive_handle_t *archive_handle) FAST_FUNC;
+char get_header_tar_xz(archive_handle_t *archive_handle) FAST_FUNC;
 char get_header_tar_bz2(archive_handle_t *archive_handle) FAST_FUNC;
 char get_header_tar_lzma(archive_handle_t *archive_handle) FAST_FUNC;
 

Reply via email to