On Thursday, April 6, 2017 3:01:46 PM CEST Kristýna Streitová wrote:
> Thank you for it. I have just small comments on your patch:
>
> 1) It seems that the first hunk slightly changes the logic.
> Before your patch, the 'is_tar_filename_too_long()' function was called
> for tar and ustar archives only but now it's called for all archive types.
good catch. It looked like a typo to me before (that non-tar archives were not
checked), but looking more precisely -- I would say it is useless check.
Previously there was comment:
/* 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 */
I'm not sure what the "additional check" note talks about, and I'm not
able to find the original report from 1999 ...
Because --rename and --rename-batch-file is valid for copy-in mode, I
think removing the check is completely safe.
> As this function checks whether the filename is too long to fit in a tar
> header, it should be called for tar/ustar archives only, I believe.
>
> 2) It's just a small thing but it seems that you forgot to use your new
> 'cpio_set_c_name()' function for 'file_hdr.c_name = CPIO_TRAILER_NAME'
> in the copyout.c file.
That's almost before exit(), but let's be consistent ... thanks for the
review.
Pavel
> Best regards,
> Kristyna Streitova
>
>From 7462d18379c3e83e943cb8cf542161025aea0d2c 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 | 13 +++++------
src/cpiohdr.h | 1 +
src/tar.c | 10 +++++----
src/util.c | 32 ++++++++++++++++++++++++++-
tests/inout.at | 19 ++++++++++++++--
6 files changed, 82 insertions(+), 61 deletions(-)
diff --git a/src/copyin.c b/src/copyin.c
index 6a306ee..ba887ae 100644
--- a/src/copyin.c
+++ b/src/copyin.c
@@ -76,28 +76,7 @@ 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);
- }
- 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 +321,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 +990,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 +1032,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 +1088,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 +1137,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 +1220,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..7532dac 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
@@ -866,8 +864,7 @@ process_copy_out ()
file_hdr.c_chksum = 0;
file_hdr.c_filesize = 0;
- file_hdr.c_namesize = 11;
- file_hdr.c_name = CPIO_TRAILER_NAME;
+ cpio_set_c_name (&file_hdr, CPIO_TRAILER_NAME);
if (archive_format != arf_tar && archive_format != arf_ustar)
write_out_header (&file_hdr, out_file_des);
else
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..10486dc 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
