From owner-svn-src-head@freebsd.org Fri Dec 2 02:25:28 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E145AC6274B; Fri, 2 Dec 2016 02:25:28 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 90F1F1142; Fri, 2 Dec 2016 02:25:27 +0000 (UTC) (envelope-from cy.schubert@komquats.com) Received: from spqr.komquats.com ([96.50.22.10]) by shaw.ca with SMTP id CdXLcSRvrKQ1GCdXMcia4w; Thu, 01 Dec 2016 19:25:21 -0700 X-Authority-Analysis: v=2.2 cv=RsK1FGuK c=1 sm=1 tr=0 a=jvE2nwUzI0ECrNeyr98KWA==:117 a=jvE2nwUzI0ECrNeyr98KWA==:17 a=kj9zAlcOel0A:10 a=n5n_aSjo0skA:10 a=6I5d2MoRAAAA:8 a=NEAV23lmAAAA:8 a=YxBL1-UpAAAA:8 a=q2U9Gq7pqrimPw9a1TIA:9 a=X8FCfygWxkzDMqUL:21 a=HV8EdFySohd3zm9c:21 a=pynXk66uh0YmACDl:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 a=Bn2pgwyD2vrAyMmN8A2t:22 a=Ia-lj3WSrqcvXOmTRaiG:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTPS id 976C4C64; Thu, 1 Dec 2016 17:11:36 -0800 (PST) Received: from slippy (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id uB22PIQl039020; Thu, 1 Dec 2016 18:25:18 -0800 (PST) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <201612020225.uB22PIQl039020@slippy.cwsent.com> X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.6 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Martin Matuska 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 In-Reply-To: Message from Martin Matuska of "Tue, 29 Nov 2016 22:14:42 +0000." <201611292214.uATMEgQH079904@repo.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Thu, 01 Dec 2016 18:25:18 -0800 X-CMAE-Envelope: MS4wfBuonkZQIrdWp+8IpGq6JZp5QQ5uyRBzNJR9HmNMMz/4jEensFfk4G1S4tmXNjhK2rShjRO7Rbh8jMYxyLnHWswG4yyIBiEWvQ2vxMVcx6zkmCkU8rmn mFbb4Hd5PR2XnZCZqIkcFEqppyNZJ3oudOqjtDrgeCjq8FG5GzLUgXjeCqtiT/i50ag554b4VveRttCAKG/+CtR1YYA5DVWSFCzSMQOhCi2yXo/KP9PwpBGh 4qCSwsXy96sFR52wBb7r+6t3B5A3tMUBz0Ss3DfZwUzoOuGm5G/Y0BCEgMYoPwdA X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Dec 2016 02:25:29 -0000 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 > 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 FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.