- start PAM with correct username - don't call pam_authenticate() for null password checking when not necessary - set pam_flags correctly for kbdint pam_authenticate() calls - improve logging
Created attachment 289 [details] PAM patches
Some comments: > - setproctitle("%s%s", authctxt->pw ? user : "unknown", > + setproctitle("%s%s", user, > use_privsep ? " [net]" : ""); We deliberately hide the username in logs and on the process list to avoid password disclosure in situations where the client has entered their password as their username (it happens...) > - PRIVSEP(start_pam(authctxt->pw == NULL ? "NOUSER" : user)); > + PRIVSEP(start_pam(user)); I am starting to change my mind that this may be correct. See Bug #117 > - PRIVSEP(start_pam("NOUSER")); > + PRIVSEP(start_pam(user)); > + authenticated = -1; /* signal illegal user */ authctxt->valid = 0 should obviate the need for the authenticated = -1, no? > + /* > + * REDACTED > + * REDACTED > ... What is this? > - retval = (do_pam_authenticate(0) == PAM_SUCCESS); > + retval = (do_pam_authenticate(options.permit_empty_passwd == 0 > + ? PAM_DISALLOW_NULL_AUTHTOK > + : 0) == PAM_SUCCESS); Is this still necessary with the CVS -current PAM code?
Created attachment 292 [details] revised PAM patch revised patches based on djm comments
> @@ -186,8 +186,8 @@ input_userauth_request(int type, u_int32 > m = authmethod_lookup(method); > if (m != NULL) { > debug2("input_userauth_request: try method %s", method); > - authenticated = m->userauth(authctxt); > + authenticated = m->userauth(authctxt) && authctxt->valid; > } > userauth_finish(authctxt, authenticated, method); This chunk is not necessary, as userauth_finish does: > if (!authctxt->valid && authenticated) > fatal("INTERNAL ERROR: authenticated invalid user %s", > authctxt->user); and no auth method should set authenticated = 1 for a non existant user :)
The chunks like: > - PRIVSEP(start_pam("NOUSER")); > + PRIVSEP(start_pam(user)); have been committed under bug #117
> > @@ -186,8 +186,8 @@ input_userauth_request(int type, u_int32 ... > This chunk is not necessary, as userauth_finish does: I didn't want to second guess what userauth_finish() would do (for maintainability going forward). Prior to the patch, userauth_finish() would never be called with authenticated=1 && authctxt->valid=0. Hence the fatal(), I guess! I wanted to preserve that assumption. > and no auth method should set authenticated = 1 for a non existant user :) You can't know what PAM will do. I had another patch where getpwnam() wouldn't run until after PAM was called. This gives PAM the chance to change the username, which it's allowed to do. FWIW, I actually have a valid use for that behavior (not just having a feature for feature's sake). A device that logs folks in to a single role account, but using individual usernames and secrets. Via PAM, that's possible to setup so that (eg) the auth goes to radius for secret verification, then the last module in the stack changes the username. The advantage is: no account maintenance on the device. I couldn't use the :style nicety because that is already used to access specific features when logging in. (I could have done something like :user.style but opted for PAM--seems cleaner.) But regardless of that use, again, you cannot know what PAM will do.
Hi Frank, > FWIW, I actually have a valid use for that behavior (not just having a > feature for feature's sake). A device that > logs folks in to a single role account, but using individual usernames > and secrets. Via PAM, that's possible to setup so that (eg) the auth > goes to radius for secret verification, then the last module in the > stack changes the username. The advantage is: no account maintenance we've got this situation here, too. RADIUS maps a nonexistent user name into a valid one; but OpenSSH 3.7.1p2 fails. Any pointer to your patch - couldn't find it in Bugzilla? /Joerg
Comment on attachment 292 [details] revised PAM patch OK, except for the last bit, I think this is all done. >+#ifdef USE_PAM >+ options.permit_empty_passwd && >+#endif This is done in auth-passwd.c: if (*password == '\0' && options.permit_empty_passwd == 0) return 0; >- PRIVSEP(start_pam(authctxt->pw == NULL ? "NOUSER" : user)); >+ PRIVSEP(start_pam(user)); Fixed a while back. >- if (pam_retval == PAM_SUCCESS && pw) { >+ if (pam_retval == PAM_SUCCESS) { > debug("PAM password authentication accepted for " >- "%.100s", pw->pw_name); >+ "%.100s", authctxt->user); All of the references to the username in auth-pam.c are now authctxt->user. >+ authenticated = m->userauth(authctxt) && authctxt->valid; Not currently needed, see comment #5. (We can review this should it ever become necessary). > /* Log before sending the reply */ >- auth_log(authctxt, authenticated, method, " ssh2"); >+ /* >+ * With an exception: don't log 'none' failures if empty passwords >+ * are not allowed; the openssh client ALWAYS requests none just >+ * to get the list of auth methods, so this is too noisy. >+ */ >+ if (!(!strcmp(method, "none") && /* method 'none' */ >+ !options.permit_empty_passwd && /* none !allowed */ >+ !authenticated)) /* failed auth */ >+ auth_log(authctxt, authenticated, method, " ssh2"); I don't see why this in needed. Until you get to options.max_authtries/2 failures (which used to be hard-coded to AUTH_FAIL_MAX/2 = 3) it will only get logged at "verbose" level anyway. >+ if (!options.password_authentication || !options.permit_empty_passwd) >+ return(0); Handled in auth-passwd.c (see above). >- retval = (do_pam_authenticate(0) == PAM_SUCCESS); >+ retval = (do_pam_authenticate(options.permit_empty_passwd == 0 >+ ? PAM_DISALLOW_NULL_AUTHTOK >+ : 0) == PAM_SUCCESS); > dispatch_set(SSH2_MSG_USERAUTH_INFO_RESPONSE, NULL); This one should probably be ported to -current (will attach a patch).
Created attachment 675 [details] pass PAM_DISALLOW_NULL_AUTHTOK to kbdint too.
Comment on attachment 675 [details] pass PAM_DISALLOW_NULL_AUTHTOK to kbdint too. ok
Committed #675 so I think this is bug is done. Thank all, closing.