Date: Thu, 01 Dec 2016 18:25:18 -0800 From: Cy Schubert <Cy.Schubert@komquats.com> To: Martin Matuska <mm@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309300 - in head: contrib/libarchive contrib/libarchive/libarchive contrib/libarchive/libarchive/test contrib/libarchive/tar/test lib/libarchive/tests Message-ID: <201612020225.uB22PIQl039020@slippy.cwsent.com> In-Reply-To: Message from Martin Matuska <mm@FreeBSD.org> of "Tue, 29 Nov 2016 22:14:42 %2B0000." <201611292214.uATMEgQH079904@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <201611292214.uATMEgQH079904@repo.freebsd.org>, Martin Matuska write s: > Author: mm > Date: Tue Nov 29 22:14:42 2016 > New Revision: 309300 > URL: https://svnweb.freebsd.org/changeset/base/309300 > > Log: > MFV r309299: > Sync libarchive with vendor. > > Important vendor bugfixes (relevant to FreeBSD): > #821: tar -P cannot extract hardlinks through symlinks > #825: Add sanity check of tar "uid, "gid" and "mtime" fields > > PR: 213255 > Reported by: Tijl Coosemans <tilj@FreeBSD.org> > MFC after: 1 week > > Added: > head/contrib/libarchive/libarchive/test/test_compat_gtar_2.tar.uu > - copied unchanged from r309299, vendor/libarchive/dist/libarchive/test/ > test_compat_gtar_2.tar.uu > head/contrib/libarchive/libarchive/test/test_compat_star_acl_posix1e.c > - copied unchanged from r309299, vendor/libarchive/dist/libarchive/test/ > test_compat_star_acl_posix1e.c > head/contrib/libarchive/libarchive/test/test_compat_star_acl_posix1e.tar.uu > - copied unchanged from r309299, vendor/libarchive/dist/libarchive/test/ > test_compat_star_acl_posix1e.tar.uu > head/contrib/libarchive/libarchive/test/test_read_format_raw.bufr.uu > - copied unchanged from r309299, vendor/libarchive/dist/libarchive/test/ > test_read_format_raw.bufr.uu > Modified: > head/contrib/libarchive/NEWS > head/contrib/libarchive/libarchive/archive_acl.c > head/contrib/libarchive/libarchive/archive_entry.c > head/contrib/libarchive/libarchive/archive_entry.h > head/contrib/libarchive/libarchive/archive_entry_acl.3 > head/contrib/libarchive/libarchive/archive_read_disk_entry_from_file.c > head/contrib/libarchive/libarchive/archive_read_support_filter_xz.c > head/contrib/libarchive/libarchive/archive_read_support_format_tar.c > head/contrib/libarchive/libarchive/archive_read_support_format_xar.c > head/contrib/libarchive/libarchive/archive_write_disk_posix.c > head/contrib/libarchive/libarchive/test/test_compat_gtar.c > head/contrib/libarchive/libarchive/test/test_read_format_raw.c > head/contrib/libarchive/libarchive/test/test_sparse_basic.c > head/contrib/libarchive/tar/test/test_symlink_dir.c > head/lib/libarchive/tests/Makefile > Directory Properties: > head/contrib/libarchive/ (props changed) > [...] > Modified: head/contrib/libarchive/libarchive/archive_read_support_format_tar. > c > ============================================================================= > = > --- head/contrib/libarchive/libarchive/archive_read_support_format_tar.c > Tue Nov 29 21:53:16 2016 (r309299) > +++ head/contrib/libarchive/libarchive/archive_read_support_format_tar.c > Tue Nov 29 22:14:42 2016 (r309300) > @@ -294,6 +294,46 @@ archive_read_format_tar_cleanup(struct a > return (ARCHIVE_OK); > } > > +static int > +validate_number_field(const char* p_field, size_t i_size) > +{ > + unsigned char marker = (unsigned char)p_field[0]; > + /* octal? */ > + if ((marker >= '0' && marker <= '7') || marker == ' ') { > + size_t i = 0; > + int octal_found = 0; > + for (i = 0; i < i_size; ++i) { > + switch (p_field[i]) > + { > + case ' ': /* skip any leading spaces and trailing space > */ > + if (octal_found == 0 || i == i_size - 1) { > + continue; > + } > + break; > + case '\0': /* null is allowed only at the end */ > + if (i != i_size - 1) { > + return 0; > + } > + break; > + /* rest must be octal digits */ > + case '0': case '1': case '2': case '3': > + case '4': case '5': case '6': case '7': > + ++octal_found; > + break; > + } > + } > + return octal_found > 0; > + } > + /* base 256 (i.e. binary number) */ > + else if (marker == 128 || marker == 255 || marker == 0) { > + /* nothing to check */ > + return 1; > + } > + /* not a number field */ > + else { > + return 0; > + } > +} > > static int > archive_read_format_tar_bid(struct archive_read *a, int best_bid) > @@ -346,23 +386,23 @@ archive_read_format_tar_bid(struct archi > return (0); > bid += 2; /* 6 bits of variation in an 8-bit field leaves 2 bits. */ > > - /* Sanity check: Look at first byte of mode field. */ > - switch (255 & (unsigned)header->mode[0]) { > - case 0: case 255: > - /* Base-256 value: No further verification possible! */ > - break; > - case ' ': /* Not recommended, but not illegal, either. */ > - break; > - case '0': case '1': case '2': case '3': > - case '4': case '5': case '6': case '7': > - /* Octal Value. */ > - /* TODO: Check format of remainder of this field. */ > - break; > - default: > - /* Not a valid mode; bail out here. */ > - return (0); > + /* > + * Check format of mode/uid/gid/mtime/size/rdevmajor/rdevminor fields. > + * These are usually octal numbers but GNU tar encodes "big" values as > + * base256 and leading zeroes are sometimes replaced by spaces. > + * Even the null terminator is sometimes omitted. Anyway, must be check > ed > + * to avoid false positives. > + */ > + if (bid > 0 && > + (validate_number_field(header->mode, sizeof(header->mode)) == 0 > || > + validate_number_field(header->uid, sizeof(header->uid)) == 0 | > | > + validate_number_field(header->gid, sizeof(header->gid)) == 0 | > | > + validate_number_field(header->mtime, sizeof(header->mtime)) == > 0 || > + validate_number_field(header->size, sizeof(header->size)) == 0 > || > + validate_number_field(header->rdevmajor, sizeof(header->rdevma > jor)) == 0 || > + validate_number_field(header->rdevminor, sizeof(header->rdevmi > nor)) == 0)) { > + bid = 0; > } > - /* TODO: Sanity test uid/gid/size/mtime/rdevmajor/rdevminor fields. */ > > return (bid); > } > Hi, This broke extract of older tarballs and compatibility with Perl Archive::Tar. You can test this by trying to extract ports/net/tcpview. The following patch fixes it: https://github.com/libarchive/libarchive/commit/2d2b3e928605f795515b03f060fd638c265b0778#diff-845deb76aa98ded5cb2860ded3e19dfb The relevant part of the patch follows: diff --git a/libarchive/archive_read_support_format_tar.c b/libarchive/archive_read_support_format_tar.c index 0ee511e..b977cb7 100644 --- libarchive/archive_read_support_format_tar.c +++ libarchive/archive_read_support_format_tar.c @@ -294,8 +294,14 @@ archive_read_format_tar_cleanup(struct archive_read *a) return (ARCHIVE_OK); } +/* + * Validate number field + * + * Flags: + * 1 - allow double \0 at field end + */ static int -validate_number_field(const char* p_field, size_t i_size) +validate_number_field(const char* p_field, size_t i_size, int flags) { unsigned char marker = (unsigned char)p_field[0]; /* octal? */ @@ -305,14 +311,24 @@ validate_number_field(const char* p_field, size_t i_size) for (i = 0; i < i_size; ++i) { switch (p_field[i]) { - case ' ': /* skip any leading spaces and trailing space*/ + case ' ': + /* skip any leading spaces and trailing space */ if (octal_found == 0 || i == i_size - 1) { continue; } break; - case '\0': /* null is allowed only at the end */ + case '\0': + /* + * null should be allowed only at the end + * + * Perl Archive::Tar terminates some fields + * with two nulls. We must allow this to stay + * compatible. + */ if (i != i_size - 1) { - return 0; + if (((flags & 1) == 0) + || i != i_size - 2) + return 0; } break; /* rest must be octal digits */ @@ -390,18 +406,25 @@ archive_read_format_tar_bid(struct archive_read *a, int best_bid) * Check format of mode/uid/gid/mtime/size/rdevmajor/rdevminor fields. * These are usually octal numbers but GNU tar encodes "big" values as * base256 and leading zeroes are sometimes replaced by spaces. - * Even the null terminator is sometimes omitted. Anyway, must be checked - * to avoid false positives. + * Even the null terminator is sometimes omitted. Anyway, must be + * checked to avoid false positives. + * + * Perl Archive::Tar does not follow the spec and terminates mode, uid, + * gid, rdevmajor and rdevminor with a double \0. For compatibility + * reasons we allow this deviation. */ - if (bid > 0 && - (validate_number_field(header->mode, sizeof(header->mode)) == 0 || - validate_number_field(header->uid, sizeof(header->uid)) == 0 || - validate_number_field(header->gid, sizeof(header->gid)) == 0 || - validate_number_field(header->mtime, sizeof(header->mtime)) == 0 || - validate_number_field(header->size, sizeof(header->size)) == 0 || - validate_number_field(header->rdevmajor, sizeof(header->rdevmajor)) == 0 || - validate_number_field(header->rdevminor, sizeof(header->rdevminor)) == 0)) { - bid = 0; + if (bid > 0 && ( + validate_number_field(header->mode, sizeof(header->mode), 1) == 0 + || validate_number_field(header->uid, sizeof(header->uid), 1) == 0 + || validate_number_field(header->gid, sizeof(header->gid), 1) == 0 + || validate_number_field(header->mtime, sizeof(header->mtime), + 0) == 0 + || validate_number_field(header->size, sizeof(header->size), 0) == 0 + || validate_number_field(header->rdevmajor, + sizeof(header->rdevmajor), 1) == 0 + || validate_number_field(header->rdevminor, + sizeof(header->rdevminor), 1) == 0)) { + bid = 0; } return (bid); Updating to git 2d2b3e928605f795515b03f060fd638c265b0778 or later probably makes the most sense. -- Cheers, Cy Schubert <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201612020225.uB22PIQl039020>