Project

General

Profile

Bug #5747

cannot lock user with NP already set

Added by Dave Eddy over 5 years ago. Updated over 5 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
lib - userland libraries
Start date:
2015-03-24
Due date:
% Done:

0%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

When a user has "NP" set as their password, attempting to lock them results in no change to their password. For example:

# getent shadow foo
foo:NP:::::::

# passwd -l foo
passwd: password information changed for foo

# getent shadow foo
foo:NP:16518::::::

The password at the end of the example is still set to "NP".

Looking into the code, it appears that when a lock is attempted on the shadow file, the password is not touch if 1) it already has "*LK*" at the beginning or 2) is set to NP. Assuming I understand the semantics of NP correctly, this behavior is incorrect. I've attached a patch that fixes this by simply not giving any special treatment to the string "NP" when locking a user.

diff --git a/usr/src/lib/passwdutil/files_attr.c b/usr/src/lib/passwdutil/files_attr.c
index 2a8ac5c..484bcee 100644
--- a/usr/src/lib/passwdutil/files_attr.c
+++ b/usr/src/lib/passwdutil/files_attr.c
@@ -737,9 +737,8 @@ files_update(attrlist *items, pwu_repository_t *rep, void *buf)
         case ATTR_LOCK_ACCOUNT:
             if (spw->sp_pwdp == NULL) {
                 spw->sp_pwdp = LOCKSTRING;
-            } else if ((strncmp(spw->sp_pwdp, LOCKSTRING,
-                sizeof (LOCKSTRING)-1) != 0) &&
-                (strcmp(spw->sp_pwdp, NOLOGINSTRING) != 0)) {
+            } else if (strncmp(spw->sp_pwdp, LOCKSTRING,
+                sizeof (LOCKSTRING)-1) != 0) {
                 len = sizeof (LOCKSTRING)-1 +
                     strlen(spw->sp_pwdp) + 1;
                 pword = malloc(len);

With this patch in place, this will result in the following behavior

# getent shadow foo
foo:NP:::::::

# passwd -l foo
passwd: password information changed for foo

# getent shadow foo
foo:*LK*NP:16518::::::

# passwd -u foo
passwd: password information changed for foo

# getent shadow foo
foo:NP:16518::::::

Here's the relevant chat log from #illumos on freenode

12:03:37       bahamas10  hey all, i asked this in #smartos pretty late last night but I think it may be more of a generic illumos thing
12:03:57       bahamas10  when locking a user using `passwd -l <user>`, no action is taken if the user has "NP" set as their password https://gist.github.com/bahamas10/9ab2115db23f81a4d7ca
12:04:52       bahamas10  digging into the source though, I can see that the code is setup to noop if the users password starts with "*LK*", or is set to "NP" exactly
12:04:53       bahamas10  https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/passwdutil/files_attr.c#L740-L742
12:05:09       bahamas10  so my question becomes, what is the purpose of "NP"?
12:05:26        LeftWing  So that you can have accounts that are not locked, but which will not allow password authentication.
12:05:29       bahamas10  when a user has NP, they are still able to login using SSH..
12:05:44        LeftWing  For SSH, it becomes a public key-only account.
12:05:56       bahamas10  LeftWing: ok, so then i've been using NP right
12:06:06       bahamas10  NP is considered unlocked correct?
12:06:18        LeftWing  That is correct
12:06:22       bahamas10  so, next question
12:06:23       bahamas10  https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/passwdutil/files_attr.c#L740-L742
12:06:35       bahamas10  the code skips locking the user, if the user has "NP" set as their password
12:06:59       bahamas10  (NOLOGINSTRING is defined as "NP")
12:08:10       bahamas10  which results in what i see in the gist i pasted. if a user has NP set, running `passwd -l <user>` doesn't lock them
12:09:44        LeftWing  I think that's probably the expected behaviour.
12:09:56        LeftWing  What are you wanting to happen by engaging the account lock?
12:10:42        LeftWing  I guess it should probably error out if it was unable to do the right thing.
12:10:56        LeftWing  Well, the _requested_ thing.
12:11:26       bahamas10  so, i guess my real question becomes what does locked actually mean?
12:11:34       bahamas10  I assumed that locking a user meant they could no longer SSH in
12:11:49       bahamas10  but perhaps that was an incorrect assumption
12:12:32        LeftWing  I think you're right, actually. The "-l" flag should probably clobber, or interact with, the "-N" flag to passwd (which is what sets NP in the shadow entry)
12:12:39        LeftWing  Rather than just ignore it.
12:13:19       bahamas10  LeftWing: that's what I imagined would happen
12:13:42       bahamas10  if you look at the code in files_attr.c, i would have assumed the "NP" checking logic would have been in the first if statement
12:14:01       bahamas10  ie. if the current password is NULL, or "NP", set it to "*LK*", else just prepend "*LK*" 
12:15:10        LeftWing  bahamas10: I think that actually *LK*NP makes sense.
12:15:28        LeftWing  That the lock flag should be orthogonal to the nopasswd flag.
12:17:55       bahamas10  LeftWing: ah ok
12:19:06        LeftWing  bahamas10: Are you planning to log an illumos bug?
12:19:12       bahamas10  LeftWing: doing it now
12:19:24        LeftWing  Something along the lines of 'passwd -N and -l flags should interact'
12:19:52        LeftWing  I don't think anybody can correctly be using the current behaviour -- i.e. "-l" fails silently if "-N" has been used.
12:20:16       bahamas10  LeftWing: agreed. mind if i quote some of this chat in the issue?
12:20:17        LeftWing  And, actually, it's dangerous to have it work that way -- given, as you point out, that non-password logins still work.
12:20:21        LeftWing  Sure!

Files

History

#1

Updated by Dave Eddy over 5 years ago

I've attached a new patch complete with webrev, copyright updated, and git pbchk output. In it I've added a block comment that explains all of the possible states of the password field and all of the possible transitions that can happen.

http://us-east.manta.joyent.com/bahamas10/public/webrev/5747-passwd/index.html

dave - sjc1-dave-01 sunos ~/dev/illumos-gate (git:master) $ git pbchk; echo $?
0
#4

Updated by Gary Mills over 5 years ago

Your comment is much larger than your source change. That's not a problem, but how much of your comment refers to the old code and how much refers to the new? I'd recommend removing anything that refers to the old code as it's no longer relevant. Just explain how the new code works.

#5

Updated by Dave Eddy over 5 years ago

Gary Mills wrote:

Your comment is much larger than your source change. That's not a problem, but how much of your comment refers to the old code and how much refers to the new? I'd recommend removing anything that refers to the old code as it's no longer relevant. Just explain how the new code works.

The comment only refers to how things work with the new code. ie. the comment only makes sense with the code change in place.

Also available in: Atom PDF