From owner-svn-src-all@freebsd.org Tue Jul 28 22:53:03 2015 Return-Path: Delivered-To: svn-src-all@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 463849AD41F; Tue, 28 Jul 2015 22:53:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 7CBDC1D8; Tue, 28 Jul 2015 22:53:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 2DC621A1ADC; Wed, 29 Jul 2015 08:52:52 +1000 (AEST) Date: Wed, 29 Jul 2015 08:52:52 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Baptiste Daroussin cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r285985 - in head/usr.sbin/pw: . tests In-Reply-To: <201507282110.t6SLAx0k035167@repo.freebsd.org> Message-ID: <20150729080932.S5059@besplex.bde.org> References: <201507282110.t6SLAx0k035167@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=XMDNMlVE c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=7YfXLusrAAAA:8 a=HdzjfYQfgVDZoVN09S8A:9 a=J1zZFbDsFyZ9_z_e:21 a=GQZ45p5tVgdAolL7:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Jul 2015 22:53:03 -0000 On Tue, 28 Jul 2015, Baptiste Daroussin wrote: > Log: > Check uid/gid used when creating a user/group are not larger than UID_MAX/GID_MAX > > PR: 173977 > Reported by: nvass@gmx.com This is broken in a different way than before. > Modified: head/usr.sbin/pw/pw.c > ============================================================================== > --- head/usr.sbin/pw/pw.c Tue Jul 28 20:52:10 2015 (r285984) > +++ head/usr.sbin/pw/pw.c Tue Jul 28 21:10:58 2015 (r285985) > @@ -269,7 +269,7 @@ main(int argc, char *argv[]) > } > if (strspn(optarg, "0123456789") != strlen(optarg)) > errx(EX_USAGE, "-g expects a number"); > - id = strtonum(optarg, 0, LONG_MAX, &errstr); > + id = strtonum(optarg, 0, GID_MAX, &errstr); `id' still has type long. The assignment overflows on 32-bit arches when the value exceeds 0x7fffffff. That is for half of all valid values. pw is broken in not supporting these values, but at least it detected them as errors in the previous version. Old versions implemented this bug using atoi() with no error checking. The behaviour in the overflowing cases is implementation-defined. Normally this converts large values to negative ones. This defeats the lower limit of 0. The change works accidentally on 64-bit arches. It is assumed that GID_MAX is representable as long long (since strtonum()'s API is too broken to use even intmax_t, and uintmax_t is needed). This assumption is satisfied now. It would break if someone expands gid_t to uint64_t. without expandling long long to int65_t or larger. Then passing GID_MAX would change it to an implementation-defined value. Normally to INT64_MIN. This negative, so there are no values in the range. Similarly for uids. > Added: head/usr.sbin/pw/tests/pw_groupadd.sh > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/usr.sbin/pw/tests/pw_groupadd.sh Tue Jul 28 21:10:58 2015 (r285985) > @@ -0,0 +1,15 @@ > +# $FreeBSD$ > + > +# Import helper functions > +. $(atf_get_srcdir)/helper_functions.shin > + > +atf_test_case group_add_gid_too_large > +group_add_gid_too_large_body() { > + populate_etc_skel > + atf_check -s exit:64 -e inline:"pw: Bad id '9999999999999': too large\n" \ > + ${PW} groupadd -n test1 -g 9999999999999 > +} Check for large valid ids on i386 (should succeed, but currently fail), negative ids (require failure), magic ids like (uid_t)-1 and (uid_t)-2 (should fail, but currently succeed on amd64), and the hex ids (should succeed, but currently fail). (uid_t)-1 is special for some syscalls, so shouldn't be permitted for users. (uid_t)-2 special for nfs (see exports(5)). The magic ids are hard to spell without using hex, but pw is too broken to accept that. For 32-bit ids, the above number should be replaced by 0x100000000 when pw supports hex. Also check that 0xffffffff and 0xfffffffe are not too large, but reserved, and that 0xfffffffd is not too large and not reserved. Bruce