Re: Race condition on change of expired password?

Russ Allbery <[email protected]> writes:

> It doesn't seem to *just* be a race condition.  I redid this test inside
> the Kerberos PAM module and added a forced five second delay in the
> prompter after printing the success message, but the subsequent AS-REQ
> still fails:

I think the code may just be wrong here, although that surprises me since
I would have thought someone would have noticed.

In lib/krb5/init_creds_pw.c, krb5_get_init_creds_password does:

    if (ret == KRB5KDC_ERR_KEY_EXPIRED && chpw == 0) {
        char buf2[1024];

        /* try to avoid recursion */
        if (in_tkt_service != NULL && strcmp(in_tkt_service, "kadmin/changepw") == 0)
           goto out;

        /* don't try to change password where then where none */
        if (prompter == NULL)
            goto out;

        ret = change_password (context,
                               client,
                               ctx->password,
                               buf2,
                               sizeof(buf),
                               prompter,
                               data,
                               options);
        if (ret)
            goto out;
        chpw = 1;
        krb5_init_creds_free(context, ctx);
        goto again;
    }

change_password puts the new password in the fourth argument to that
function, which is buf2.  (Passing in sizeof(buf) instead of sizeof(buf2)
also looks wrong, since BUFSIZ may be larger than 1024.)  But then nothing
is ever done with buf2 before the function goes back to the top and does
the authentication again.  The authentication will therefore be retried
with the original password, and of course fail.

I think buf2 needs to be lifted to the outside scope, and in the above
change password code there should be a:

    password = buf2;

before retrying so that the new password is used for the retry.

Also, buf2 needs to be cleared.

Attached is a proposed patch, although I've not tested it.  Also pushed to
rra/heimdal on github.

-- 
Russ Allbery ([email protected])             <http://www.eyrie.org/~eagle/>

From d782936bb75112ca0609bf4dc6602f302023aeb7 Mon Sep 17 00:00:00 2001
From: Russ Allbery <[email protected]>
Date: Wed, 21 Dec 2011 18:38:36 -0800
Subject: [PATCH] Fix reauthentication after password change in
 init_creds_password

When retrying authentication after a password change of an expired
password, use the new password instead of the original one.  Also,
pass in the correct length for the new password buffer to
change_password and zero the buffer that holds the new password on
function exit.

Signed-off-by: Russ Allbery <[email protected]>
---
 lib/krb5/init_creds_pw.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/krb5/init_creds_pw.c b/lib/krb5/init_creds_pw.c
index f6f6ff5..24cecb6 100644
--- a/lib/krb5/init_creds_pw.c
+++ b/lib/krb5/init_creds_pw.c
@@ -2422,7 +2422,7 @@ krb5_get_init_creds_password(krb5_context context,
 			     krb5_get_init_creds_opt *options)
 {
     krb5_init_creds_context ctx;
-    char buf[BUFSIZ];
+    char buf[BUFSIZ], buf2[BUFSIZ];
     krb5_error_code ret;
     int chpw = 0;
 
@@ -2474,8 +2474,6 @@ krb5_get_init_creds_password(krb5_context context,
 
 
     if (ret == KRB5KDC_ERR_KEY_EXPIRED && chpw == 0) {
-	char buf2[1024];
-
 	/* try to avoid recursion */
 	if (in_tkt_service != NULL && strcmp(in_tkt_service, "kadmin/changepw") == 0)
 	   goto out;
@@ -2488,12 +2486,13 @@ krb5_get_init_creds_password(krb5_context context,
 			       client,
 			       ctx->password,
 			       buf2,
-			       sizeof(buf),
+			       sizeof(buf2),
 			       prompter,
 			       data,
 			       options);
 	if (ret)
 	    goto out;
+	password = buf2;
 	chpw = 1;
 	krb5_init_creds_free(context, ctx);
 	goto again;
@@ -2507,6 +2506,7 @@ krb5_get_init_creds_password(krb5_context context,
 	krb5_init_creds_free(context, ctx);
 
     memset(buf, 0, sizeof(buf));
+    memset(buf2, 0, sizeof(buf2));
     return ret;
 }
 
-- 
1.7.7.3