NetBSD Problem Report #45044

From www@NetBSD.org  Fri Jun 10 15:17:50 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 2031463B99B
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 10 Jun 2011 15:17:50 +0000 (UTC)
Message-Id: <20110610151749.11AFD63B970@www.NetBSD.org>
Date: Fri, 10 Jun 2011 15:17:49 +0000 (UTC)
From: tcort@minix3.org
Reply-To: tcort@minix3.org
To: gnats-bugs@NetBSD.org
Subject: archivers/libarchive minix support
X-Send-Pr-Version: www-1.0

>Number:         45044
>Category:       pkg
>Synopsis:       archivers/libarchive minix support
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    joerg
>State:          closed
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Fri Jun 10 15:20:00 +0000 2011
>Closed-Date:    Fri Jun 17 12:09:45 +0000 2011
>Last-Modified:  Fri Jun 17 12:09:45 +0000 2011
>Originator:     Thomas Cort
>Release:        N/A
>Organization:
Minix3
>Environment:
Minix 192.168.122.210 3.2.0 i686
>Description:
Minix UIDs are 16-bits and Minix GIDs are 8-bits. The tar format supports much larger UIDs/GIDs. libarchive should handle the case where the UIDs/GIDs overflow the archive_entry uid/gid fields by setting the uid/gid to something valid (example: the uid of the nobody user and the gid of the nobody group) and it should produce a warning when doing so.
>How-To-Repeat:
Compile libarchive on Minix and attempt to extract files with large uid/gid values from a tar archive.
>Fix:
diff --git a/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c b/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
index dae13dc..882672a 100644
--- a/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
+++ b/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
@@ -26,6 +26,9 @@
 #include "archive_platform.h"
 __FBSDID("$FreeBSD: head/lib/libarchive/archive_read_support_format_tar.c 201161 2009-12-29 05:44:39Z kientzle $");

+#include <grp.h>
+#include <pwd.h>
+
 #ifdef HAVE_ERRNO_H
 #include <errno.h>
 #endif
@@ -917,8 +920,11 @@ static int
 header_common(struct archive_read *a, struct tar *tar,
     struct archive_entry *entry, const void *h)
 {
+	int err = ARCHIVE_OK;
 	const struct archive_entry_header_ustar	*header;
 	char	tartype;
+	uid_t uid;
+	gid_t gid;

 	(void)a; /* UNUSED */

@@ -931,8 +937,85 @@ header_common(struct archive_read *a, struct tar *tar,

 	/* Parse out the numeric fields (all are octal) */
 	archive_entry_set_mode(entry, tar_atol(header->mode, sizeof(header->mode)));
-	archive_entry_set_uid(entry, tar_atol(header->uid, sizeof(header->uid)));
-	archive_entry_set_gid(entry, tar_atol(header->gid, sizeof(header->gid)));
+
+	uid = (uid_t) tar_atol(header->uid, sizeof(header->uid));
+
+	/* Sanity check: uid overflow. Some systems have a limited uid_t.
+	 * For example, Minix 3.2.0 has 16-bit uids.
+	 */
+	if (uid != tar_atol(header->uid, sizeof(header->uid))) {
+
+		/* This isn't a fatal error, so we try to set the uid to
+		 * the uid of the "nobody" user or 99.
+		 */
+
+		static int warned = 0;
+		static struct passwd *nobodyuser = NULL;
+
+		if (nobodyuser == NULL) {
+			nobodyuser = getpwnam("nobody");
+		}
+
+		if (nobodyuser != NULL) {
+			uid = nobodyuser->pw_uid;
+		} else {
+			uid = (uid_t) 99;
+		}
+
+		if (warned == 0) {
+			archive_set_error(&a->archive, EINVAL,
+				"uid %ld out of range; will be extracted as %d.",
+				tar_atol(header->uid, sizeof(header->uid)),
+				uid);
+
+			warned = 1; /* only warn once about invalid uid */
+			err = ARCHIVE_WARN;
+		}
+	}
+
+	archive_entry_set_uid(entry, uid);
+
+	gid = (gid_t) tar_atol(header->gid, sizeof(header->gid));
+
+	/* Sanity check: gid overflow. Some systems have a limited gid_t.
+	 * For example, Minix 3.2.0 has 8-bit gids.
+	 */
+	if (gid != tar_atol(header->gid, sizeof(header->gid))) {
+
+		/* This isn't a fatal error, so we try to set the gid to
+		 * the gid of the "nobody" or "nogroup" group or 99.
+		 */
+
+		static int warned = 0;
+		static struct group *nobodygroup = NULL;
+
+		if (nobodygroup == NULL) {
+
+			nobodygroup = getgrnam("nobody");
+			if (nobodygroup == NULL) {
+				nobodygroup = getgrnam("nogroup");
+			}
+		}
+
+		if (nobodygroup != NULL) {
+			gid = nobodygroup->gr_gid;
+		} else {
+			gid = (gid_t) 99;
+		}
+
+		if (warned == 0) {
+			archive_set_error(&a->archive, EINVAL,
+				"gid %ld out of range; will be extracted as %d",
+				tar_atol(header->gid, sizeof(header->gid)),
+				gid);
+
+			warned = 1; /* only warn once about invalid gid */
+			err = ARCHIVE_WARN;
+		}
+	}
+
+	archive_entry_set_gid(entry, gid);
+
 	tar->entry_bytes_remaining = tar_atol(header->size, sizeof(header->size));
 	tar->realsize = tar->entry_bytes_remaining;
 	archive_entry_set_size(entry, tar->entry_bytes_remaining);
@@ -1063,7 +1146,8 @@ header_common(struct archive_read *a, struct tar *tar,
 		archive_entry_set_filetype(entry, AE_IFREG);
 		break;
 	}
-	return (0);
+
+	return err;
 }

 /*
@@ -1073,6 +1157,7 @@ static int
 header_old_tar(struct archive_read *a, struct tar *tar,
     struct archive_entry *entry, const void *h)
 {
+	int err;
 	const struct archive_entry_header_ustar	*header;

 	/* Copy filename over (to ensure null termination). */
@@ -1081,10 +1166,10 @@ header_old_tar(struct archive_read *a, struct tar *tar,
 	archive_entry_copy_pathname(entry, tar->entry_pathname.s);

 	/* Grab rest of common fields */
-	header_common(a, tar, entry, h);
+	err = header_common(a, tar, entry, h);

 	tar->entry_padding = 0x1ff & (-tar->entry_bytes_remaining);
-	return (0);
+	return err;
 }

 /*
@@ -1143,6 +1228,7 @@ static int
 header_ustar(struct archive_read *a, struct tar *tar,
     struct archive_entry *entry, const void *h)
 {
+	int err;
 	const struct archive_entry_header_ustar	*header;
 	struct archive_string *as;

@@ -1161,7 +1247,7 @@ header_ustar(struct archive_read *a, struct tar *tar,
 	archive_entry_copy_pathname(entry, as->s);

 	/* Handle rest of common fields. */
-	header_common(a, tar, entry, h);
+	err = header_common(a, tar, entry, h);

 	/* Handle POSIX ustar fields. */
 	archive_strncpy(&(tar->entry_uname), header->uname,
@@ -1182,7 +1268,7 @@ header_ustar(struct archive_read *a, struct tar *tar,

 	tar->entry_padding = 0x1ff & (-tar->entry_bytes_remaining);

-	return (0);
+	return err;
 }


@@ -1662,6 +1748,7 @@ static int
 header_gnutar(struct archive_read *a, struct tar *tar,
     struct archive_entry *entry, const void *h)
 {
+	int err;
 	const struct archive_entry_header_gnutar *header;

 	(void)a;
@@ -1673,7 +1760,7 @@ header_gnutar(struct archive_read *a, struct tar *tar,
 	 */

 	/* Grab fields common to all tar variants. */
-	header_common(a, tar, entry, h);
+	err = header_common(a, tar, entry, h);

 	/* Copy filename over (to ensure null termination). */
 	header = (const struct archive_entry_header_gnutar *)h;
@@ -1723,7 +1810,7 @@ header_gnutar(struct archive_read *a, struct tar *tar,
 		}
 	}

-	return (0);
+	return err;
 }

 static void

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: pkg-manager->joerg
Responsible-Changed-By: obache@NetBSD.org
Responsible-Changed-When: Fri, 10 Jun 2011 22:20:04 +0000
Responsible-Changed-Why:
Over to maintainer.


From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc: pkg-manager@netbsd.org, gnats-admin@netbsd.org, pkgsrc-bugs@netbsd.org
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Sat, 11 Jun 2011 20:02:28 +0200

 On Fri, Jun 10, 2011 at 03:20:01PM +0000, tcort@minix3.org wrote:
 > Minix UIDs are 16-bits and Minix GIDs are 8-bits. The tar format
 > supports much larger UIDs/GIDs. libarchive should handle the case where
 > the UIDs/GIDs overflow the archive_entry uid/gid fields by setting
 > the uid/gid to something valid (example: the uid of the nobody user and
 > the gid of the nobody group) and it should produce a warning when doing so.

 I don't think the complexity of this is justified with the gain.
 Ultimately, you are either ending up ignoring the UID/GID or it won't
 work at all. The behavior is already "set it to something valid", since
 it truncates. Doing a user name look up in this piece of code is
 completely inacceptable. In short, I feel this is a case of "use saner
 types or just cope". You can, of course, bring this up on the libarchive
 list or bug tracker, but I am going to repeat my answer in that case.

 Joerg

From: David Holland <dholland-pbugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Sun, 12 Jun 2011 04:34:25 +0000

 On Sat, Jun 11, 2011 at 06:05:04PM +0000, Joerg Sonnenberger wrote:
  >> Minix UIDs are 16-bits and Minix GIDs are 8-bits. The tar format
  >> supports much larger UIDs/GIDs. libarchive should handle the case where
  >> the UIDs/GIDs overflow the archive_entry uid/gid fields by setting
  >> the uid/gid to something valid (example: the uid of the nobody user and
  >> the gid of the nobody group) and it should produce a warning when doing so.
  >  
  >  I don't think the complexity of this is justified with the gain.

 It doesn't make sense to worry about this unless you're root and the
 tar -p option (or equivalent) is in effect, and in that case it should
 print a warning, set the id to 0, and be sure to clear the set[ug]id
 bits. Much simpler that way...

 -- 
 David A. Holland
 dholland@netbsd.org

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc: joerg@NetBSD.org, gnats-admin@netbsd.org, pkgsrc-bugs@netbsd.org,
	tcort@minix3.org
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Sun, 12 Jun 2011 12:03:34 +0200

 On Sun, Jun 12, 2011 at 04:35:03AM +0000, David Holland wrote:
 > The following reply was made to PR pkg/45044; it has been noted by GNATS.
 > 
 > From: David Holland <dholland-pbugs@netbsd.org>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: pkg/45044: archivers/libarchive minix support
 > Date: Sun, 12 Jun 2011 04:34:25 +0000
 > 
 >  On Sat, Jun 11, 2011 at 06:05:04PM +0000, Joerg Sonnenberger wrote:
 >   >> Minix UIDs are 16-bits and Minix GIDs are 8-bits. The tar format
 >   >> supports much larger UIDs/GIDs. libarchive should handle the case where
 >   >> the UIDs/GIDs overflow the archive_entry uid/gid fields by setting
 >   >> the uid/gid to something valid (example: the uid of the nobody user and
 >   >> the gid of the nobody group) and it should produce a warning when doing so.
 >   >  
 >   >  I don't think the complexity of this is justified with the gain.
 >  
 >  It doesn't make sense to worry about this unless you're root and the
 >  tar -p option (or equivalent) is in effect, and in that case it should
 >  print a warning, set the id to 0, and be sure to clear the set[ug]id
 >  bits. Much simpler that way...

 As root permissions are extracted by default. But there are two things
 to keep in mind here:
 (1) user/group name is prefered over numeric version
 (2) If you are not doing a tar tvf first, it doesn't matter. If you do
 it, you get the same numeric values as a later extract would use.

 Joerg

From: Thomas Cort <tcort@minix3.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Mon, 13 Jun 2011 14:13:01 -0400

 I asked the developer(s) who made the uid/gid overflow detection
 changes why exactly the changes were needed. The devs seem to remember
 that some of the pkg_* utilities try querying for the username and
 segfault when the uid is invalid.

 I did some grep-ing and user_from_uid() from
 libnbcompat/files/pwcache.c is called in a couple of places in
 pkgtools/pkg_install without checking the return value.
 user_from_uid() is called with noname=1 which, when a uid without a
 corresponding username is passed, returns NULL. I'll investigate this
 issue further in the coming days.

 So my plan is to take a deeper look at the core issue
 (user_from_uid()), fix that problem (create a patch and new PR), and
 then see if anything else might require the libarchive change we
 originally proposed.

 > I feel this is a case of "use saner types or just cope".

 We'd still like to at least have a warning when a uid or gid is
 truncated. Would you accept such a change? If so, I'll create a new
 patch.

From: Thomas Cort <tcort@minix3.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Thu, 16 Jun 2011 16:18:56 -0400

 I did a bulk build of all of the minix packages with the uid/gid overflow
 detection patch removed, and I did not run into any problems building
 the packages nor installing them. It appears that the patch I originally
 submitted is no longer needed. It was written about a year ago, and whatever
 was causing the problem appears to have been fixed by another change.

 We are still interested in displaying a warning when the UID/GID overflows.
 A patch to display a warning follows:


 diff --git a/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c b/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
 index dae13dc..f77ae14 100644
 --- a/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
 +++ b/archivers/libarchive/files/libarchive/archive_read_support_format_tar.c
 @@ -26,6 +26,9 @@
  #include "archive_platform.h"
  __FBSDID("$FreeBSD: head/lib/libarchive/archive_read_support_format_tar.c 201161 2009-12-29 05:44:39Z kientzle $");

 +#include <grp.h>
 +#include <pwd.h>
 +
  #ifdef HAVE_ERRNO_H
  #include <errno.h>
  #endif
 @@ -917,8 +920,11 @@ static int
  header_common(struct archive_read *a, struct tar *tar,
      struct archive_entry *entry, const void *h)
  {
 +	int err = ARCHIVE_OK;
  	const struct archive_entry_header_ustar	*header;
  	char	tartype;
 +	uid_t uid;
 +	gid_t gid;

  	(void)a; /* UNUSED */

 @@ -931,8 +937,41 @@ header_common(struct archive_read *a, struct tar *tar,

  	/* Parse out the numeric fields (all are octal) */
  	archive_entry_set_mode(entry, tar_atol(header->mode, sizeof(header->mode)));
 -	archive_entry_set_uid(entry, tar_atol(header->uid, sizeof(header->uid)));
 -	archive_entry_set_gid(entry, tar_atol(header->gid, sizeof(header->gid)));
 +
 +	uid = (uid_t) tar_atol(header->uid, sizeof(header->uid));
 +
 +	/* Sanity check: uid overflow. Some systems have a limited uid_t.
 +	 * For example, Minix 3.2.0 has 16-bit uids.
 +	 */
 +	if (uid != tar_atol(header->uid, sizeof(header->uid))) {
 +
 +		archive_set_error(&a->archive, EINVAL,
 +			"uid %ld out of range; will be extracted as %d.",
 +			tar_atol(header->uid, sizeof(header->uid)),
 +			uid);
 +
 +		err = ARCHIVE_WARN;
 +	}
 +
 +	archive_entry_set_uid(entry, uid);
 +
 +	gid = (gid_t) tar_atol(header->gid, sizeof(header->gid));
 +
 +	/* Sanity check: gid overflow. Some systems have a limited gid_t.
 +	 * For example, Minix 3.2.0 has 8-bit gids.
 +	 */
 +	if (gid != tar_atol(header->gid, sizeof(header->gid))) {
 +
 +		archive_set_error(&a->archive, EINVAL,
 +			"gid %ld out of range; will be extracted as %d",
 +			tar_atol(header->gid, sizeof(header->gid)),
 +			gid);
 +
 +		err = ARCHIVE_WARN;
 +	}
 +
 +	archive_entry_set_gid(entry, gid);
 +
  	tar->entry_bytes_remaining = tar_atol(header->size, sizeof(header->size));
  	tar->realsize = tar->entry_bytes_remaining;
  	archive_entry_set_size(entry, tar->entry_bytes_remaining);
 @@ -1063,7 +1102,8 @@ header_common(struct archive_read *a, struct tar *tar,
  		archive_entry_set_filetype(entry, AE_IFREG);
  		break;
  	}
 -	return (0);
 +
 +	return err;
  }

  /*
 @@ -1073,6 +1113,7 @@ static int
  header_old_tar(struct archive_read *a, struct tar *tar,
      struct archive_entry *entry, const void *h)
  {
 +	int err;
  	const struct archive_entry_header_ustar	*header;

  	/* Copy filename over (to ensure null termination). */
 @@ -1081,10 +1122,10 @@ header_old_tar(struct archive_read *a, struct tar *tar,
  	archive_entry_copy_pathname(entry, tar->entry_pathname.s);

  	/* Grab rest of common fields */
 -	header_common(a, tar, entry, h);
 +	err = header_common(a, tar, entry, h);

  	tar->entry_padding = 0x1ff & (-tar->entry_bytes_remaining);
 -	return (0);
 +	return err;
  }

  /*
 @@ -1143,6 +1184,7 @@ static int
  header_ustar(struct archive_read *a, struct tar *tar,
      struct archive_entry *entry, const void *h)
  {
 +	int err;
  	const struct archive_entry_header_ustar	*header;
  	struct archive_string *as;

 @@ -1161,7 +1203,7 @@ header_ustar(struct archive_read *a, struct tar *tar,
  	archive_entry_copy_pathname(entry, as->s);

  	/* Handle rest of common fields. */
 -	header_common(a, tar, entry, h);
 +	err = header_common(a, tar, entry, h);

  	/* Handle POSIX ustar fields. */
  	archive_strncpy(&(tar->entry_uname), header->uname,
 @@ -1182,7 +1224,7 @@ header_ustar(struct archive_read *a, struct tar *tar,

  	tar->entry_padding = 0x1ff & (-tar->entry_bytes_remaining);

 -	return (0);
 +	return err;
  }


 @@ -1662,6 +1704,7 @@ static int
  header_gnutar(struct archive_read *a, struct tar *tar,
      struct archive_entry *entry, const void *h)
  {
 +	int err;
  	const struct archive_entry_header_gnutar *header;

  	(void)a;
 @@ -1673,7 +1716,7 @@ header_gnutar(struct archive_read *a, struct tar *tar,
  	 */

  	/* Grab fields common to all tar variants. */
 -	header_common(a, tar, entry, h);
 +	err = header_common(a, tar, entry, h);

  	/* Copy filename over (to ensure null termination). */
  	header = (const struct archive_entry_header_gnutar *)h;
 @@ -1723,7 +1766,7 @@ header_gnutar(struct archive_read *a, struct tar *tar,
  		}
  	}

 -	return (0);
 +	return err;
  }

  static void

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Fri, 17 Jun 2011 12:52:56 +0200

 On Thu, Jun 16, 2011 at 08:20:04PM +0000, Thomas Cort wrote:
 >  We are still interested in displaying a warning when the UID/GID overflows.

 Bring it up on http://code.google.com/p/libarchive/, please.

 Joerg

From: Thomas Cort <tcort@minix3.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: pkg/45044: archivers/libarchive minix support
Date: Fri, 17 Jun 2011 07:17:10 -0400

 > Bring it up on http://code.google.com/p/libarchive/, please.

 Done.

 http://code.google.com/p/libarchive/issues/detail?id=158

State-Changed-From-To: open->closed
State-Changed-By: joerg@NetBSD.org
State-Changed-When: Fri, 17 Jun 2011 12:09:45 +0000
State-Changed-Why:
Original patch rejected and retracted. Modified version will be discussed
upstream and flow back if accepted.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.