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

[Pavel Raiskup]

Hi Krystýno, thanks for reporting back!

On Sunday, April 2, 2017 12:53:47 PM CEST Kristýna Streitová wrote:
> 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.

It means that we should initialize c_namesize I guess, even though this is
different issue.

> 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.

Yes, that's something which complicated fixing and reading of the code, so
the attached patch should rather use buffer on heap.  It is still not very
clean approach, and not as trivial patch as before - but the code should be
a bit cleaner I hope.

As the patch drops one hack which comes from Debian (no references in
git-history), I'm curious whether there are some test-cases in Debian
repos so we can prove no regressions were added?

Pavel

> 
> -----
> 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)
> >>
> >
>

0001-CVE-2016-2037-1-byte-out-of-bounds-write.patch
Description: Text Data