openssh bugs - Jun 2005 - [Bug 1058] Updating protected password database in HP-UX

http://bugzilla.mindrot.org/show_bug.cgi?id=1058

           Summary: Updating protected password database in HP-UX
           Product: Portable OpenSSH
           Version: 4.1p1
          Platform: All
        OS/Version: HP-UX
            Status: NEW
          Severity: normal
          Priority: P2
         Component: sshd
        AssignedTo: bitbucket at mindrot.org
        ReportedBy: senthilkumar_sen at hotpop.com


The HP-UX protected password database (trusted system) have an feature to track 
the number of consecutive unsuccessful login entries and locks the account if it
exceeds the allowed limit. Currently, this update takes place with the help of 
PAM Modules. It would be helpful for the users of HP-UX if OpenSSH extends this 
update for key based auth. methods.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1058





------- Additional Comments From senthilkumar_sen at hotpop.com  2005-06-29
16:10 -------
Created an attachment (id=932)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=932&action=view)
Patch for protected password database update on bad login

The attached patch update the protected password database of the user in HP-UX
for each bad login entry. The patch is taken against OpenSSH 4.1p1. I like to
hear comments on incorporating this feature on OpenSSH.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1058





------- Additional Comments From dtucker at zip.com.au  2005-06-29 16:23 -------
(From update of attachment 932)
>+                pr=getprpwnam((char *)username);
>+                if(!pr->uflg.fg_nlogins)
You need to check getprpwnam for failure (ie pr == NULL) otherwise this will
segfault if getprpwnam fails.
>+                        pr->uflg.fg_nlogins=1;
The man pages (putprpwnam, from memory) say that you must copy and update the
record rather than mangling the one that getprpwnam returns.

[...]
>+if(!authctxt->postponed && !authenticated &&
options.use_pam && strcmp(method,"
>none") && strcmp(method, "password") &&
strcmp(method, "challenge-res
>+ponse") && strcmp(method,
"keyboard-interactive/pam"))
>+       PRIVSEP(update_trusted_badlogins(authctxt->user));
Why not use the CUSTOM_FAILED_LOGIN hook?  That's what it's for, and
would not
require as much code.

In principle, I'm OK with updating the failed login counter for Trusted
Mode.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1058


senthilkumar_sen at hotpop.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #932 is|0                           |1
           obsolete|                            |




------- Additional Comments From senthilkumar_sen at hotpop.com  2005-07-05
22:32 -------
Created an attachment (id=936)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=936&action=view)
Updated patch on protected password database for bad login

The patch is now updated based on comments from Darren. 



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1058





------- Additional Comments From dtucker at zip.com.au  2005-07-05 23:42 -------
(From update of attachment 936)
>+                pr=getprpwnam((char *)username);
>+                putpr=pr;
You're just copying a pointer to the struct not copying the struct itself.
>+                if(putpr != NULL) {
>+                       if(!putpr->uflg.fg_nlogins)
>+                               putpr->uflg.fg_nlogins=1;
>+                putpr->ufld.fd_nlogins++;
This will segfault if getprpwnam fails too.
>+                putprpwnam(username,putpr);
ditto.

You can take advantage of the fact that it's a function call and return
early
rather than nesting your code (this tends to make it easier to read).  This
would look something like:

	struct pr_passwd *pr, putpr;

	if (!iscomsec())
		return;
	if ((pr = getprpwnam((char *)user)) == NULL)
		return;
	putpr = *pr;	/* copy and modify */
	putpr.uflg.fg_nlogins = 1;
	putpr.ufld.fd_nlogins++;
	putprpwnam((char *)user, &putpr);

Also this code is in auth.c which is one of the files we need to keep in sync,
it would be better elsewhere (auth-shadow.c is my first guess).
> #ifdef CUSTOM_FAILED_LOGIN
[...]
>+          if (authenticated == 0 && !authctxt->postponed
&& options.use_pam && strcmp(method, "none"))
Why "options.use_pam"?	If anything, I would have expected to skip this
if PAM
is enabled.
>+               PRIVSEP(update_trusted_badlogins(authctxt->user));
This is not quite what I had in mind, but I now see that because we already use
record_failed_login() for btmp logging and can't use it directly.  Let me
think
about it for a bit.

Also, are you ever clearing the counter on sucessful login?  Should you?




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1058





------- Additional Comments From senthilkumar_sen at hotpop.com  2005-07-09
00:39 -------
In reply to comment #4
>You're just copying a pointer to the struct not copying the struct
itself.
oops!, How I missed it ??
>it would be better elsewhere (auth-shadow.c is my first guess)
I will try this possibility.
>Why "options.use_pam"?If anything, I would have expected to skip
this if PAM
is enabled.

If PAM is enabled for hpux, it will take care of updating the counter as part of
authentication. But for key based authentications with PAM, skipping PAM
won't
update the counter. So its necessary for this update with PAM so that this 
feature is available for all Authentication methods.
> Let me think about it for a bit.
Ok, I will wait for some other possible options.
>Also, are you ever clearing the counter on sucessful login?  Should you?
Yes, it should be done. When I test with PAM modules they are clearing the 
counter after successful login. 







------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.