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.
Created attachment 932 [details] 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.
Comment on attachment 932 [details] Patch for protected password database update on bad login >+ 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.
Created attachment 936 [details] Updated patch on protected password database for bad login The patch is now updated based on comments from Darren.
Comment on attachment 936 [details] Updated patch on protected password database for bad login >+ 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?
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.