From 0f9cc4e11065a590307162faed564325ae438b15 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Tue, 8 Aug 2017 12:02:37 +0200 Subject: [PATCH 1/3] pw useradd: Validate the user name before creating the entry Previouly it was possible to create users with spaces in the name with: pw useradd -u 1234 -g 1234 -n 'test user' The "-g 1234" is relevant, without it the name was already rejected as expected: [fk@test ~]$ sudo pw useradd -u 1234 -n 'test user' pw: invalid character ` ' at position 4 in userid/group name Bug unintentionally found with a salt config without explicit name entry: test user: user.present: - uid: 1234 - gid: 1234 - fullname: Test user - shell: /usr/local/bin/bash - home: /home/test - groups: - wheel - salt "Luckily" salt modules rarely bother with input validation either ... Obtained from: ElectroBSD --- usr.sbin/pw/pw_user.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/usr.sbin/pw/pw_user.c b/usr.sbin/pw/pw_user.c index 73f43ea43865..a5c334fb2d72 100644 --- a/usr.sbin/pw/pw_user.c +++ b/usr.sbin/pw/pw_user.c @@ -1202,7 +1202,7 @@ pw_user_add(int argc, char **argv, char *arg1) if (arg1[strspn(arg1, "0123456789")] == '\0') id = pw_checkid(arg1, UID_MAX); else - name = arg1; + name = pw_checkname(arg1, 0); } while ((ch = getopt(argc, argv, args)) != -1) { @@ -1214,7 +1214,7 @@ pw_user_add(int argc, char **argv, char *arg1) quiet = true; break; case 'n': - name = optarg; + name = pw_checkname(optarg, 0); break; case 'u': userid = optarg; -- 2.13.2 From 59effa6b075d26f6f8f3144b6748357427b62916 Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Tue, 8 Aug 2017 14:30:24 +0200 Subject: [PATCH 2/3] usr.sbin/pw/tests: Add test to confirm that user names with spaces are rejected ... if no group id is specified. Obtained from: ElectroBSD --- usr.sbin/pw/tests/pw_useradd_test.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/usr.sbin/pw/tests/pw_useradd_test.sh b/usr.sbin/pw/tests/pw_useradd_test.sh index d14e4dde82ee..4303f2ad4cb0 100755 --- a/usr.sbin/pw/tests/pw_useradd_test.sh +++ b/usr.sbin/pw/tests/pw_useradd_test.sh @@ -176,6 +176,18 @@ user_add_name_too_long_body() { ${PW} useradd name_very_vert_very_very_very_long } +atf_test_case user_add_name_with_spaces +user_add_name_with_spaces_body() { + populate_etc_skel + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd 'test user' + atf_check -s exit:1 -o empty grep "^test user:.*" $HOME/master.passwd + # Try again with the -n option which uses a slightly different code path. + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd -n 'test user' + atf_check -s exit:1 -o empty grep "^test user:.*" $HOME/master.passwd +} + atf_test_case user_add_expiration user_add_expiration_body() { populate_etc_skel @@ -415,6 +427,7 @@ atf_init_test_cases() { atf_add_test_case user_add_password_expiration_date_month atf_add_test_case user_add_password_expiration_date_relative atf_add_test_case user_add_name_too_long + atf_add_test_case user_add_name_with_spaces atf_add_test_case user_add_expiration atf_add_test_case user_add_invalid_user_entry atf_add_test_case user_add_invalid_group_entry -- 2.13.2 From dd2654d86452761ca3a570d20a86e91bd710744b Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Tue, 8 Aug 2017 14:44:12 +0200 Subject: [PATCH 3/3] usr.sbin/pw/tests: Add test to verify that invalid user names are caught ... even if the -g option is specified. Obtained from: ElectroBSD --- usr.sbin/pw/tests/pw_useradd_test.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/usr.sbin/pw/tests/pw_useradd_test.sh b/usr.sbin/pw/tests/pw_useradd_test.sh index 4303f2ad4cb0..1569b06585c0 100755 --- a/usr.sbin/pw/tests/pw_useradd_test.sh +++ b/usr.sbin/pw/tests/pw_useradd_test.sh @@ -188,6 +188,31 @@ user_add_name_with_spaces_body() { atf_check -s exit:1 -o empty grep "^test user:.*" $HOME/master.passwd } +atf_test_case user_add_name_with_spaces_and_gid_specified +user_add_name_with_spaces_and_gid_specified_body() { + populate_etc_skel + gid=12345 + user_name="test user" + # pw useradd should fail because of the space in the user + # name, not because the group doesn't exist. + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd "${user_name}" -g ${gid} + atf_check -s exit:1 -o empty grep "^${user_name}:.*" $HOME/master.passwd + # Try again with the -n option which uses a slightly different code path. + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd -n "${user_name}" -g ${gid} + atf_check -s exit:1 -o empty grep "^${user_name}:.*" $HOME/master.passwd + # Make sure the user isn't added even if the group exists + atf_check -s exit:0 ${PW} groupadd blafasel -g ${gid} + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd "${user_name}" -g ${gid} + atf_check -s exit:1 -o empty grep "^${user_name}:.*" $HOME/master.passwd + # Try again with the -n option. + atf_check -s exit:65 -e match:"invalid character" \ + ${PW} useradd -n "${user_name}" -g ${gid} + atf_check -s exit:1 -o empty grep "^${user_name}:.*" $HOME/master.passwd +} + atf_test_case user_add_expiration user_add_expiration_body() { populate_etc_skel @@ -428,6 +453,7 @@ atf_init_test_cases() { atf_add_test_case user_add_password_expiration_date_relative atf_add_test_case user_add_name_too_long atf_add_test_case user_add_name_with_spaces + atf_add_test_case user_add_name_with_spaces_and_gid_specified atf_add_test_case user_add_expiration atf_add_test_case user_add_invalid_user_entry atf_add_test_case user_add_invalid_group_entry -- 2.13.2