On Tue, 2019-08-06 at 14:33 +0300, Dmitry V. Levin wrote:
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 ...
Ok, see the updated patch attached.
> _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?
I do not think there is any rationale, however I believe there are
other things of this kind (do things twice for sure :)) in libpam.
We can do separate cleanup patch for that.
--
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.]