Improve PKINIT client memory management

In pkinit_as_req_create, create and encode stack-allocated auth-pack
structures containing only alias pointers, instead of heap-allocated
structures containing a mix of alias pointers, owner pointers, and
appropriated caller memory.  Keep everything we temporarily allocate
in separate local variables and free them through those variables.

In pa_pkinit_gen_req, use safer memory practices to avoid problems
like issue #7878.  Free the checksum since pkinit_as_req_create no
longer takes ownership it.  Remove a broken overly defensive check
after calling pkinit_as_req_create.

Remove init_krb5_auth_pack and init_krb5_auth_pack_draft9 as they are
no longer required.

3 files changed, 54 insertions, 103 deletions

diff --git a/src/plugins/preauth/pkinit/pkinit.h b/src/plugins/preauth/pkinit/pkinit.h
index f9e1485d0..ee1334b8b 100644
--- a/src/plugins/preauth/pkinit/pkinit.h
+++ b/src/plugins/preauth/pkinit/pkinit.h
@@ -340,8 +340,6 @@ void init_krb5_pa_pk_as_req_draft9(krb5_pa_pk_as_req_draft9 **in);
 void init_krb5_reply_key_pack(krb5_reply_key_pack **in);
 void init_krb5_reply_key_pack_draft9(krb5_reply_key_pack_draft9 **in);
 
-void init_krb5_auth_pack(krb5_auth_pack **in);
-void init_krb5_auth_pack_draft9(krb5_auth_pack_draft9 **in);
 void init_krb5_pa_pk_as_rep(krb5_pa_pk_as_rep **in);
 void init_krb5_pa_pk_as_rep_draft9(krb5_pa_pk_as_rep_draft9 **in);
 void init_krb5_subject_pk_info(krb5_subject_pk_info **in);
diff --git a/src/plugins/preauth/pkinit/pkinit_clnt.c b/src/plugins/preauth/pkinit/pkinit_clnt.c
index cfef5b9dc..2a003700b 100644
--- a/src/plugins/preauth/pkinit/pkinit_clnt.c
+++ b/src/plugins/preauth/pkinit/pkinit_clnt.c
@@ -39,6 +39,7 @@
 #include <dlfcn.h>
 #include <sys/stat.h>
 
+#include "k5-int.h"
 #include "pkinit.h"
 #include "k5-json.h"
 
@@ -110,7 +111,7 @@ pa_pkinit_gen_req(krb5_context context,
     krb5_data *der_req = NULL;
     krb5_pa_data **return_pa_data = NULL;
 
-    cksum.contents = NULL;
+    memset(&cksum, 0, sizeof(cksum));
     reqctx->pa_type = pa_type;
 
     pkiDebug("kdc_options = 0x%x  till = %d\n",
@@ -158,31 +159,24 @@ pa_pkinit_gen_req(krb5_context context,
 
     retval = pkinit_as_req_create(context, plgctx, reqctx, ctsec, cusec,
                                   nonce, &cksum, request->client, request->server, &out_data);
-    if (retval || !out_data->length) {
+    if (retval) {
         pkiDebug("error %d on pkinit_as_req_create; aborting PKINIT\n",
                  (int) retval);
         goto cleanup;
     }
-    retval = ENOMEM;
+
     /*
      * The most we'll return is two pa_data, normally just one.
      * We need to make room for the NULL terminator.
      */
-    return_pa_data = malloc(3 * sizeof(krb5_pa_data *));
+    return_pa_data = k5calloc(3, sizeof(*return_pa_data), &retval);
     if (return_pa_data == NULL)
         goto cleanup;
 
-    return_pa_data[1] = NULL;   /* in case of an early trip to cleanup */
-    return_pa_data[2] = NULL;   /* Terminate the list */
-
-    return_pa_data[0] = malloc(sizeof(krb5_pa_data));
+    return_pa_data[0] = k5alloc(sizeof(*return_pa_data[0]), &retval);
     if (return_pa_data[0] == NULL)
         goto cleanup;
 
-    return_pa_data[1] = malloc(sizeof(krb5_pa_data));
-    if (return_pa_data[1] == NULL)
-        goto cleanup;
-
     return_pa_data[0]->magic = KV5M_PA_DATA;
 
     if (pa_type == KRB5_PADATA_PK_AS_REQ_OLD)
@@ -191,6 +185,7 @@ pa_pkinit_gen_req(krb5_context context,
         return_pa_data[0]->pa_type = pa_type;
     return_pa_data[0]->length = out_data->length;
     return_pa_data[0]->contents = (krb5_octet *) out_data->data;
+    *out_data = empty_data();
 
     /*
      * LH Beta 3 requires the extra pa-data, even for RFC requests,
@@ -199,31 +194,20 @@ pa_pkinit_gen_req(krb5_context context,
      */
     if ((return_pa_data[0]->pa_type == KRB5_PADATA_PK_AS_REP_OLD
          && reqctx->opts->win2k_require_cksum) || (longhorn == 1)) {
+        return_pa_data[1] = k5alloc(sizeof(*return_pa_data[1]), &retval);
+        if (return_pa_data[1] == NULL)
+            goto cleanup;
         return_pa_data[1]->pa_type = KRB5_PADATA_AS_CHECKSUM;
-        return_pa_data[1]->length = 0;
-        return_pa_data[1]->contents = NULL;
-    } else {
-        free(return_pa_data[1]);
-        return_pa_data[1] = NULL;   /* Move the list terminator */
     }
+
     *out_padata = return_pa_data;
-    retval = 0;
+    return_pa_data = NULL;
 
 cleanup:
-    if (der_req != NULL)
-        krb5_free_data(context, der_req);
-
-    if (retval) {
-        if (return_pa_data) {
-            free(return_pa_data[0]);
-            free(return_pa_data[1]);
-            free(return_pa_data);
-        }
-        if (out_data) {
-            free(out_data->data);
-        }
-    }
-    free(out_data);
+    krb5_free_data(context, der_req);
+    krb5_free_checksum_contents(context, &cksum);
+    krb5_free_data(context, out_data);
+    krb5_free_pa_data(context, return_pa_data);
     return retval;
 }
 
@@ -240,13 +224,16 @@ pkinit_as_req_create(krb5_context context,
                      krb5_data ** as_req)
 {
     krb5_error_code retval = ENOMEM;
-    krb5_subject_pk_info *info = NULL;
+    krb5_subject_pk_info info;
     krb5_data *coded_auth_pack = NULL;
-    krb5_auth_pack *auth_pack = NULL;
+    krb5_auth_pack auth_pack;
     krb5_pa_pk_as_req *req = NULL;
-    krb5_auth_pack_draft9 *auth_pack9 = NULL;
+    krb5_auth_pack_draft9 auth_pack9;
     krb5_pa_pk_as_req_draft9 *req9 = NULL;
+    krb5_algorithm_identifier **cmstypes = NULL;
     int protocol = reqctx->opts->dh_or_rsa;
+    unsigned char *dh_params = NULL, *dh_pubkey = NULL;
+    unsigned int dh_params_len, dh_pubkey_len;
 
     pkiDebug("pkinit_as_req_create pa_type = %d\n", reqctx->pa_type);
 
@@ -254,34 +241,28 @@ pkinit_as_req_create(krb5_context context,
     switch((int)reqctx->pa_type) {
     case KRB5_PADATA_PK_AS_REQ_OLD:
         protocol = RSA_PROTOCOL;
-        init_krb5_auth_pack_draft9(&auth_pack9);
-        if (auth_pack9 == NULL)
-            goto cleanup;
-        auth_pack9->pkAuthenticator.ctime = ctsec;
-        auth_pack9->pkAuthenticator.cusec = cusec;
-        auth_pack9->pkAuthenticator.nonce = nonce;
-        auth_pack9->pkAuthenticator.kdcName = server;
-        free(cksum->contents);
+        memset(&auth_pack9, 0, sizeof(auth_pack9));
+        auth_pack9.pkAuthenticator.ctime = ctsec;
+        auth_pack9.pkAuthenticator.cusec = cusec;
+        auth_pack9.pkAuthenticator.nonce = nonce;
+        auth_pack9.pkAuthenticator.kdcName = server;
         break;
     case KRB5_PADATA_PK_AS_REQ:
-        init_krb5_subject_pk_info(&info);
-        if (info == NULL)
-            goto cleanup;
-        init_krb5_auth_pack(&auth_pack);
-        if (auth_pack == NULL)
-            goto cleanup;
-        auth_pack->pkAuthenticator.ctime = ctsec;
-        auth_pack->pkAuthenticator.cusec = cusec;
-        auth_pack->pkAuthenticator.nonce = nonce;
-        auth_pack->pkAuthenticator.paChecksum = *cksum;
-        auth_pack->clientDHNonce.length = 0;
-        auth_pack->clientPublicValue = info;
-        auth_pack->supportedKDFs = (krb5_data **) supported_kdf_alg_ids;
+        memset(&info, 0, sizeof(info));
+        memset(&auth_pack, 0, sizeof(auth_pack));
+        auth_pack.pkAuthenticator.ctime = ctsec;
+        auth_pack.pkAuthenticator.cusec = cusec;
+        auth_pack.pkAuthenticator.nonce = nonce;
+        auth_pack.pkAuthenticator.paChecksum = *cksum;
+        auth_pack.clientDHNonce.length = 0;
+        auth_pack.clientPublicValue = &info;
+        auth_pack.supportedKDFs = (krb5_data **)supported_kdf_alg_ids;
 
         /* add List of CMS algorithms */
         retval = create_krb5_supportedCMSTypes(context, plgctx->cryptoctx,
-                                               reqctx->cryptoctx, reqctx->idctx,
-                                               &auth_pack->supportedCMSTypes);
+                                               reqctx->cryptoctx,
+                                               reqctx->idctx, &cmstypes);
+        auth_pack.supportedCMSTypes = cmstypes;
         if (retval)
             goto cleanup;
         break;
@@ -296,35 +277,29 @@ pkinit_as_req_create(krb5_context context,
     case DH_PROTOCOL:
         TRACE_PKINIT_CLIENT_REQ_DH(context);
         pkiDebug("as_req: DH key transport algorithm\n");
-        retval = pkinit_copy_krb5_data(&info->algorithm.algorithm, &dh_oid);
-        if (retval) {
-            pkiDebug("failed to copy dh_oid\n");
-            goto cleanup;
-        }
+        info.algorithm.algorithm = dh_oid;
 
         /* create client-side DH keys */
-        if ((retval = client_create_dh(context, plgctx->cryptoctx,
-                                       reqctx->cryptoctx, reqctx->idctx, reqctx->opts->dh_size,
-                                       (unsigned char **)
-                                       &info->algorithm.parameters.data,
-                                       &info->algorithm.parameters.length,
-                                       (unsigned char **)
-                                       &info->subjectPublicKey.data,
-                                       &info->subjectPublicKey.length)) != 0) {
+        retval = client_create_dh(context, plgctx->cryptoctx,
+                                  reqctx->cryptoctx, reqctx->idctx,
+                                  reqctx->opts->dh_size, &dh_params,
+                                  &dh_params_len, &dh_pubkey, &dh_pubkey_len);
+        if (retval != 0) {
             pkiDebug("failed to create dh parameters\n");
             goto cleanup;
         }
+        info.algorithm.parameters = make_data(dh_params, dh_params_len);
+        info.subjectPublicKey = make_data(dh_pubkey, dh_pubkey_len);
         break;
     case RSA_PROTOCOL:
         TRACE_PKINIT_CLIENT_REQ_RSA(context);
         pkiDebug("as_req: RSA key transport algorithm\n");
         switch((int)reqctx->pa_type) {
         case KRB5_PADATA_PK_AS_REQ_OLD:
-            auth_pack9->clientPublicValue = NULL;
+            auth_pack9.clientPublicValue = NULL;
             break;
         case KRB5_PADATA_PK_AS_REQ:
-            free_krb5_subject_pk_info(&info);
-            auth_pack->clientPublicValue = NULL;
+            auth_pack.clientPublicValue = NULL;
             break;
         }
         break;
@@ -338,10 +313,10 @@ pkinit_as_req_create(krb5_context context,
     /* Encode the authpack */
     switch((int)reqctx->pa_type) {
     case KRB5_PADATA_PK_AS_REQ:
-        retval = k5int_encode_krb5_auth_pack(auth_pack, &coded_auth_pack);
+        retval = k5int_encode_krb5_auth_pack(&auth_pack, &coded_auth_pack);
         break;
     case KRB5_PADATA_PK_AS_REQ_OLD:
-        retval = k5int_encode_krb5_auth_pack_draft9(auth_pack9,
+        retval = k5int_encode_krb5_auth_pack_draft9(&auth_pack9,
                                                     &coded_auth_pack);
         break;
     }
@@ -451,12 +426,11 @@ pkinit_as_req_create(krb5_context context,
 #endif
 
 cleanup:
-    if (auth_pack != NULL)
-        auth_pack->supportedKDFs = NULL; /*alias to global constant*/
-    free_krb5_auth_pack(&auth_pack);
+    free_krb5_algorithm_identifiers(&cmstypes);
+    free(dh_params);
+    free(dh_pubkey);
     free_krb5_pa_pk_as_req(&req);
     free_krb5_pa_pk_as_req_draft9(&req9);
-    free(auth_pack9);
 
     pkiDebug("pkinit_as_req_create retval=%d\n", (int) retval);
 
diff --git a/src/plugins/preauth/pkinit/pkinit_lib.c b/src/plugins/preauth/pkinit/pkinit_lib.c
index f1d818040..a7bf54e3d 100644
--- a/src/plugins/preauth/pkinit/pkinit_lib.c
+++ b/src/plugins/preauth/pkinit/pkinit_lib.c
@@ -302,27 +302,6 @@ init_krb5_reply_key_pack_draft9(krb5_reply_key_pack_draft9 **in)
 }
 
 void
-init_krb5_auth_pack(krb5_auth_pack **in)
-{
-    (*in) = malloc(sizeof(krb5_auth_pack));
-    if ((*in) == NULL) return;
-    (*in)->clientPublicValue = NULL;
-    (*in)->supportedCMSTypes = NULL;
-    (*in)->clientDHNonce.length = 0;
-    (*in)->clientDHNonce.data = NULL;
-    (*in)->pkAuthenticator.paChecksum.contents = NULL;
-    (*in)->supportedKDFs = NULL;
-}
-
-void
-init_krb5_auth_pack_draft9(krb5_auth_pack_draft9 **in)
-{
-    (*in) = malloc(sizeof(krb5_auth_pack_draft9));
-    if ((*in) == NULL) return;
-    (*in)->clientPublicValue = NULL;
-}
-
-void
 init_krb5_pa_pk_as_rep(krb5_pa_pk_as_rep **in)
 {
     (*in) = malloc(sizeof(krb5_pa_pk_as_rep));