all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Leo Famulari <leo@famulari.name>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: guix-devel@gnu.org
Subject: libarchive security fixes (was Re: Core-updates timeline)
Date: Sun, 2 Oct 2016 16:14:04 -0400	[thread overview]
Message-ID: <20161002201404.GA9126@jasmine> (raw)
In-Reply-To: <20161002185034.GA32485@jasmine>


[-- Attachment #1.1: Type: text/plain, Size: 1060 bytes --]

On Sun, Oct 02, 2016 at 02:50:34PM -0400, Leo Famulari wrote:
> On Sun, Oct 02, 2016 at 03:38:58PM +0200, Ludovic Courtès wrote:
> > We could wait an additional day for libarchive if it’s more convenient,
> > but maybe not longer than that.
> > 
> > What do you think would be the most convenient approach?
> 
> I will send a patch that cherry-picks what I think are the most
> important bug fixes. I can't guess when libarchive 3.2.2 will be
> released.

I've attached a patch.

It cherry-picks some fixes for some filesystem attacks and two overflows
that can be triggered with "crafted" input. The details are in the patch
files.

I understand if this approach of cherry-picking a handful of commits is
not acceptable. It's hard to judge the full impact of taking only these
changes, some of which a quite significant, without being familiar with
the libarchive code.

That's the reason why I've been waiting for a new upstream release. But
I figured I should at least try to get these bug fixes into the next
release of Guix :)

[-- Attachment #1.2: 0001-gnu-libarchive-Fix-several-security-issues.patch --]
[-- Type: text/plain, Size: 24051 bytes --]

From 042d5a7df4962c3b81fbfefa0027b6f1cf356b5f Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Sun, 2 Oct 2016 15:58:06 -0400
Subject: [PATCH] gnu: libarchive: Fix several security issues.

* gnu/packages/backup.scm (libarchive)[replacement]: New field.
(libarchive/fixed): New variable.
* gnu/packages/patches/libarchive-7zip-heap-overflow.patch,
gnu/packages/patches/libarchive-fix-symlink-check.patch,
gnu/packages/patches/libarchive-fix-filesystem-attacks.patch,
gnu/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch: New files.
* gnu/local.mk (dist_patch_DATA): Add them.
---
 gnu/local.mk                                       |   4 +
 gnu/packages/backup.scm                            |  12 +
 .../patches/libarchive-7zip-heap-overflow.patch    |  77 ++++
 .../libarchive-fix-filesystem-attacks.patch        | 445 +++++++++++++++++++++
 .../libarchive-safe_fprintf-buffer-overflow.patch  |  44 ++
 5 files changed, 582 insertions(+)
 create mode 100644 gnu/packages/patches/libarchive-7zip-heap-overflow.patch
 create mode 100644 gnu/packages/patches/libarchive-fix-filesystem-attacks.patch
 create mode 100644 gnu/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 4260a92..02cd680 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -622,6 +622,10 @@ dist_patch_DATA =						\
   %D%/packages/patches/liba52-link-with-libm.patch		\
   %D%/packages/patches/liba52-set-soname.patch			\
   %D%/packages/patches/liba52-use-mtune-not-mcpu.patch		\
+  %D%/packages/patches/libarchive-7zip-heap-overflow.patch	\
+  %D%/packages/patches/libarchive-fix-symlink-check.patch	\
+  %D%/packages/patches/libarchive-fix-filesystem-attacks.patch	\
+  %D%/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch	\
   %D%/packages/patches/libbonobo-activation-test-race.patch	\
   %D%/packages/patches/libcanberra-sound-theme-freedesktop.patch \
   %D%/packages/patches/libcmis-fix-test-onedrive.patch		\
diff --git a/gnu/packages/backup.scm b/gnu/packages/backup.scm
index c6f1321..797c06e 100644
--- a/gnu/packages/backup.scm
+++ b/gnu/packages/backup.scm
@@ -172,6 +172,7 @@ backups (called chunks) to allow easy burning to CD/DVD.")
 (define-public libarchive
   (package
     (name "libarchive")
+    (replacement libarchive/fixed)
     (version "3.2.1")
     (source
      (origin
@@ -227,6 +228,17 @@ archive.  In particular, note that there is currently no built-in support for
 random access nor for in-place modification.")
     (license license:bsd-2)))
 
+(define libarchive/fixed
+  (package
+    (inherit libarchive)
+    (source (origin
+              (inherit (package-source libarchive))
+              (patches (search-patches
+                         "libarchive-7zip-heap-overflow.patch"
+                         "libarchive-fix-symlink-check.patch"
+                         "libarchive-fix-filesystem-attacks.patch"
+                         "libarchive-safe_fprintf-buffer-overflow.patch"))))))
+
 (define-public rdup
   (package
     (name "rdup")
diff --git a/gnu/packages/patches/libarchive-7zip-heap-overflow.patch b/gnu/packages/patches/libarchive-7zip-heap-overflow.patch
new file mode 100644
index 0000000..bef628f
--- /dev/null
+++ b/gnu/packages/patches/libarchive-7zip-heap-overflow.patch
@@ -0,0 +1,77 @@
+Fix buffer overflow reading 7Zip files:
+
+https://github.com/libarchive/libarchive/issues/761
+
+Patch copied from upstream repository:
+
+https://github.com/libarchive/libarchive/commit/7f17c791dcfd8c0416e2cd2485b19410e47ef126
+
+From 7f17c791dcfd8c0416e2cd2485b19410e47ef126 Mon Sep 17 00:00:00 2001
+From: Tim Kientzle <kientzle@acm.org>
+Date: Sun, 18 Sep 2016 18:14:58 -0700
+Subject: [PATCH] Issue 761:  Heap overflow reading corrupted 7Zip files
+
+The sample file that demonstrated this had multiple 'EmptyStream'
+attributes.  The first one ended up being used to calculate
+certain statistics, then was overwritten by the second which
+was incompatible with those statistics.
+
+The fix here is to reject any header with multiple EmptyStream
+attributes.  While here, also reject headers with multiple
+EmptyFile, AntiFile, Name, or Attributes markers.
+---
+ libarchive/archive_read_support_format_7zip.c | 10 ++++++++++
+ 1 file changed, 10 insertions(+)
+
+diff --git a/libarchive/archive_read_support_format_7zip.c b/libarchive/archive_read_support_format_7zip.c
+index 1dfe52b..c0a536c 100644
+--- a/libarchive/archive_read_support_format_7zip.c
++++ b/libarchive/archive_read_support_format_7zip.c
+@@ -2431,6 +2431,8 @@ read_Header(struct archive_read *a, struct _7z_header_info *h,
+ 
+ 		switch (type) {
+ 		case kEmptyStream:
++			if (h->emptyStreamBools != NULL)
++				return (-1);
+ 			h->emptyStreamBools = calloc((size_t)zip->numFiles,
+ 			    sizeof(*h->emptyStreamBools));
+ 			if (h->emptyStreamBools == NULL)
+@@ -2451,6 +2453,8 @@ read_Header(struct archive_read *a, struct _7z_header_info *h,
+ 					return (-1);
+ 				break;
+ 			}
++			if (h->emptyFileBools != NULL)
++				return (-1);
+ 			h->emptyFileBools = calloc(empty_streams,
+ 			    sizeof(*h->emptyFileBools));
+ 			if (h->emptyFileBools == NULL)
+@@ -2465,6 +2469,8 @@ read_Header(struct archive_read *a, struct _7z_header_info *h,
+ 					return (-1);
+ 				break;
+ 			}
++			if (h->antiBools != NULL)
++				return (-1);
+ 			h->antiBools = calloc(empty_streams,
+ 			    sizeof(*h->antiBools));
+ 			if (h->antiBools == NULL)
+@@ -2491,6 +2497,8 @@ read_Header(struct archive_read *a, struct _7z_header_info *h,
+ 			if ((ll & 1) || ll < zip->numFiles * 4)
+ 				return (-1);
+ 
++			if (zip->entry_names != NULL)
++				return (-1);
+ 			zip->entry_names = malloc(ll);
+ 			if (zip->entry_names == NULL)
+ 				return (-1);
+@@ -2543,6 +2551,8 @@ read_Header(struct archive_read *a, struct _7z_header_info *h,
+ 			if ((p = header_bytes(a, 2)) == NULL)
+ 				return (-1);
+ 			allAreDefined = *p;
++			if (h->attrBools != NULL)
++				return (-1);
+ 			h->attrBools = calloc((size_t)zip->numFiles,
+ 			    sizeof(*h->attrBools));
+ 			if (h->attrBools == NULL)
+-- 
+2.10.0
+
diff --git a/gnu/packages/patches/libarchive-fix-filesystem-attacks.patch b/gnu/packages/patches/libarchive-fix-filesystem-attacks.patch
new file mode 100644
index 0000000..bce63d5
--- /dev/null
+++ b/gnu/packages/patches/libarchive-fix-filesystem-attacks.patch
@@ -0,0 +1,445 @@
+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.
diff --git a/gnu/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch b/gnu/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch
new file mode 100644
index 0000000..0e70ac9
--- /dev/null
+++ b/gnu/packages/patches/libarchive-safe_fprintf-buffer-overflow.patch
@@ -0,0 +1,44 @@
+Fixes this buffer overflow:
+https://github.com/libarchive/libarchive/commit/e37b620fe8f14535d737e89a4dcabaed4517bf1a
+
+Patch copied from upstream source repository:
+https://github.com/libarchive/libarchive/commit/e37b620fe8f14535d737e89a4dcabaed4517bf1a
+
+From e37b620fe8f14535d737e89a4dcabaed4517bf1a Mon Sep 17 00:00:00 2001
+From: Tim Kientzle <kientzle@acm.org>
+Date: Sun, 21 Aug 2016 10:51:43 -0700
+Subject: [PATCH] Issue #767:  Buffer overflow printing a filename
+
+The safe_fprintf function attempts to ensure clean output for an
+arbitrary sequence of bytes by doing a trial conversion of the
+multibyte characters to wide characters -- if the resulting wide
+character is printable then we pass through the corresponding bytes
+unaltered, otherwise, we convert them to C-style ASCII escapes.
+
+The stack trace in Issue #767 suggest that the 20-byte buffer
+was getting overflowed trying to format a non-printable multibyte
+character.  This should only happen if there is a valid multibyte
+character of more than 5 bytes that was unprintable.  (Each byte
+would get expanded to a four-charcter octal-style escape of the form
+"\123" resulting in >20 characters for the >5 byte multibyte character.)
+
+I've not been able to reproduce this, but have expanded the conversion
+buffer to 128 bytes on the belief that no multibyte character set
+has a single character of more than 32 bytes.
+---
+ tar/util.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/tar/util.c b/tar/util.c
+index 9ff22f2..2b4aebe 100644
+--- a/tar/util.c
++++ b/tar/util.c
+@@ -182,7 +182,7 @@ safe_fprintf(FILE *f, const char *fmt, ...)
+ 		}
+ 
+ 		/* If our output buffer is full, dump it and keep going. */
+-		if (i > (sizeof(outbuff) - 20)) {
++		if (i > (sizeof(outbuff) - 128)) {
+ 			outbuff[i] = '\0';
+ 			fprintf(f, "%s", outbuff);
+ 			i = 0;
-- 
2.10.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

  reply	other threads:[~2016-10-02 20:14 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-20  4:56 [PATCH 0/2] Perl: Enable threading support Ben Woodcroft
2016-09-20  4:56 ` [PATCH 1/2] gnu: perl: Split configure phase Ben Woodcroft
2016-09-24  5:02   ` Ludovic Courtès
2016-09-20  4:56 ` [PATCH 2/2] gnu: perl: Enable threading support Ben Woodcroft
2016-09-24  5:05   ` Ludovic Courtès
2016-09-26 10:03     ` Ben Woodcroft
2016-10-01 13:22       ` Ludovic Courtès
2016-10-01 16:40         ` Core-updates timeline (was: Re: [PATCH 2/2] gnu: perl: Enable threading support.) Leo Famulari
2016-10-02 13:38           ` Core-updates timeline Ludovic Courtès
2016-10-02 18:50             ` Leo Famulari
2016-10-02 20:14               ` Leo Famulari [this message]
2016-10-03 16:10                 ` libarchive security fixes (was Re: Core-updates timeline) Ludovic Courtès
2016-10-03 18:14                   ` Leo Famulari
2016-10-07 20:16             ` Core-updates timeline Ludovic Courtès
2016-09-20 20:58 ` [PATCH 0/2] Perl: Enable threading support Eric Bavier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161002201404.GA9126@jasmine \
    --to=leo@famulari.name \
    --cc=guix-devel@gnu.org \
    --cc=ludo@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.