On Thu, Jul 25, 2019 at 06:33:00PM +0200, Tomas Mraz wrote: > This patch solves double prompt for password verification if you > have > stacked modules which explicitly call pam_get_authtok_verify. > > For example this stack of modules will trigger such behavior: > > pam_pwhistory.so > pam_cracklib.so > pam_unix.so > > Please review, > > -- > Tomáš Mráz > No matter how far down the wrong road you've gone, turn back. > Turkish proverb > [You'll know whether the road is wrong if you carefully listen to > your > conscience.] > > diff --git a/libpam/pam_get_authtok.c b/libpam/pam_get_authtok.c > index 800c6e5..cda7432 100644 > --- a/libpam/pam_get_authtok.c > +++ b/libpam/pam_get_authtok.c > @@ -140,6 +140,8 @@ pam_get_authtok_internal (pam_handle_t *pamh, > int item, > } > else if (chpass) > { > + pamh->authtok_verified = 0; > + > retval = pam_prompt (pamh, PAM_PROMPT_ECHO_OFF, &resp[0], > PROMPT1, authtok_type, > strlen (authtok_type) > 0?" ":""); > @@ -165,14 +167,18 @@ pam_get_authtok_internal (pam_handle_t *pamh, > int item, > return PAM_AUTHTOK_ERR; > } > > - if (chpass > 1 && strcmp (resp[0], resp[1]) != 0) > + if (chpass > 1) > { > - pam_error (pamh, MISTYPED_PASS); > - _pam_overwrite (resp[0]); > - _pam_drop (resp[0]); > - _pam_overwrite (resp[1]); > - _pam_drop (resp[1]); > - return PAM_TRY_AGAIN; > + if (strcmp (resp[0], resp[1]) != 0) > + { > + pam_error (pamh, MISTYPED_PASS); > + _pam_overwrite (resp[0]); > + _pam_drop (resp[0]); > + _pam_overwrite (resp[1]); > + _pam_drop (resp[1]); > + return PAM_TRY_AGAIN; > + } > + pamh->authtok_verified = 1; > } I think pamh->authtok_verified should be set later in pam_get_authtok_internal at the point before the last pam_get_item invocation, for consistency with ...

> _pam_overwrite (resp[1]); > @@ -214,6 +220,9 @@ pam_get_authtok_verify (pam_handle_t *pamh, > const char **authtok, > if (authtok == NULL || pamh->choice != PAM_CHAUTHTOK) > return PAM_SYSTEM_ERR; > > + if (pamh->authtok_verified) > + return pam_get_item (pamh, PAM_AUTHTOK, (const void > **)authtok); > + > if (prompt != NULL) > { > retval = pam_prompt (pamh, PAM_PROMPT_ECHO_OFF, &resp, > @@ -239,6 +248,7 @@ pam_get_authtok_verify (pam_handle_t *pamh, > const char **authtok, > > if (strcmp (*authtok, resp) != 0) > { > + pamh->authtok_verified = 0; > pam_set_item (pamh, PAM_AUTHTOK, NULL); > pam_error (pamh, MISTYPED_PASS); > _pam_overwrite (resp); > @@ -252,5 +262,7 @@ pam_get_authtok_verify (pam_handle_t *pamh, > const char **authtok, > if (retval != PAM_SUCCESS) > return retval; > > + pamh->authtok_verified = 1; > + > return pam_get_item(pamh, PAM_AUTHTOK, (const void **)authtok); > } ... this part of the patch. > diff --git a/libpam/pam_private.h b/libpam/pam_private.h > index 7ff9f75..58a26f5 100644 > --- a/libpam/pam_private.h > +++ b/libpam/pam_private.h > @@ -172,6 +172,7 @@ struct pam_handle { > #ifdef HAVE_LIBAUDIT > int audit_state; /* keep track of reported audit > messages */ > #endif > + int authtok_verified; > }; > > /* Values for select arg to _pam_dispatch() */ > diff --git a/libpam/pam_start.c b/libpam/pam_start.c > index 328416d..e27c64b 100644 > --- a/libpam/pam_start.c > +++ b/libpam/pam_start.c > @@ -94,6 +94,7 @@ int pam_start ( > #endif > (*pamh)->xdisplay = NULL; > (*pamh)->authtok_type = NULL; > + (*pamh)->authtok_verified = 0; > memset (&((*pamh)->xauth), 0, sizeof ((*pamh)->xauth)); A side note: looks like we don't trust calloc; is there any rationale for that?

On Tue, Aug 06, 2019 at 02:10:58PM +0200, Tomas Mraz wrote: [...] > @@ -214,6 +219,9 @@ pam_get_authtok_verify (pam_handle_t *pamh, > const char **authtok, > if (authtok == NULL || pamh->choice != PAM_CHAUTHTOK) > return PAM_SYSTEM_ERR; > > + if (pamh->authtok_verified) > + return pam_get_item (pamh, PAM_AUTHTOK, (const void > **)authtok); > + > if (prompt != NULL) > { > retval = pam_prompt (pamh, PAM_PROMPT_ECHO_OFF, &resp, OK, we check pamh->authtok_verified early in this function, so ... > @@ -239,6 +247,7 @@ pam_get_authtok_verify (pam_handle_t *pamh, > const char **authtok, > > if (strcmp (*authtok, resp) != 0) > { > + pamh->authtok_verified = 0; > pam_set_item (pamh, PAM_AUTHTOK, NULL); > pam_error (pamh, MISTYPED_PASS); > _pam_overwrite (resp); ... here pamh->authtok_verified is expected to be zero. Why it is reset again?

On Tue, Aug 06, 2019 at 07:20:32PM +0200, Tomas Mraz wrote: > On Tue, 2019-08-06 at 19:21 +0300, Dmitry V. Levin wrote: > > On Tue, Aug 06, 2019 at 02:10:58PM +0200, Tomas Mraz wrote: > > [...] > > > @@ -214,6 +219,9 @@ pam_get_authtok_verify (pam_handle_t *pamh, > > > const char **authtok, > > > if (authtok == NULL || pamh->choice != PAM_CHAUTHTOK) > > > return PAM_SYSTEM_ERR; > > > > > > + if (pamh->authtok_verified) > > > + return pam_get_item (pamh, PAM_AUTHTOK, (const void > > > **)authtok); > > > + > > > if (prompt != NULL) > > > { > > > retval = pam_prompt (pamh, PAM_PROMPT_ECHO_OFF, &resp, > > > > OK, we check pamh->authtok_verified early in this function, so > > ... > > > > > @@ -239,6 +247,7 @@ pam_get_authtok_verify (pam_handle_t *pamh, > > > const char **authtok, > > > > > > if (strcmp (*authtok, resp) != 0) > > > { > > > + pamh->authtok_verified = 0; > > > pam_set_item (pamh, PAM_AUTHTOK, NULL); > > > pam_error (pamh, MISTYPED_PASS); > > > _pam_overwrite (resp); > > > > ... here pamh->authtok_verified is expected to be zero. > > Why it is reset again? > > Yeah, that is redundant, kind of defensive programming. But I'll > drop > it, if you want. If we couldn't assume that pamh->authtok_verified remains zero, then we should reset it in other places where this function returns an error, too. So yes, let's drop it instead.