nixo
/
guix-devel
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity
guix-devel/gnu/packages/patches/libarchive-fix-filesystem-a...

446 lines
14 KiB
Diff
Raw Blame History

This patch fixes two bugs that allow attackers to overwrite or change
the permissions of arbitrary files:
https://github.com/libarchive/libarchive/issues/745
https://github.com/libarchive/libarchive/issues/746
Patch copied from upstream repository:
https://github.com/libarchive/libarchive/commit/dfd6b54ce33960e420fb206d8872fb759b577ad9
From dfd6b54ce33960e420fb206d8872fb759b577ad9 Mon Sep 17 00:00:00 2001
From: Tim Kientzle <kientzle@acm.org>
Date: Sun, 11 Sep 2016 13:21:57 -0700
Subject: [PATCH] Fixes for Issue #745 and Issue #746 from Doran Moppert.
---
libarchive/archive_write_disk_posix.c | 294 ++++++++++++++++++++++++++--------
1 file changed, 227 insertions(+), 67 deletions(-)
diff --git a/libarchive/archive_write_disk_posix.c b/libarchive/archive_write_disk_posix.c
index 8f0421e..abe1a86 100644
--- a/libarchive/archive_write_disk_posix.c
+++ b/libarchive/archive_write_disk_posix.c
@@ -326,12 +326,14 @@ struct archive_write_disk {
#define HFS_BLOCKS(s) ((s) >> 12)
+static int check_symlinks_fsobj(char *path, int *error_number, struct archive_string *error_string, int flags);
static int check_symlinks(struct archive_write_disk *);
static int create_filesystem_object(struct archive_write_disk *);
static struct fixup_entry *current_fixup(struct archive_write_disk *, const char *pathname);
#if defined(HAVE_FCHDIR) && defined(PATH_MAX)
static void edit_deep_directories(struct archive_write_disk *ad);
#endif
+static int cleanup_pathname_fsobj(char *path, int *error_number, struct archive_string *error_string, int flags);
static int cleanup_pathname(struct archive_write_disk *);
static int create_dir(struct archive_write_disk *, char *);
static int create_parent_dir(struct archive_write_disk *, char *);
@@ -2014,6 +2016,10 @@ create_filesystem_object(struct archive_write_disk *a)
const char *linkname;
mode_t final_mode, mode;
int r;
+ /* these for check_symlinks_fsobj */
+ char *linkname_copy; /* non-const copy of linkname */
+ struct archive_string error_string;
+ int error_number;
/* We identify hard/symlinks according to the link names. */
/* Since link(2) and symlink(2) don't handle modes, we're done here. */
@@ -2022,6 +2028,27 @@ create_filesystem_object(struct archive_write_disk *a)
#if !HAVE_LINK
return (EPERM);
#else
+ archive_string_init(&error_string);
+ linkname_copy = strdup(linkname);
+ if (linkname_copy == NULL) {
+ return (EPERM);
+ }
+ /* TODO: consider using the cleaned-up path as the link target? */
+ r = cleanup_pathname_fsobj(linkname_copy, &error_number, &error_string, a->flags);
+ if (r != ARCHIVE_OK) {
+ archive_set_error(&a->archive, error_number, "%s", error_string.s);
+ free(linkname_copy);
+ /* EPERM is more appropriate than error_number for our callers */
+ return (EPERM);
+ }
+ r = check_symlinks_fsobj(linkname_copy, &error_number, &error_string, a->flags);
+ if (r != ARCHIVE_OK) {
+ archive_set_error(&a->archive, error_number, "%s", error_string.s);
+ free(linkname_copy);
+ /* EPERM is more appropriate than error_number for our callers */
+ return (EPERM);
+ }
+ free(linkname_copy);
r = link(linkname, a->name) ? errno : 0;
/*
* New cpio and pax formats allow hardlink entries
@@ -2362,115 +2389,228 @@ current_fixup(struct archive_write_disk *a, const char *pathname)
* recent paths.
*/
/* TODO: Extend this to support symlinks on Windows Vista and later. */
+
+/*
+ * Checks the given path to see if any elements along it are symlinks. Returns
+ * ARCHIVE_OK if there are none, otherwise puts an error in errmsg.
+ */
static int
-check_symlinks(struct archive_write_disk *a)
+check_symlinks_fsobj(char *path, int *error_number, struct archive_string *error_string, int flags)
{
#if !defined(HAVE_LSTAT)
/* Platform doesn't have lstat, so we can't look for symlinks. */
(void)a; /* UNUSED */
+ (void)path; /* UNUSED */
+ (void)error_number; /* UNUSED */
+ (void)error_string; /* UNUSED */
+ (void)flags; /* UNUSED */
return (ARCHIVE_OK);
#else
- char *pn;
+ int res = ARCHIVE_OK;
+ char *tail;
+ char *head;
+ int last;
char c;
int r;
struct stat st;
+ int restore_pwd;
+
+ /* Nothing to do here if name is empty */
+ if(path[0] == '\0')
+ return (ARCHIVE_OK);
/*
* Guard against symlink tricks. Reject any archive entry whose
* destination would be altered by a symlink.
+ *
+ * Walk the filename in chunks separated by '/'. For each segment:
+ * - if it doesn't exist, continue
+ * - if it's symlink, abort or remove it
+ * - if it's a directory and it's not the last chunk, cd into it
+ * As we go:
+ * head points to the current (relative) path
+ * tail points to the temporary \0 terminating the segment we're currently examining
+ * c holds what used to be in *tail
+ * last is 1 if this is the last tail
*/
- /* Whatever we checked last time doesn't need to be re-checked. */
- pn = a->name;
- if (archive_strlen(&(a->path_safe)) > 0) {
- char *p = a->path_safe.s;
- while ((*pn != '\0') && (*p == *pn))
- ++p, ++pn;
- }
+ restore_pwd = open(".", O_RDONLY | O_BINARY | O_CLOEXEC);
+ __archive_ensure_cloexec_flag(restore_pwd);
+ if (restore_pwd < 0)
+ return (ARCHIVE_FATAL);
+ head = path;
+ tail = path;
+ last = 0;
+ /* TODO: reintroduce a safe cache here? */
/* Skip the root directory if the path is absolute. */
- if(pn == a->name && pn[0] == '/')
- ++pn;
- c = pn[0];
- /* Keep going until we've checked the entire name. */
- while (pn[0] != '\0' && (pn[0] != '/' || pn[1] != '\0')) {
+ if(tail == path && tail[0] == '/')
+ ++tail;
+ /* Keep going until we've checked the entire name.
+ * head, tail, path all alias the same string, which is
+ * temporarily zeroed at tail, so be careful restoring the
+ * stashed (c=tail[0]) for error messages.
+ * Exiting the loop with break is okay; continue is not.
+ */
+ while (!last) {
+ /* Skip the separator we just consumed, plus any adjacent ones */
+ while (*tail == '/')
+ ++tail;
/* Skip the next path element. */
- while (*pn != '\0' && *pn != '/')
- ++pn;
- c = pn[0];
- pn[0] = '\0';
+ while (*tail != '\0' && *tail != '/')
+ ++tail;
+ /* is this the last path component? */
+ last = (tail[0] == '\0') || (tail[0] == '/' && tail[1] == '\0');
+ /* temporarily truncate the string here */
+ c = tail[0];
+ tail[0] = '\0';
/* Check that we haven't hit a symlink. */
- r = lstat(a->name, &st);
+ r = lstat(head, &st);
if (r != 0) {
+ tail[0] = c;
/* We've hit a dir that doesn't exist; stop now. */
if (errno == ENOENT) {
break;
} else {
- /* Note: This effectively disables deep directory
+ /* Treat any other error as fatal - best to be paranoid here
+ * Note: This effectively disables deep directory
* support when security checks are enabled.
* Otherwise, very long pathnames that trigger
* an error here could evade the sandbox.
* TODO: We could do better, but it would probably
* require merging the symlink checks with the
* deep-directory editing. */
- return (ARCHIVE_FAILED);
+ if (error_number) *error_number = errno;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Could not stat %s",
+ path);
+ res = ARCHIVE_FAILED;
+ break;
+ }
+ } else if (S_ISDIR(st.st_mode)) {
+ if (!last) {
+ if (chdir(head) != 0) {
+ tail[0] = c;
+ if (error_number) *error_number = errno;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Could not chdir %s",
+ path);
+ res = (ARCHIVE_FATAL);
+ break;
+ }
+ /* Our view is now from inside this dir: */
+ head = tail + 1;
}
} else if (S_ISLNK(st.st_mode)) {
- if (c == '\0') {
+ if (last) {
/*
* Last element is symlink; remove it
* so we can overwrite it with the
* item being extracted.
*/
- if (unlink(a->name)) {
- archive_set_error(&a->archive, errno,
- "Could not remove symlink %s",
- a->name);
- pn[0] = c;
- return (ARCHIVE_FAILED);
+ if (unlink(head)) {
+ tail[0] = c;
+ if (error_number) *error_number = errno;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Could not remove symlink %s",
+ path);
+ res = ARCHIVE_FAILED;
+ break;
}
- a->pst = NULL;
/*
* Even if we did remove it, a warning
* is in order. The warning is silly,
* though, if we're just replacing one
* symlink with another symlink.
*/
- if (!S_ISLNK(a->mode)) {
- archive_set_error(&a->archive, 0,
- "Removing symlink %s",
- a->name);
+ tail[0] = c;
+ /* FIXME: not sure how important this is to restore
+ if (!S_ISLNK(path)) {
+ if (error_number) *error_number = 0;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Removing symlink %s",
+ path);
}
+ */
/* Symlink gone. No more problem! */
- pn[0] = c;
- return (0);
- } else if (a->flags & ARCHIVE_EXTRACT_UNLINK) {
+ res = ARCHIVE_OK;
+ break;
+ } else if (flags & ARCHIVE_EXTRACT_UNLINK) {
/* User asked us to remove problems. */
- if (unlink(a->name) != 0) {
- archive_set_error(&a->archive, 0,
- "Cannot remove intervening symlink %s",
- a->name);
- pn[0] = c;
- return (ARCHIVE_FAILED);
+ if (unlink(head) != 0) {
+ tail[0] = c;
+ if (error_number) *error_number = 0;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Cannot remove intervening symlink %s",
+ path);
+ res = ARCHIVE_FAILED;
+ break;
}
- a->pst = NULL;
+ tail[0] = c;
} else {
- archive_set_error(&a->archive, 0,
- "Cannot extract through symlink %s",
- a->name);
- pn[0] = c;
- return (ARCHIVE_FAILED);
+ tail[0] = c;
+ if (error_number) *error_number = 0;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Cannot extract through symlink %s",
+ path);
+ res = ARCHIVE_FAILED;
+ break;
}
}
- pn[0] = c;
- if (pn[0] != '\0')
- pn++; /* Advance to the next segment. */
+ /* be sure to always maintain this */
+ tail[0] = c;
+ if (tail[0] != '\0')
+ tail++; /* Advance to the next segment. */
}
- pn[0] = c;
- /* We've checked and/or cleaned the whole path, so remember it. */
- archive_strcpy(&a->path_safe, a->name);
- return (ARCHIVE_OK);
+ /* Catches loop exits via break */
+ tail[0] = c;
+#ifdef HAVE_FCHDIR
+ /* If we changed directory above, restore it here. */
+ if (restore_pwd >= 0) {
+ r = fchdir(restore_pwd);
+ if (r != 0) {
+ if(error_number) *error_number = errno;
+ if(error_string)
+ archive_string_sprintf(error_string,
+ "chdir() failure");
+ }
+ close(restore_pwd);
+ restore_pwd = -1;
+ if (r != 0) {
+ res = (ARCHIVE_FATAL);
+ }
+ }
+#endif
+ /* TODO: reintroduce a safe cache here? */
+ return res;
#endif
}
+/*
+ * Check a->name for symlinks, returning ARCHIVE_OK if its clean, otherwise
+ * calls archive_set_error and returns ARCHIVE_{FATAL,FAILED}
+ */
+static int
+check_symlinks(struct archive_write_disk *a)
+{
+ struct archive_string error_string;
+ int error_number;
+ int rc;
+ archive_string_init(&error_string);
+ rc = check_symlinks_fsobj(a->name, &error_number, &error_string, a->flags);
+ if (rc != ARCHIVE_OK) {
+ archive_set_error(&a->archive, error_number, "%s", error_string.s);
+ }
+ archive_string_free(&error_string);
+ a->pst = NULL; /* to be safe */
+ return rc;
+}
+
+
#if defined(__CYGWIN__)
/*
* 1. Convert a path separator from '\' to '/' .
@@ -2544,15 +2684,17 @@ cleanup_pathname_win(struct archive_write_disk *a)
* is set) if the path is absolute.
*/
static int
-cleanup_pathname(struct archive_write_disk *a)
+cleanup_pathname_fsobj(char *path, int *error_number, struct archive_string *error_string, int flags)
{
char *dest, *src;
char separator = '\0';
- dest = src = a->name;
+ dest = src = path;
if (*src == '\0') {
- archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC,
- "Invalid empty pathname");
+ if (error_number) *error_number = ARCHIVE_ERRNO_MISC;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Invalid empty pathname");
return (ARCHIVE_FAILED);
}
@@ -2561,9 +2703,11 @@ cleanup_pathname(struct archive_write_disk *a)
#endif
/* Skip leading '/'. */
if (*src == '/') {
- if (a->flags & ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS) {
- archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC,
- "Path is absolute");
+ if (flags & ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS) {
+ if (error_number) *error_number = ARCHIVE_ERRNO_MISC;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Path is absolute");
return (ARCHIVE_FAILED);
}
@@ -2590,10 +2734,11 @@ cleanup_pathname(struct archive_write_disk *a)
} else if (src[1] == '.') {
if (src[2] == '/' || src[2] == '\0') {
/* Conditionally warn about '..' */
- if (a->flags & ARCHIVE_EXTRACT_SECURE_NODOTDOT) {
- archive_set_error(&a->archive,
- ARCHIVE_ERRNO_MISC,
- "Path contains '..'");
+ if (flags & ARCHIVE_EXTRACT_SECURE_NODOTDOT) {
+ if (error_number) *error_number = ARCHIVE_ERRNO_MISC;
+ if (error_string)
+ archive_string_sprintf(error_string,
+ "Path contains '..'");
return (ARCHIVE_FAILED);
}
}
@@ -2624,7 +2769,7 @@ cleanup_pathname(struct archive_write_disk *a)
* We've just copied zero or more path elements, not including the
* final '/'.
*/
- if (dest == a->name) {
+ if (dest == path) {
/*
* Nothing got copied. The path must have been something
* like '.' or '/' or './' or '/././././/./'.
@@ -2639,6 +2784,21 @@ cleanup_pathname(struct archive_write_disk *a)
return (ARCHIVE_OK);
}
+static int
+cleanup_pathname(struct archive_write_disk *a)
+{
+ struct archive_string error_string;
+ int error_number;
+ int rc;
+ archive_string_init(&error_string);
+ rc = cleanup_pathname_fsobj(a->name, &error_number, &error_string, a->flags);
+ if (rc != ARCHIVE_OK) {
+ archive_set_error(&a->archive, error_number, "%s", error_string.s);
+ }
+ archive_string_free(&error_string);
+ return rc;
+}
+
/*
* Create the parent directory of the specified path, assuming path
* is already in mutable storage.
Reference in New Issue View Git Blame Copy Permalink