Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Sat, 1 Aug 2015, Jilles Tjoelker wrote:
On Wed, Jul 29, 2015 at 08:52:52AM +1000, Bruce Evans wrote:
On Tue, 28 Jul 2015, Baptiste Daroussin wrote:
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 '9': too
large\n \
+ ${PW} groupadd -n test1 -g 9
+}
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 0x1 when pw supports hex. Also check
that 0x and 0xfffe are not too large, but reserved, and
that 0xfffd is not too large and not reserved.
These values are easily written using arithmetic expansion, for example
largeid=$((0x1)).
Not really. Shells are also very buggy or limited in this area. I
often use old versions of sh and bash that only support up to INT32_MAX
and have broken overflow handling. /bin/sh in -current only supports
up to INT64_MAX (or maybe INTMAX_MAX) and has broken overflow handling
(it clamps to INT64_MAX). Not so old versions of bash only support
up to INT64_MAX and have differently broken overflow handling (4.3.99
blindly assigns to int64_t, so $((0x8000)) becomes
-0x8000.
expr is also limited to INT64_MAX, but attempts to have non-broken
overflow handling.
When using strtol() or similar functions, accepting hex typically
implies accepting octal as well, which causes confusing and
POSIX-violating results like 010 interpreted as eight.
This is a problem. strtonum could accept hex but not octal by calling
strtoimax() twice for bases 10 and 16. Also dehumanized formats like
1k and 1K. It should also actually accept numbers as input. 1.1e1 if
not I * Pi.
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Wed, Jul 29, 2015 at 08:52:52AM +1000, Bruce Evans wrote:
On Tue, 28 Jul 2015, Baptiste Daroussin wrote:
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 '9': too
large\n \
+ ${PW} groupadd -n test1 -g 9
+}
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 0x1 when pw supports hex. Also check
that 0x and 0xfffe are not too large, but reserved, and
that 0xfffd is not too large and not reserved.
These values are easily written using arithmetic expansion, for example
largeid=$((0x1)).
When using strtol() or similar functions, accepting hex typically
implies accepting octal as well, which causes confusing and
POSIX-violating results like 010 interpreted as eight.
--
Jilles Tjoelker
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Sat, 1 Aug 2015, Jilles Tjoelker wrote: On Sun, Aug 02, 2015 at 02:59:00AM +1000, Bruce Evans wrote: On Sat, 1 Aug 2015, Jilles Tjoelker wrote: These values are easily written using arithmetic expansion, for example largeid=$((0x1)). Not really. Shells are also very buggy or limited in this area. I often use old versions of sh and bash that only support up to INT32_MAX and have broken overflow handling. /bin/sh in -current only supports up to INT64_MAX (or maybe INTMAX_MAX) and has broken overflow handling (it clamps to INT64_MAX). Not so old versions of bash only support up to INT64_MAX and have differently broken overflow handling (4.3.99 blindly assigns to int64_t, so $((0x8000)) becomes -0x8000. expr is also limited to INT64_MAX, but attempts to have non-broken overflow handling. The tests need not work with old versions of sh and bash (they already rely on many more recent features and bugfixes). The broken overflow handling in parsing literals does not affect the given example. But I complained about the example being deficient. It only needs to test up to UINT32_MAX now, but that may change. A similar test in dd already needs to go up to UINT64_MAX. dd does support hex numbers, and is suppose to support offsets about OFF_MAX, but this is broken by excessive support for the signedness of off_t. All kernel addresses in /dev/kmem are above 0x on amd64. I tried to get green@ to fix this in Y2K, but the result was a larger mess. When using strtol() or similar functions, accepting hex typically implies accepting octal as well, which causes confusing and POSIX-violating results like 010 interpreted as eight. This is a problem. strtonum could accept hex but not octal by calling strtoimax() twice for bases 10 and 16. Also dehumanized formats like 1k and 1K. It should also actually accept numbers as input. 1.1e1 if not I * Pi. Silently expanding what strtonum() accepts might cause breakage or even security vulnerabilities. It is hard to fix broken APIs/UIs once they are used. strtonum() is not used often in /usr/src. Its main use is in openssh, and that is hard to fix since both strtonum and opensh are from OpenBSD. Using names in the reserved namespace causes portability problems. FreeBSD's expand_number() is a much worse name, but at least it doesn't conflict with better future uses of a reserved name. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Sun, Aug 02, 2015 at 02:59:00AM +1000, Bruce Evans wrote: On Sat, 1 Aug 2015, Jilles Tjoelker wrote: These values are easily written using arithmetic expansion, for example largeid=$((0x1)). Not really. Shells are also very buggy or limited in this area. I often use old versions of sh and bash that only support up to INT32_MAX and have broken overflow handling. /bin/sh in -current only supports up to INT64_MAX (or maybe INTMAX_MAX) and has broken overflow handling (it clamps to INT64_MAX). Not so old versions of bash only support up to INT64_MAX and have differently broken overflow handling (4.3.99 blindly assigns to int64_t, so $((0x8000)) becomes -0x8000. expr is also limited to INT64_MAX, but attempts to have non-broken overflow handling. The tests need not work with old versions of sh and bash (they already rely on many more recent features and bugfixes). The broken overflow handling in parsing literals does not affect the given example. When using strtol() or similar functions, accepting hex typically implies accepting octal as well, which causes confusing and POSIX-violating results like 010 interpreted as eight. This is a problem. strtonum could accept hex but not octal by calling strtoimax() twice for bases 10 and 16. Also dehumanized formats like 1k and 1K. It should also actually accept numbers as input. 1.1e1 if not I * Pi. Silently expanding what strtonum() accepts might cause breakage or even security vulnerabilities. -- Jilles Tjoelker ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Wed, 29 Jul 2015, Baptiste Daroussin wrote: On Wed, Jul 29, 2015 at 08:52:52AM +1000, Bruce Evans wrote: 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: nv...@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 0x7fff. 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. So writting a function like strtonum like function with that prototype intmax_t strtonumber(const char *, intmax_t min, intmax_t max, const char **); and an unsigned equivalent uintmax_t strtonumber(const char *, uintmax_t min, uintmax_t max, const char **); Not completely, since this would restore some of the complications of the strto*() family (there is API explosion, and types to match). These 2 functions don't even handle all the integer ranges (with assymmetric signedness), or hex numbers, or floating point). To match the types for gid_t's with these APIs, you could write: gid_t id = strtounum(optarg, 0, GID_MAX, errstr); or uintmax_t id = strtounum(optarg, 0, GID_MAX, errstr); but can't mix these id types with others that need different signedness.. or use plain int/long/u_long for anything. This is not a problem for uids and gids since they usually have the same underlying type. POSIX (XSI) made a mess of this for waitid(idtype_t idtype, id_t id, ...). id can be either a pid, a uid, a gid or a session id. So id_t is specified as an integer type that can contain all of these. Representing all of these is generally impossible due to the sign differences, and the conversions made to contain seem to be unspecified. FreeBSD actually uses int64_t for id_t and an enum for idtype_t, so simple conversions preserve the value at the cost of bloat. This wouldn't work with 64-bit uid_t. A similar mess can be made for strto*num() by specifying it to work with a container type num_t and making this type opaque so that it is hard to use :-), except there no strict container exists for int64_t and uint64_t. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
On Wed, Jul 29, 2015 at 08:52:52AM +1000, Bruce Evans wrote: 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: nv...@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 0x7fff. 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. So writting a function like strtonum like function with that prototype intmax_t strtonumber(const char *, intmax_t min, intmax_t max, const char **); and an unsigned equivalent uintmax_t strtonumber(const char *, uintmax_t min, uintmax_t max, const char **); would do the right thing? Best regards, Bapt pgpHeRbuvPuy0.pgp Description: PGP signature
Re: svn commit: r285985 - in head/usr.sbin/pw: . tests
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: nv...@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 0x7fff. 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 '9': too
large\n \
+ ${PW} groupadd -n test1 -g 9
+}
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 0x1 when pw supports hex. Also check
that 0x and 0xfffe are not too large, but reserved, and
that 0xfffd is not too large and not reserved.
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r285985 - in head/usr.sbin/pw: . tests
Author: bapt
Date: Tue Jul 28 21:10:58 2015
New Revision: 285985
URL: https://svnweb.freebsd.org/changeset/base/285985
Log:
Check uid/gid used when creating a user/group are not larger than
UID_MAX/GID_MAX
PR: 173977
Reported by: nv...@gmx.com
Added:
head/usr.sbin/pw/tests/pw_groupadd.sh (contents, props changed)
Modified:
head/usr.sbin/pw/pw.c
head/usr.sbin/pw/tests/Makefile
head/usr.sbin/pw/tests/pw_useradd.sh
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);
if (errstr != NULL)
errx(EX_USAGE, Bad id '%s': %s, optarg,
errstr);
@@ -281,7 +281,7 @@ main(int argc, char *argv[])
addarg(arglist, 'u', optarg);
break;
}
- id = strtonum(optarg, 0, LONG_MAX, errstr);
+ id = strtonum(optarg, 0, UID_MAX, errstr);
if (errstr != NULL)
errx(EX_USAGE, Bad id '%s': %s, optarg,
errstr);
Modified: head/usr.sbin/pw/tests/Makefile
==
--- head/usr.sbin/pw/tests/Makefile Tue Jul 28 20:52:10 2015
(r285984)
+++ head/usr.sbin/pw/tests/Makefile Tue Jul 28 21:10:58 2015
(r285985)
@@ -8,6 +8,7 @@ TESTSDIR= ${TESTSBASE}/usr.sbin/pw
ATF_TESTS_SH= pw_etcdir \
pw_lock \
pw_config \
+ pw_groupadd \
pw_groupdel \
pw_groupmod \
pw_useradd \
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 '9': too
large\n \
+ ${PW} groupadd -n test1 -g 9
+}
+
+atf_init_test_cases() {
+ atf_add_test_case group_add_gid_too_large
+}
Modified: head/usr.sbin/pw/tests/pw_useradd.sh
==
--- head/usr.sbin/pw/tests/pw_useradd.shTue Jul 28 20:52:10 2015
(r285984)
+++ head/usr.sbin/pw/tests/pw_useradd.shTue Jul 28 21:10:58 2015
(r285985)
@@ -289,6 +289,13 @@ user_add_uid0_body() {
-s exit:0 ${PW} usershow foo
}
+atf_test_case user_add_uid_too_large
+user_add_uid_too_large_body() {
+ populate_etc_skel
+ atf_check -s exit:64 -e inline:pw: Bad id '9': too
large\n \
+ ${PW} useradd -n test1 -u 9
+}
+
atf_init_test_cases() {
atf_add_test_case user_add
atf_add_test_case user_add_noupdate
@@ -313,4 +320,5 @@ atf_init_test_cases() {
atf_add_test_case user_add_R
atf_add_test_case user_add_skel
atf_add_test_case user_add_uid0
+ atf_add_test_case user_add_uid_too_large
}
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
