From 565b5aea491671ae33df1ca63697c10d54c00165 Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Thu, 10 Oct 2024 23:16:12 -0700 Subject: [PATCH] Don't crash on truncated tar archives (#2364) The tar header parsing overhaul in #2127 introduced a systematic mishandling of truncated files: the code incorrectly checks for whether a given read operation failed, and ends up dereferencing a NULL pointer in this case. I've gone back and double-checked how `__archive_read_ahead` actually works (it returns NULL precisely when it was unable to satisfy the read request) and reworked the error handling for each call to this function in archive_read_support_format_tar.c Resolves #2353 Resolves https://issues.oss-fuzz.com/issues/42537231 --- libarchive/archive_read_support_format_tar.c | 150 ++++++++++--------- 1 file changed, 80 insertions(+), 70 deletions(-) diff --git a/libarchive/archive_read_support_format_tar.c b/libarchive/archive_read_support_format_tar.c index 5517198ed..4a3d71783 100644 --- a/libarchive/archive_read_support_format_tar.c +++ b/libarchive/archive_read_support_format_tar.c @@ -627,8 +627,6 @@ archive_read_format_tar_read_data(struct archive_read *a, } *buff = __archive_read_ahead(a, 1, &bytes_read); - if (bytes_read < 0) - return (ARCHIVE_FATAL); if (*buff == NULL) { archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, "Truncated tar archive" @@ -746,8 +744,6 @@ tar_read_header(struct archive_read *a, struct tar *tar, /* Read 512-byte header record */ h = __archive_read_ahead(a, 512, &bytes); - if (bytes < 0) - return ((int)bytes); if (bytes == 0) { /* EOF at a block boundary. */ if (eof_fatal) { /* We've read a special header already; @@ -760,7 +756,7 @@ tar_read_header(struct archive_read *a, struct tar *tar, return (ARCHIVE_EOF); } } - if (bytes < 512) { /* Short block at EOF; this is bad. */ + if (h == NULL) { /* Short block at EOF; this is bad. */ archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Truncated tar archive" @@ -1638,6 +1634,9 @@ read_mac_metadata_blob(struct archive_read *a, tar_flush_unconsumed(a, unconsumed); data = __archive_read_ahead(a, msize, NULL); if (data == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading macOS metadata"); *unconsumed = 0; return (ARCHIVE_FATAL); } @@ -1825,10 +1824,7 @@ header_pax_extension(struct archive_read *a, struct tar *tar, to_read = ext_size; } p = __archive_read_ahead(a, to_read, &did_read); - if (did_read < 0) { - return ((int)did_read); - } - if (did_read == 0) { /* EOF */ + if (p == NULL) { /* EOF */ archive_set_error(&a->archive, EINVAL, "Truncated tar archive" " detected while reading pax attribute name"); @@ -1925,10 +1921,7 @@ header_pax_extension(struct archive_read *a, struct tar *tar, /* Consume the `\n` that follows the pax attribute value. */ tar_flush_unconsumed(a, unconsumed); p = __archive_read_ahead(a, 1, &did_read); - if (did_read < 0) { - return ((int)did_read); - } - if (did_read == 0) { + if (p == NULL) { archive_set_error(&a->archive, EINVAL, "Truncated tar archive" " detected while completing pax attribute"); @@ -2310,13 +2303,15 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent err = ARCHIVE_FAILED; } else { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p != NULL) { - if (gnu_sparse_01_parse(a, tar, p, value_length) != ARCHIVE_OK) { - err = ARCHIVE_WARN; - } - } else { + if (p == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading GNU sparse data"); return (ARCHIVE_FATAL); } + if (gnu_sparse_01_parse(a, tar, p, value_length) != ARCHIVE_OK) { + err = ARCHIVE_WARN; + } } __archive_read_consume(a, value_length); return (err); @@ -2392,24 +2387,23 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent /* LIBARCHIVE.symlinktype */ if (value_length < 16) { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p != NULL) { - if (value_length == 4 && memcmp(p, "file", 4) == 0) { - archive_entry_set_symlink_type(entry, - AE_SYMLINK_TYPE_FILE); - } else if (value_length == 3 && memcmp(p, "dir", 3) == 0) { - archive_entry_set_symlink_type(entry, - AE_SYMLINK_TYPE_DIRECTORY); - } else { - archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, - "Unrecognized symlink type"); - err = ARCHIVE_WARN; - } - } else { + if (p == NULL) { archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Truncated tar archive " "detected while reading `symlinktype` attribute"); return (ARCHIVE_FATAL); } + if (value_length == 4 && memcmp(p, "file", 4) == 0) { + archive_entry_set_symlink_type(entry, + AE_SYMLINK_TYPE_FILE); + } else if (value_length == 3 && memcmp(p, "dir", 3) == 0) { + archive_entry_set_symlink_type(entry, + AE_SYMLINK_TYPE_DIRECTORY); + } else { + archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, + "Unrecognized symlink type"); + err = ARCHIVE_WARN; + } } else { archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, "symlink type is very long" @@ -2427,8 +2421,13 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent err = ARCHIVE_WARN; } else { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p == NULL - || pax_attribute_LIBARCHIVE_xattr(entry, key, key_length, p, value_length)) { + if (p == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading xattr information"); + return (ARCHIVE_FATAL); + } + if (pax_attribute_LIBARCHIVE_xattr(entry, key, key_length, p, value_length)) { /* TODO: Unable to parse xattr */ err = ARCHIVE_WARN; } @@ -2451,8 +2450,13 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent err = ARCHIVE_WARN; } else { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p == NULL - || pax_attribute_RHT_security_selinux(entry, p, value_length)) { + if (p == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading selinux data"); + return (ARCHIVE_FATAL); + } + if (pax_attribute_RHT_security_selinux(entry, p, value_length)) { /* TODO: Unable to parse xattr */ err = ARCHIVE_WARN; } @@ -2499,13 +2503,15 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent else if (key_length == 6 && memcmp(key, "fflags", 6) == 0) { if (value_length < fflags_limit) { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p != NULL) { - archive_entry_copy_fflags_text_len(entry, p, value_length); - err = ARCHIVE_OK; - } else { + if (p == NULL) { /* Truncated archive */ - err = ARCHIVE_FATAL; + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading SCHILY.fflags"); + return (ARCHIVE_FATAL); } + archive_entry_copy_fflags_text_len(entry, p, value_length); + err = ARCHIVE_OK; } else { /* Overlong fflags field */ err = ARCHIVE_WARN; @@ -2544,8 +2550,13 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent key += 6; if (value_length < xattr_limit) { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p == NULL - || pax_attribute_SCHILY_xattr(entry, key, key_length, p, value_length)) { + if (p == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading SCHILY.xattr"); + return (ARCHIVE_FATAL); + } + if (pax_attribute_SCHILY_xattr(entry, key, key_length, p, value_length)) { /* TODO: Unable to parse xattr */ err = ARCHIVE_WARN; } @@ -2568,16 +2579,18 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent /* SUN.holesdata */ if (value_length < sparse_map_limit) { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p != NULL) { - err = pax_attribute_SUN_holesdata(a, tar, entry, p, value_length); - if (err < ARCHIVE_OK) { - archive_set_error(&a->archive, - ARCHIVE_ERRNO_MISC, - "Parse error: SUN.holesdata"); - } - } else { + if (p == NULL) { + archive_set_error(&a->archive, EINVAL, + "Truncated archive" + " detected while reading SUN.holesdata"); return (ARCHIVE_FATAL); } + err = pax_attribute_SUN_holesdata(a, tar, entry, p, value_length); + if (err < ARCHIVE_OK) { + archive_set_error(&a->archive, + ARCHIVE_ERRNO_MISC, + "Parse error: SUN.holesdata"); + } } else { archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, "Unreasonably large sparse map: %d > %d", @@ -2629,26 +2642,25 @@ pax_attribute(struct archive_read *a, struct tar *tar, struct archive_entry *ent if (key_length == 10 && memcmp(key, "hdrcharset", 10) == 0) { if (value_length < 64) { p = __archive_read_ahead(a, value_length, &bytes_read); - if (p != NULL) { - if (value_length == 6 - && memcmp(p, "BINARY", 6) == 0) { - /* Binary mode. */ - tar->pax_hdrcharset_utf8 = 0; - err = ARCHIVE_OK; - } else if (value_length == 23 - && memcmp(p, "ISO-IR 10646 2000 UTF-8", 23) == 0) { - tar->pax_hdrcharset_utf8 = 1; - err = ARCHIVE_OK; - } else { - /* TODO: Unrecognized character set */ - err = ARCHIVE_WARN; - } - } else { + if (p == NULL) { archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Truncated tar archive " "detected while reading hdrcharset attribute"); return (ARCHIVE_FATAL); } + if (value_length == 6 + && memcmp(p, "BINARY", 6) == 0) { + /* Binary mode. */ + tar->pax_hdrcharset_utf8 = 0; + err = ARCHIVE_OK; + } else if (value_length == 23 + && memcmp(p, "ISO-IR 10646 2000 UTF-8", 23) == 0) { + tar->pax_hdrcharset_utf8 = 1; + err = ARCHIVE_OK; + } else { + /* TODO: Unrecognized character set */ + err = ARCHIVE_WARN; + } } else { archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "hdrcharset attribute is unreasonably large (%d bytes)", @@ -2981,9 +2993,7 @@ gnu_sparse_old_read(struct archive_read *a, struct tar *tar, do { tar_flush_unconsumed(a, unconsumed); data = __archive_read_ahead(a, 512, &bytes_read); - if (bytes_read < 0) - return (ARCHIVE_FATAL); - if (bytes_read < 512) { + if (data == NULL) { archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Truncated tar archive " "detected while reading sparse file data"); @@ -3403,7 +3413,7 @@ readline(struct archive_read *a, struct tar *tar, const char **start, tar_flush_unconsumed(a, unconsumed); t = __archive_read_ahead(a, 1, &bytes_read); - if (bytes_read <= 0) + if (bytes_read <= 0 || t == NULL) return (ARCHIVE_FATAL); s = t; /* Start of line? */ p = memchr(t, '\n', bytes_read); @@ -3444,7 +3454,7 @@ readline(struct archive_read *a, struct tar *tar, const char **start, } /* Read some more. */ t = __archive_read_ahead(a, 1, &bytes_read); - if (bytes_read <= 0) + if (bytes_read <= 0 || t == NULL) return (ARCHIVE_FATAL); s = t; /* Start of line? */ p = memchr(t, '\n', bytes_read); -- 2.47.2