Mailing List Archive

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





------- Additional Comments From senthilkumar_sen@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@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@hotpop.com changed:

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




------- Additional Comments From senthilkumar_sen@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@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@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.