The uname and gname fields of a ustar header are fixed-size and need
not be NUL-terminated within the archive. read_in_tar_header passed
them straight to getuidbyname and getgidbyname, which call getpwnam
and getgrnam and treat them as C strings, so a header with those
fields (and the record tail) fully populated reads past the 512-byte
on-stack record. This completes on the read side the bounds fix made
for write_out_tar_header in commits e8931bebb9e1 and b5c7d6ee2752.
* src/tar.c (read_in_tar_header): Copy uname and gname into
NUL-terminated buffers before the name lookups.
---
This is the read-side counterpart to the ustar name-field hardening
done on the write side in e8931bebb9e1 and b5c7d6ee2752. Those bounded
the copies into a header being written out. The copies out of a header
being read in were still unbounded.
uname and gname are 32-byte fields with no required terminator. Take a
ustar header where uname, gname and the record tail (devmajor, devminor,
prefix, padding) are all non-zero. getuidbyname and getgidbyname then
run getpwnam/getgrnam over uname as a C string and walk off the end of
the 512-byte record. It is a stack out-of-bounds read, reachable from a
plain "cpio -itv < archive" with default options: ustar is auto-detected
and numeric_uid is off by default. Nothing is written, so I would call
the severity modest, but it is a clean overread on attacker-controlled
input.
Reproducer: one 512-byte ustar record with a valid checksum, the ustar
magic, and uname/gname/devmajor/devminor/prefix/padding set to non-zero
bytes. Built under ASan on this commit it aborts with
stack-buffer-overflow READ of size 248 in getpwnam
<- getuidbyname (idcache.c:105)
<- read_in_tar_header (tar.c:333)
With the patch it lists the entry and exits cleanly. The test suite
still passes (all 17).
The change is under the FSF tiny-change threshold, so no copyright
assignment should be needed.
src/tar.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/tar.c b/src/tar.c
index ff7af7c0d45e..b66286c6a6c2 100644
--- a/src/tar.c
+++ b/src/tar.c
@@ -266,6 +266,8 @@ read_in_tar_header (struct cpio_file_stat *file_hdr, int
in_des)
struct tar_header *tar_hdr = (struct tar_header *) &tar_rec;
uid_t *uidp;
gid_t *gidp;
+ char uname[sizeof (tar_hdr->uname) + 1];
+ char gname[sizeof (tar_hdr->gname) + 1];
tape_buffered_read ((char *) &tar_rec, in_des, TARRECORDSIZE);
@@ -329,14 +331,20 @@ read_in_tar_header (struct cpio_file_stat *file_hdr, int
in_des)
/* Debian hack: This version of cpio uses the -n flag also to extract
tar archives using the numeric UID/GID instead of the user/group
names in /etc/passwd and /etc/groups. (98/10/15) -BEM */
+ /* uname and gname need not be NUL-terminated in the archive;
+ terminate copies before the lookups treat them as C strings. */
+ strncpy (uname, tar_hdr->uname, sizeof (tar_hdr->uname));
+ uname[sizeof (tar_hdr->uname)] = '\0';
+ strncpy (gname, tar_hdr->gname, sizeof (tar_hdr->gname));
+ gname[sizeof (tar_hdr->gname)] = '\0';
if (archive_format == arf_ustar && !numeric_uid
- && (uidp = getuidbyname (tar_hdr->uname)))
+ && (uidp = getuidbyname (uname)))
file_hdr->c_uid = *uidp;
else
file_hdr->c_uid = FROM_OCTAL (tar_hdr->uid);
if (archive_format == arf_ustar && !numeric_uid
- && (gidp = getgidbyname (tar_hdr->gname)))
+ && (gidp = getgidbyname (gname)))
file_hdr->c_gid = *gidp;
else
file_hdr->c_gid = FROM_OCTAL (tar_hdr->gid);
--
2.55.0
