fix usermod -Z / -S

[Sebastian Benoit]

I am locking an account with

# doas usermod -Z foobar

after that, i want to remove the user from all groups:

# doas usermod -S '' foobar
usermod: Invalid password: `*$2b$09$Pp.mDUEORDRbCUUy4D.Vf.EhvxVA.B1u0T7VAlsKN7sU7wqhs0l3W'

This happens in -current and is caused by usr.sbin/user/user.c as of

/* $OpenBSD: user.c,v 1.111 2016/05/03 21:05:14 mestre Exp $ */

which was done to fix another bug.

The problem with that change is this change

+ if (up != NULL) {
+ if ((*pwp->pw_passwd != '\0') && (up->u_flags &~ F_PASSWORD)) {
+ up->u_flags |= F_PASSWORD;

which sets F_PASSWORD and causes this bit to check the password

if (up->u_flags & F_PASSWORD) {
if (up->u_password != NULL) {
if (!valid_password_length(up->u_password)) {
(void) close(ptmpfd);
pw_abort();
errx(EXIT_FAILURE, "Invalid password: `%s'",
up->u_password);
}
pwp->pw_passwd = up->u_password;
}
}


Of course this will not only affect -S, but all other changes that are done
on an account with an invalid password.

from usermod(8):

-Z Lock the account by appending a `-' to the user's shell and
prefixing the password with `*'. -Z and -U are mutually
exclusive and cannot be used with -p.

and

-S secondary-group[,group,...]
Sets the secondary groups the user will be a member of in the
/etc/group file. Setting secondary-group to an empty value
(e.g. '') removes the user from all secondary groups.

This fix uses a new flag for the check and does not verify if the password
is valid in that case.

ok?

diff --git usr.sbin/user/user.c usr.sbin/user/user.c
index 27344a7..c4c33d6 100644
--- usr.sbin/user/user.c
+++ usr.sbin/user/user.c
@@ -103,7 +103,8 @@ enum {
F_CLASS = 0x1000,
F_SETSECGROUP = 0x4000,
F_ACCTLOCK = 0x8000,
- F_ACCTUNLOCK = 0x10000
+ F_ACCTUNLOCK = 0x10000,
+ F_KEEPPASSWORD = 0x20000
};

#define CONFFILE "/etc/usermgmt.conf"
@@ -1410,7 +1411,7 @@ moduser(char *login_name, char *newlogin, user_t *up)
}
if (up != NULL) {
if ((*pwp->pw_passwd != '\0') && (up->u_flags &~ F_PASSWORD)) {
- up->u_flags |= F_PASSWORD;
+ up->u_flags |= F_KEEPPASSWORD;
memsave(&up->u_password, pwp->pw_passwd,
strlen(pwp->pw_passwd));
memset(pwp->pw_passwd, 'X', strlen(pwp->pw_passwd));
@@ -1486,6 +1487,9 @@ moduser(char *login_name, char *newlogin, user_t *up)
pwp->pw_passwd = up->u_password;
}
}
+ if (up->u_flags & F_KEEPPASSWORD) {
+ pwp->pw_passwd = up->u_password;
+ }
if (up->u_flags & F_ACCTLOCK) {
/* lock the account */
if (*shell_last_char != *acctlock_str) {

[Todd C. Miller]

I'd prefer it if we just removed the invalid password check entirely.

- todd

[Todd C. Miller]

Here is a diff to remove the encrypted password length check. I
don't believe that user(8) has any business mucking about with
either existing encrypted passwords in master.password or with the
password specified by the user.

This also eliminates the exceptionally ugly 13 '*' character entries
that I have always despised.

- todd

Index: usr.sbin/user/user.c
===================================================================
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.111
diff -u -p -u -r1.111 user.c
--- usr.sbin/user/user.c 3 May 2016 21:05:14 -0000 1.111
+++ usr.sbin/user/user.c 5 Aug 2016 18:57:30 -0000
@@ -164,9 +164,6 @@ enum {
MaxUserNameLen = _PW_NAME_LEN,
MaxCommandLen = 2048,
PasswordLength = _PASSWORD_LEN,
-
- DES_Len = 13,
-
LowGid = DEF_LOWUID,
HighGid = DEF_HIGHUID
};
@@ -855,51 +852,6 @@ getnextuid(int sync_uid_gid, uid_t *uid,
return 0;
}

-/* structure which defines a password type */
-typedef struct passwd_type_t {
- const char *type; /* optional type descriptor */
- int desc_length; /* length of type descriptor */
- int length; /* length of password */
-} passwd_type_t;
-
-#define NBLF "$2b"
-#define BLF "$2a"
-#define MD5 "$1"
-#define DES ""
-
-static passwd_type_t passwd_types[] = {
- { NBLF, 3, 54 }, /* Blowfish bcrypt version 2b */
- { BLF, 3, 54 }, /* Blowfish */
- { MD5, 2, 34 }, /* MD5 */
- { DES, 0, DES_Len }, /* standard DES */
- { NULL, -1, -1 } /* none - terminate search */
-};
-
-/* return non-zero if it's a valid password - check length for cipher type */
-static int
-valid_password_length(char *newpasswd)
-{
- passwd_type_t *pwtp;
-
- for (pwtp = passwd_types ; pwtp->desc_length >= 0 ; pwtp++) {
- if (strncmp(newpasswd, pwtp->type, pwtp->desc_length) == 0) {
- char *p;
-
- if (strcmp(pwtp->type, BLF) != 0 &&
- strcmp(pwtp->type, NBLF) != 0) {
- return strlen(newpasswd) == pwtp->length;
- }
- /* Skip first three `$'. */
- if ((p = strchr(newpasswd, '$')) == NULL ||
- *(++p) == '$' || (p = strchr(p, '$')) == NULL ||
- *(++p) == '$' || (p = strchr(p, '$')) == NULL)
- continue;
- return (strlen(p) - 1);
- }
- }
- return 0;
-}
-
/* look for a valid time, return 0 if it was specified but bad */
static int
scantime(time_t *tp, char *s)
@@ -1130,16 +1082,8 @@ adduser(char *login_name, user_t *up)
warnx("Warning: home directory `%s' doesn't exist, and -m was"
" not specified", home);
}
- if (up->u_password != NULL && valid_password_length(up->u_password)) {
- (void) strlcpy(password, up->u_password, sizeof(password));
- } else {
- (void) memset(password, '*', DES_Len);
- password[DES_Len] = 0;
- if (up->u_password != NULL) {
- warnx("Password `%s' is invalid: setting it to `%s'",
- up->u_password, password);
- }
- }
+ (void) strlcpy(password, up->u_password ? up->u_password : "*",
+ sizeof(password));
cc = snprintf(buf, sizeof(buf), "%s:%s:%u:%u:%s:%lld:%lld:%s:%s:%s\n",
login_name,
password,
@@ -1476,15 +1420,8 @@ moduser(char *login_name, char *newlogin

}
}
if (up->u_flags & F_PASSWORD) {

- if (up->u_password != NULL) {
- if (!valid_password_length(up->u_password)) {
- (void) close(ptmpfd);
- pw_abort();
- errx(EXIT_FAILURE, "Invalid password: `%s'",
- up->u_password);
- }
+ if (up->u_password != NULL)

pwp->pw_passwd = up->u_password;

- }

}
if (up->u_flags & F_ACCTLOCK) {
/* lock the account */

@@ -2028,7 +1965,6 @@ userdel(int argc, char **argv)
{
struct passwd *pwp;
user_t u;
- char password[PasswordLength + 1];
int defaultfield;
int rmhome;
int bigD;
@@ -2086,9 +2022,7 @@ userdel(int argc, char **argv)
if (u.u_preserve) {
u.u_flags |= F_SHELL;
memsave(&u.u_shell, NOLOGIN, strlen(NOLOGIN));
- (void) memset(password, '*', DES_Len);
- password[DES_Len] = 0;
- memsave(&u.u_password, password, strlen(password));
+ memsave(&u.u_password, "*", strlen("*"));
u.u_flags |= F_PASSWORD;
openlog("userdel", LOG_PID, LOG_USER);
return moduser(*argv, *argv, &u) ? EXIT_SUCCESS : EXIT_FAILURE;