[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-cpio] [PATCH] fix 1-byte out-of-bounds write
From: |
Kristýna Streitová |
Subject: |
Re: [Bug-cpio] [PATCH] fix 1-byte out-of-bounds write |
Date: |
Sun, 2 Apr 2017 12:53:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
Hello Pavel,
It seems that we encountered a regression caused by this patch.
Basically, there are 2 problems:
1) file_hdr.c_namesize is not defined for tar and ustar archive formats.
Therefore we have a random memory here and that's the reason why it's
reproducible randomly.
2) file_hdr.c_name uses static memory [1] for tar and ustar archive
formats. Therefore 'xrealloc(file_hdr.c_name, 2)' call causes a dump for
these archive types as we are trying to realloc static memory.
-----
Patch
-----
The proposed patch (attached) includes adjusting the initial patch to
not call xrealloc for tar and ustar archives:
+ if (archive_format != arf_tar && archive_format != arf_ustar
+ && 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);
The patch is still correct for "fix 1-byte out-of-bounds write" issue
(we have to be sure that the allocated NAME buffer has a capacity at
least 2 bytes) as static char array for tar and ustar has size 2 at
least [1]:
static char hold_tar_filename[TARNAMESIZE + TARPREFIXSIZE + 2];
-------------------
Various reproducers
-------------------
1)
tar --no-recursion -c . | cpio -i
2)
touch empty-file; echo empty-file | cpio --format=tar --create | cpio
--format=tar --list; rm empty-file
3)
mkdir ~/cpio-test
cd ~/cpio-test
mkdir topack
mkdir unpacked
mkdir archive
echo "test" > topack/test
cd ~/cpio-test/topack && ls | cpio -o -H tar -O
~/cpio-test/archive/test_tar 2>/dev/null
cd ~/cpio-test/unpacked && cpio -i -H tar -I
~/cpio-test/archive/test_tar 2> /dev/null
-----
Later I've also found out that it was reported for Debian [2] too.
Best regards,
Kristyna Streitova
[1] http://git.savannah.gnu.org/cgit/cpio.git/tree/src/tar.c line 65
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851632
Dne 13.2.2017 v 11:01 Pavel Raiskup napsal(a):
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)
cpio-2.12-out_of_bounds_write.patch
Description: Text Data
- Re: [Bug-cpio] [PATCH] fix 1-byte out-of-bounds write,
Kristýna Streitová <=