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) > >> > > >
>From 9e43d05e74ad76968890c95b3e16b5ea986da57e Mon Sep 17 00:00:00 2001 From: Pavel Raiskup <prais...@redhat.com> Date: Tue, 26 Jan 2016 23:17:54 +0100 Subject: [PATCH] CVE-2016-2037 - 1 byte out-of-bounds write Ensure that cpio_safer_name_suffix always works with dynamically allocated buffer, and that it has size of at least 32 bytes. Then, any call to cpio_safer_name_suffix is safe (it requires at least 2 bytes in the buffer). Also ensure that c_namesize is always correctly initialized (by cpio_set_c_name) to avoid undefined behavior when reading file_hdr.c_namesize (previously happened for tar archives). References: http://www.mail-archive.com/bug-cpio@gnu.org/msg00545.html * src/copyin.c (query_rename): Drop the hack, as we now work with dynamically allocated buffer. Use cpio_set_c_name. (create_defered_links_to_skipped): Use cpio_set_c_name rather than manual assignment. (read_name_from_file): New function to avoid C&P. (read_in_old_ascii, read_in_new_ascii, read_in_binary): Use read_name_from_file. (process_copy_in): Initialize file_hdr.c_namesize. * src/copyout.c (process_copy_out): Use cpio_set_c_name. * src/cpiohdr.h (cpio_set_c_name): New prototype. * src/tar.c (read_in_tar_header): Use cpio_set_c_name. * src/util.c (cpio_set_c_name): New function to set file_hdr->c_name and c_namesize from arbitrary string. (cpio_safer_name_suffix): Some docs fixes. * tests/inout.at: Also test copy-in, and try various formats. --- src/copyin.c | 68 +++++++++++++++++++++------------------------------------- src/copyout.c | 10 ++++----- src/cpiohdr.h | 1 + src/tar.c | 10 +++++---- src/util.c | 32 ++++++++++++++++++++++++++- tests/inout.at | 19 ++++++++++++++-- 6 files changed, 84 insertions(+), 56 deletions(-) diff --git a/src/copyin.c b/src/copyin.c index 6a306ee..b93207e 100644 --- a/src/copyin.c +++ b/src/copyin.c @@ -76,27 +76,12 @@ query_rename(struct cpio_file_stat* file_hdr, FILE *tty_in, FILE *tty_out, return -1; } else - /* Debian hack: file_hrd.c_name is sometimes set to - point to static memory by code in tar.c. This - causes a segfault. This has been fixed and an - additional check to ensure that the file name - is not too long has been added. (Reported by - Horst Knobloch.) This bug has been reported to - "bug-gnu-ut...@prep.ai.mit.edu". (99/1/6) -BEM */ { - if (archive_format != arf_tar && archive_format != arf_ustar) - { - free (file_hdr->c_name); - file_hdr->c_name = xstrdup (new_name.ds_string); - } + if (is_tar_filename_too_long (new_name.ds_string)) + error (0, 0, _("%s: file name too long"), + new_name.ds_string); else - { - if (is_tar_filename_too_long (new_name.ds_string)) - error (0, 0, _("%s: file name too long"), - new_name.ds_string); - else - strcpy (file_hdr->c_name, new_name.ds_string); - } + cpio_set_c_name (file_hdr, new_name.ds_string); } return 0; } @@ -342,8 +327,7 @@ create_defered_links_to_skipped (struct cpio_file_stat *file_hdr, d_prev->next = d->next; else deferments = d->next; - free (file_hdr->c_name); - file_hdr->c_name = xstrdup(d->header.c_name); + cpio_set_c_name (file_hdr, d->header.c_name); free_deferment (d); copyin_regular_file(file_hdr, in_file_des); return 0; @@ -1012,6 +996,22 @@ read_in_header (struct cpio_file_stat *file_hdr, int in_des) } } +static void +read_name_from_file (struct cpio_file_stat *file_hdr, int fd, uintmax_t len) +{ + static char *tmp_filename; + static size_t buflen; + + if (buflen < len) + { + buflen = len; + tmp_filename = xrealloc (tmp_filename, buflen); + } + + tape_buffered_read (tmp_filename, fd, len); + cpio_set_c_name (file_hdr, tmp_filename); +} + /* Fill in FILE_HDR by reading an old-format ASCII format cpio header from file descriptor IN_DES, except for the magic number, which is already filled in. */ @@ -1038,14 +1038,8 @@ read_in_old_ascii (struct cpio_file_stat *file_hdr, int in_des) file_hdr->c_rdev_min = minor (dev); file_hdr->c_mtime = FROM_OCTAL (ascii_header.c_mtime); - file_hdr->c_namesize = FROM_OCTAL (ascii_header.c_namesize); file_hdr->c_filesize = FROM_OCTAL (ascii_header.c_filesize); - - /* Read file name from input. */ - if (file_hdr->c_name != NULL) - free (file_hdr->c_name); - file_hdr->c_name = (char *) xmalloc (file_hdr->c_namesize + 1); - tape_buffered_read (file_hdr->c_name, in_des, (long) file_hdr->c_namesize); + read_name_from_file (file_hdr, in_des, FROM_OCTAL (ascii_header.c_namesize)); /* HP/UX cpio creates archives that look just like ordinary archives, but for devices it sets major = 0, minor = 1, and puts the @@ -1100,14 +1094,8 @@ read_in_new_ascii (struct cpio_file_stat *file_hdr, int in_des) file_hdr->c_dev_min = FROM_HEX (ascii_header.c_dev_min); file_hdr->c_rdev_maj = FROM_HEX (ascii_header.c_rdev_maj); file_hdr->c_rdev_min = FROM_HEX (ascii_header.c_rdev_min); - file_hdr->c_namesize = FROM_HEX (ascii_header.c_namesize); file_hdr->c_chksum = FROM_HEX (ascii_header.c_chksum); - - /* Read file name from input. */ - if (file_hdr->c_name != NULL) - free (file_hdr->c_name); - file_hdr->c_name = (char *) xmalloc (file_hdr->c_namesize); - tape_buffered_read (file_hdr->c_name, in_des, (long) file_hdr->c_namesize); + read_name_from_file (file_hdr, in_des, FROM_HEX (ascii_header.c_namesize)); /* In SVR4 ASCII format, the amount of space allocated for the header is rounded up to the next long-word, so we might need to drop @@ -1155,16 +1143,9 @@ read_in_binary (struct cpio_file_stat *file_hdr, file_hdr->c_rdev_min = minor (short_hdr->c_rdev); file_hdr->c_mtime = (unsigned long) short_hdr->c_mtimes[0] << 16 | short_hdr->c_mtimes[1]; - - file_hdr->c_namesize = short_hdr->c_namesize; file_hdr->c_filesize = (unsigned long) short_hdr->c_filesizes[0] << 16 | short_hdr->c_filesizes[1]; - - /* Read file name from input. */ - if (file_hdr->c_name != NULL) - free (file_hdr->c_name); - file_hdr->c_name = (char *) xmalloc (file_hdr->c_namesize); - tape_buffered_read (file_hdr->c_name, in_des, (long) file_hdr->c_namesize); + read_name_from_file (file_hdr, in_des, short_hdr->c_namesize); /* In binary mode, the amount of space allocated in the header for the filename is `c_namesize' rounded up to the next short-word, @@ -1245,6 +1226,7 @@ process_copy_in () read_pattern_file (); } file_hdr.c_name = NULL; + file_hdr.c_namesize = 0; if (rename_batch_file) { diff --git a/src/copyout.c b/src/copyout.c index 87c1932..9d192da 100644 --- a/src/copyout.c +++ b/src/copyout.c @@ -660,8 +660,7 @@ process_copy_out () cpio_safer_name_suffix (input_name.ds_string, false, !no_abs_paths_flag, true); #ifndef HPUX_CDF - file_hdr.c_name = input_name.ds_string; - file_hdr.c_namesize = strlen (input_name.ds_string) + 1; + cpio_set_c_name (&file_hdr, input_name.ds_string); #else if ( (archive_format != arf_tar) && (archive_format != arf_ustar) ) { @@ -670,16 +669,15 @@ process_copy_out () properly recreate the directory as hidden (in case the files of a directory go into the archive before the directory itself (e.g from "find ... -depth ... | cpio")). */ - file_hdr.c_name = add_cdf_double_slashes (input_name.ds_string); - file_hdr.c_namesize = strlen (file_hdr.c_name) + 1; + cpio_set_c_name (&file_hdr, + add_cdf_double_slashes (input_name.ds_string)); } else { /* We don't mark CDF's in tar files. We assume the "hidden" directory will always go into the archive before any of its files. */ - file_hdr.c_name = input_name.ds_string; - file_hdr.c_namesize = strlen (input_name.ds_string) + 1; + cpio_set_c_name (&file_hdr, input_name.ds_string); } #endif diff --git a/src/cpiohdr.h b/src/cpiohdr.h index c297415..588135b 100644 --- a/src/cpiohdr.h +++ b/src/cpiohdr.h @@ -129,5 +129,6 @@ struct cpio_file_stat /* Internal representation of a CPIO header */ char *c_tar_linkname; }; +void cpio_set_c_name(struct cpio_file_stat *file_hdr, char *name); #endif /* cpiohdr.h */ diff --git a/src/tar.c b/src/tar.c index 1b1156e..0a34845 100644 --- a/src/tar.c +++ b/src/tar.c @@ -282,7 +282,7 @@ read_in_tar_header (struct cpio_file_stat *file_hdr, int in_des) if (null_block ((long *) &tar_rec, TARRECORDSIZE)) #endif { - file_hdr->c_name = CPIO_TRAILER_NAME; + cpio_set_c_name (file_hdr, CPIO_TRAILER_NAME); return; } #if 0 @@ -316,9 +316,11 @@ read_in_tar_header (struct cpio_file_stat *file_hdr, int in_des) } if (archive_format != arf_ustar) - file_hdr->c_name = stash_tar_filename (NULL, tar_hdr->name); + cpio_set_c_name (file_hdr, stash_tar_filename (NULL, tar_hdr->name)); else - file_hdr->c_name = stash_tar_filename (tar_hdr->prefix, tar_hdr->name); + cpio_set_c_name (file_hdr, stash_tar_filename (tar_hdr->prefix, + tar_hdr->name)); + file_hdr->c_nlink = 1; file_hdr->c_mode = FROM_OCTAL (tar_hdr->mode); file_hdr->c_mode = file_hdr->c_mode & 07777; @@ -398,7 +400,7 @@ read_in_tar_header (struct cpio_file_stat *file_hdr, int in_des) case AREGTYPE: /* Old tar format; if the last char in filename is '/' then it is a directory, otherwise it's a regular file. */ - if (file_hdr->c_name[strlen (file_hdr->c_name) - 1] == '/') + if (file_hdr->c_name[file_hdr->c_namesize - 1] == '/') file_hdr->c_mode |= CP_IFDIR; else file_hdr->c_mode |= CP_IFREG; diff --git a/src/util.c b/src/util.c index d78d576..b477c14 100644 --- a/src/util.c +++ b/src/util.c @@ -1409,8 +1409,34 @@ set_file_times (int fd, utime_error (name); } + +void +cpio_set_c_name(struct cpio_file_stat *file_hdr, char *name) +{ + static size_t buflen = 0; + size_t len = strlen(name) + 1; + + if (buflen == 0) + { + buflen = len; + if (buflen < 32) + buflen = 32; + file_hdr->c_name = xmalloc (buflen); + } + else if (buflen < len) + { + buflen = len; + file_hdr->c_name = xrealloc (file_hdr->c_name, buflen); + } + + file_hdr->c_namesize = len; + memmove (file_hdr->c_name, name, len); +} + /* 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 +1451,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)); } diff --git a/tests/inout.at b/tests/inout.at index 86c7d02..fd7baea 100644 --- a/tests/inout.at +++ b/tests/inout.at @@ -35,7 +35,22 @@ while read NAME LENGTH do genfile --length $LENGTH > $NAME echo $NAME -done < filelist | - cpio --quiet -o > archive]) +done < filelist > filelist_raw + +for format in bin odc newc crc tar ustar hpbin hpodc +do + cpio --format=$format --quiet -o < filelist_raw > archive.$format + rm -rf output + mkdir output && cd output + cpio -i --quiet < ../archive.$format + + while read file + do + test -f $file || echo "$file not found" + done < ../filelist_raw + + cd .. +done +]) AT_CLEANUP -- 2.9.3
