Re: [Bug-cpio] [PATCH] fix 1-byte out-of-bounds write

[Kristýna Streitová]

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