Just trying to ping with re-based patch.
Even though this is probably not very serious security issue, it might
lead to crash .. and I'm pushed to fix this downstream (Debian and other distros
already applied this patch and our clients are also requesting this).
I was thinking what to do better WRT original issue, but doing anything
more systematic in CPIO/PAXUTILS, the change would be probably much
larger. OTOH, I'm fine to have a look if this is considered too bad fix.
Thanks for having a look!
Pavel
On Tuesday, January 26, 2016 11:17:54 PM CET Pavel Raiskup wrote:
> Other calls to cpio_safer_name_suffix seem to be safe.
>
> * src/copyin.c (process_copy_in): Make sure that file_hdr.c_name
> has at least two bytes allocated.
> * src/util.c (cpio_safer_name_suffix): Document that use of this
> function requires to be careful.
> ---
> src/copyin.c | 2 ++
> src/util.c | 5 ++++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/copyin.c b/src/copyin.c
> index cde911e..032d35f 100644
> --- a/src/copyin.c
> +++ b/src/copyin.c
> @@ -1385,6 +1385,8 @@ process_copy_in ()
> break;
> }
>
> + if (file_hdr.c_namesize <= 1)
> + file_hdr.c_name = xrealloc(file_hdr.c_name, 2);
> cpio_safer_name_suffix (file_hdr.c_name, false, !no_abs_paths_flag,
> false);
>
> diff --git a/src/util.c b/src/util.c
> index 6ff6032..2763ac1 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -1411,7 +1411,10 @@ set_file_times (int fd,
> }
>
> /* Do we have to ignore absolute paths, and if so, does the filename
> - have an absolute path? */
> + have an absolute path?
> + Before calling this function make sure that the allocated NAME buffer has
> + capacity at least 2 bytes to allow us to store the "." string inside. */
> +
> void
> cpio_safer_name_suffix (char *name, bool link_target, bool absolute_names,
> bool strip_leading_dots)
>
>From e9a408ea5eb73bd584ff817e1a2db300f83cc10b Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <prais...@redhat.com>
Date: Tue, 26 Jan 2016 23:17:54 +0100
Subject: [PATCH] fix 1-byte out-of-bounds write
Other calls to cpio_safer_name_suffix seem to be safe.
* src/copyin.c (process_copy_in): Make sure that file_hdr.c_name
has at least two bytes allocated.
* src/util.c (cpio_safer_name_suffix): Document that use of this
function requires to be careful.
---
src/copyin.c | 5 ++++-
src/util.c | 8 +++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/copyin.c b/src/copyin.c
index 6a306ee..87c26ae 100644
--- a/src/copyin.c
+++ b/src/copyin.c
@@ -1337,9 +1337,12 @@ process_copy_in ()
break;
}
+ if (file_hdr.c_namesize <= 1)
+ file_hdr.c_name = xrealloc(file_hdr.c_name, 2);
+
cpio_safer_name_suffix (file_hdr.c_name, false, !no_abs_paths_flag,
false);
-
+
/* Does the file name match one of the given patterns? */
if (num_patterns <= 0)
skip_file = false;
diff --git a/src/util.c b/src/util.c
index d78d576..9211aa0 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1410,7 +1410,9 @@ set_file_times (int fd,
}
/* Do we have to ignore absolute paths, and if so, does the filename
- have an absolute path? */
+ have an absolute path? Before calling this function make sure that the
+ allocated NAME buffer has capacity at least 2 bytes. */
+
void
cpio_safer_name_suffix (char *name, bool link_target, bool absolute_names,
bool strip_leading_dots)
@@ -1425,6 +1427,10 @@ cpio_safer_name_suffix (char *name, bool link_target, bool absolute_names,
++p;
}
if (p != name)
+ /* The 'p' string is shortened version of 'name' with one exception; when
+ the 'name' points to an empty string (buffer where name[0] == '\0') the
+ 'p' then points to static string ".". So caller needs to ensure there
+ are at least two bytes available in 'name' buffer so memmove succeeds. */
memmove (name, p, (size_t)(strlen (p) + 1));
}
--
2.9.3
