aklog: Free client/server princs in get_credv5 61/13761/6
authorYadavendra Yadav <yadayada@in.ibm.com>
Fri, 9 Aug 2019 21:24:38 +0000 (02:24 +0530)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 30 Aug 2019 16:11:36 +0000 (12:11 -0400)
Inside get_credv5, client_principal is static so the first time
get_credv5 runs we'll allocate memory for it, and on subsequent calls
we'll reuse the same value.

However, if we call get_credv5_akimpersonate, we'll free
client_principal and never change what client_principal points to. If we
need to call get_credv5 again (because we need to retry getting creds),
we'll reuse the old value for client_principal, but since it points to
free memory we'll segfault or cause other problems.

To avoid this, change get_credv5 so we allocate the client and server
principals on each invocation of get_credv5 and free them before
returning from get_credv5. Since we free the client and server
principals inside get_credv5, remove freeing the client and server
principals inside get_credv5_akimpersonate.

Change-Id: Ie263aa2c03efc75e818d9007347dca9e42380dd4
Reviewed-on: https://gerrit.openafs.org/13761
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>

src/aklog/aklog.c

index b753bff..f141554 100644 (file)
@@ -2119,10 +2119,6 @@ cleanup:
     if (deref_enc_data(&ticket_reply->enc_part))
         free(deref_enc_data(&ticket_reply->enc_part));
     krb5_free_keytab_entry_contents(context, entry);
-    if (client_principal)
-        krb5_free_principal(context, client_principal);
-    if (service_principal)
-        krb5_free_principal(context, service_principal);
     if (cc)
         krb5_cc_close(context, cc);
     if (kt)
@@ -2142,7 +2138,6 @@ get_credv5(krb5_context context, char *name, char *inst, char *realm,
 {
     krb5_creds increds;
     krb5_error_code r;
-    static krb5_principal client_principal = 0;
 
     afs_dprintf("Getting tickets: %s%s%s@%s\n", name,
            (inst && inst[0]) ? "/" : "", inst ? inst : "", realm);
@@ -2154,26 +2149,25 @@ get_credv5(krb5_context context, char *name, char *inst, char *realm,
                                  name,
                                  (inst && strlen(inst)) ? inst : NULL,
                                  NULL))) {
-        return r;
+        goto out;
     }
 
 
     if (!_krb425_ccache) {
         r = krb5_cc_default(context, &_krb425_ccache);
        if (r)
-           return r;
+           goto out;
     }
-    if (!client_principal) {
-       if (client) {
-           r = krb5_parse_name(context, client,  &client_principal);
-       } else {
-           r = krb5_cc_get_principal(context, _krb425_ccache, &client_principal);
-       }
-       if (r)
-           return r;
+
+    if (client) {
+        r = krb5_parse_name(context, client,  &increds.client);
+    } else {
+        r = krb5_cc_get_principal(context, _krb425_ccache, &increds.client);
     }
 
-    increds.client = client_principal;
+    if (r)
+       goto out;
+
     increds.times.endtime = 0;
     if (do524)
        /* Ask for DES since that is what V4 understands */
@@ -2195,9 +2189,19 @@ get_credv5(krb5_context context, char *name, char *inst, char *realm,
     } else {
        r = krb5_get_credentials(context, 0, _krb425_ccache, &increds, creds);
     }
-    return r;
-}
 
+ out:
+
+   if (increds.server) {
+       krb5_free_principal(context, increds.server);
+   }
+
+   if (increds.client) {
+       krb5_free_principal(context, increds.client);
+   }
+
+   return r;
+}
 
 static int
 get_user_realm(krb5_context context, char **realm)