krb5/Refactor-KDC-krb5_pa_data-utility-functions.patch

From 7c59b7ee063489a4259c34b725728fee7e411c46 Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
Date: Thu, 21 Dec 2017 11:28:52 -0500
Subject: [PATCH] Refactor KDC krb5_pa_data utility functions

Move alloc_padata from fast_util.c to kdc_util.c and make it
non-static so it can be used by other files.  Rename it to
alloc_pa_data for consistency with add_pa_data_element.  Make it
correctly handle zero length using a null contents pointer.

Make add_pa_data_element claim both the container and contents memory
from the caller, now that callers can use alloc_pa_data to simplify
allocation and copying.  Remove the copy parameter and the unused
context parameter, and put the list parameter first.  Adjust all
callers accordingly, making small simplifications to memory handling
where applicable.

(cherry picked from commit 4af478c18b02e1d2444a328bb79e6976ef3d312b)
---
 src/kdc/fast_util.c   |  28 +------
 src/kdc/kdc_preauth.c |  14 ++--
 src/kdc/kdc_util.c    | 187 +++++++++++++++++++++---------------------
 src/kdc/kdc_util.h    |   8 +-
 4 files changed, 109 insertions(+), 128 deletions(-)

diff --git a/src/kdc/fast_util.c b/src/kdc/fast_util.c
index e05107ef3..6a3fc11b9 100644
--- a/src/kdc/fast_util.c
+++ b/src/kdc/fast_util.c
@@ -451,36 +451,12 @@ kdc_fast_hide_client(struct kdc_request_state *state)
     return (state->fast_options & KRB5_FAST_OPTION_HIDE_CLIENT_NAMES) != 0;
 }
 
-/* Allocate a pa-data entry with an uninitialized buffer of size len. */
-static krb5_error_code
-alloc_padata(krb5_preauthtype pa_type, size_t len, krb5_pa_data **out)
-{
-    krb5_pa_data *pa;
-    uint8_t *buf;
-
-    *out = NULL;
-    buf = malloc(len);
-    if (buf == NULL)
-        return ENOMEM;
-    pa = malloc(sizeof(*pa));
-    if (pa == NULL) {
-        free(buf);
-        return ENOMEM;
-    }
-    pa->magic = KV5M_PA_DATA;
-    pa->pa_type = pa_type;
-    pa->length = len;
-    pa->contents = buf;
-    *out = pa;
-    return 0;
-}
-
 /* Create a pa-data entry with the specified type and contents. */
 static krb5_error_code
 make_padata(krb5_preauthtype pa_type, const void *contents, size_t len,
             krb5_pa_data **out)
 {
-    if (alloc_padata(pa_type, len, out) != 0)
+    if (alloc_pa_data(pa_type, len, out) != 0)
         return ENOMEM;
     memcpy((*out)->contents, contents, len);
     return 0;
@@ -720,7 +696,7 @@ kdc_fast_make_cookie(krb5_context context, struct kdc_request_state *state,
         goto cleanup;
 
     /* Construct the cookie pa-data entry. */
-    ret = alloc_padata(KRB5_PADATA_FX_COOKIE, 8 + enc.ciphertext.length, &pa);
+    ret = alloc_pa_data(KRB5_PADATA_FX_COOKIE, 8 + enc.ciphertext.length, &pa);
     memcpy(pa->contents, "MIT1", 4);
     store_32_be(kvno, pa->contents + 4);
     memcpy(pa->contents + 8, enc.ciphertext.data, enc.ciphertext.length);
diff --git a/src/kdc/kdc_preauth.c b/src/kdc/kdc_preauth.c
index 739c5e776..edc30bd83 100644
--- a/src/kdc/kdc_preauth.c
+++ b/src/kdc/kdc_preauth.c
@@ -1617,18 +1617,20 @@ return_referral_enc_padata( krb5_context context,
 {
     krb5_error_code             code;
     krb5_tl_data                tl_data;
-    krb5_pa_data                pa_data;
+    krb5_pa_data                *pa;
 
     tl_data.tl_data_type = KRB5_TL_SVR_REFERRAL_DATA;
     code = krb5_dbe_lookup_tl_data(context, server, &tl_data);
     if (code || tl_data.tl_data_length == 0)
         return 0;
 
-    pa_data.magic = KV5M_PA_DATA;
-    pa_data.pa_type = KRB5_PADATA_SVR_REFERRAL_INFO;
-    pa_data.length = tl_data.tl_data_length;
-    pa_data.contents = tl_data.tl_data_contents;
-    return add_pa_data_element(context, &pa_data, &reply->enc_padata, TRUE);
+    code = alloc_pa_data(KRB5_PADATA_SVR_REFERRAL_INFO, tl_data.tl_data_length,
+                         &pa);
+    if (code)
+        return code;
+    memcpy(pa->contents, tl_data.tl_data_contents, tl_data.tl_data_length);
+    /* add_pa_data_element() claims pa on success or failure. */
+    return add_pa_data_element(&reply->enc_padata, pa);
 }
 
 krb5_error_code
diff --git a/src/kdc/kdc_util.c b/src/kdc/kdc_util.c
index 754570c01..13111215d 100644
--- a/src/kdc/kdc_util.c
+++ b/src/kdc/kdc_util.c
@@ -1353,9 +1353,9 @@ kdc_make_s4u2self_rep(krb5_context context,
                       krb5_enc_kdc_rep_part *reply_encpart)
 {
     krb5_error_code             code;
-    krb5_data                   *data = NULL;
+    krb5_data                   *der_user_id = NULL, *der_s4u_x509_user = NULL;
     krb5_pa_s4u_x509_user       rep_s4u_user;
-    krb5_pa_data                padata;
+    krb5_pa_data                *pa;
     krb5_enctype                enctype;
     krb5_keyusage               usage;
 
@@ -1366,7 +1366,7 @@ kdc_make_s4u2self_rep(krb5_context context,
     rep_s4u_user.user_id.options =
         req_s4u_user->user_id.options & KRB5_S4U_OPTS_USE_REPLY_KEY_USAGE;
 
-    code = encode_krb5_s4u_userid(&rep_s4u_user.user_id, &data);
+    code = encode_krb5_s4u_userid(&rep_s4u_user.user_id, &der_user_id);
     if (code != 0)
         goto cleanup;
 
@@ -1377,29 +1377,25 @@ kdc_make_s4u2self_rep(krb5_context context,
 
     code = krb5_c_make_checksum(context, req_s4u_user->cksum.checksum_type,
                                 tgs_subkey != NULL ? tgs_subkey : tgs_session,
-                                usage, data,
-                                &rep_s4u_user.cksum);
+                                usage, der_user_id, &rep_s4u_user.cksum);
     if (code != 0)
         goto cleanup;
 
-    krb5_free_data(context, data);
-    data = NULL;
-
-    code = encode_krb5_pa_s4u_x509_user(&rep_s4u_user, &data);
+    code = encode_krb5_pa_s4u_x509_user(&rep_s4u_user, &der_s4u_x509_user);
     if (code != 0)
         goto cleanup;
 
-    padata.magic = KV5M_PA_DATA;
-    padata.pa_type = KRB5_PADATA_S4U_X509_USER;
-    padata.length = data->length;
-    padata.contents = (krb5_octet *)data->data;
-
-    code = add_pa_data_element(context, &padata, &reply->padata, FALSE);
+    /* Add a padata element, stealing memory from der_s4u_x509_user. */
+    code = alloc_pa_data(KRB5_PADATA_S4U_X509_USER, 0, &pa);
+    if (code != 0)
+        goto cleanup;
+    pa->length = der_s4u_x509_user->length;
+    pa->contents = (uint8_t *)der_s4u_x509_user->data;
+    der_s4u_x509_user->data = NULL;
+    /* add_pa_data_element() claims pa on success or failure. */
+    code = add_pa_data_element(&reply->padata, pa);
     if (code != 0)
         goto cleanup;
-
-    free(data);
-    data = NULL;
 
     if (tgs_subkey != NULL)
         enctype = tgs_subkey->enctype;
@@ -1413,33 +1409,27 @@ kdc_make_s4u2self_rep(krb5_context context,
      */
     if ((req_s4u_user->user_id.options & KRB5_S4U_OPTS_USE_REPLY_KEY_USAGE) &&
         enctype_requires_etype_info_2(enctype) == FALSE) {
-        padata.length = req_s4u_user->cksum.length +
-            rep_s4u_user.cksum.length;
-        padata.contents = malloc(padata.length);
-        if (padata.contents == NULL) {
-            code = ENOMEM;
+        code = alloc_pa_data(KRB5_PADATA_S4U_X509_USER,
+                             req_s4u_user->cksum.length +
+                             rep_s4u_user.cksum.length, &pa);
+        if (code != 0)
             goto cleanup;
-        }
+        memcpy(pa->contents,
+               req_s4u_user->cksum.contents, req_s4u_user->cksum.length);
+        memcpy(&pa->contents[req_s4u_user->cksum.length],
+               rep_s4u_user.cksum.contents, rep_s4u_user.cksum.length);
 
-        memcpy(padata.contents,
-               req_s4u_user->cksum.contents,
-               req_s4u_user->cksum.length);
-        memcpy(&padata.contents[req_s4u_user->cksum.length],
-               rep_s4u_user.cksum.contents,
-               rep_s4u_user.cksum.length);
-
-        code = add_pa_data_element(context,&padata,
-                                   &reply_encpart->enc_padata, FALSE);
-        if (code != 0) {
-            free(padata.contents);
+        /* add_pa_data_element() claims pa on success or failure. */
+        code = add_pa_data_element(&reply_encpart->enc_padata, pa);
+        if (code != 0)
             goto cleanup;
-        }
     }
 
 cleanup:
     if (rep_s4u_user.cksum.contents != NULL)
         krb5_free_checksum_contents(context, &rep_s4u_user.cksum);
-    krb5_free_data(context, data);
+    krb5_free_data(context, der_user_id);
+    krb5_free_data(context, der_s4u_x509_user);
 
     return code;
 }
@@ -1707,46 +1697,50 @@ enctype_requires_etype_info_2(krb5_enctype enctype)
     }
 }
 
-/* XXX where are the generic helper routines for this? */
+/* Allocate a pa-data entry with an uninitialized buffer of size len. */
 krb5_error_code
-add_pa_data_element(krb5_context context,
-                    krb5_pa_data *padata,
-                    krb5_pa_data ***inout_padata,
-                    krb5_boolean copy)
+alloc_pa_data(krb5_preauthtype pa_type, size_t len, krb5_pa_data **out)
 {
-    int                         i;
-    krb5_pa_data                **p;
+    krb5_pa_data *pa;
+    uint8_t *buf = NULL;
 
-    if (*inout_padata != NULL) {
-        for (i = 0; (*inout_padata)[i] != NULL; i++)
-            ;
-    } else
-        i = 0;
-
-    p = realloc(*inout_padata, (i + 2) * sizeof(krb5_pa_data *));
-    if (p == NULL)
-        return ENOMEM;
-
-    *inout_padata = p;
-
-    p[i] = (krb5_pa_data *)malloc(sizeof(krb5_pa_data));
-    if (p[i] == NULL)
-        return ENOMEM;
-    *(p[i]) = *padata;
-
-    p[i + 1] = NULL;
-
-    if (copy) {
-        p[i]->contents = (krb5_octet *)malloc(padata->length);
-        if (p[i]->contents == NULL) {
-            free(p[i]);
-            p[i] = NULL;
+    *out = NULL;
+    if (len > 0) {
+        buf = malloc(len);
+        if (buf == NULL)
             return ENOMEM;
-        }
-
-        memcpy(p[i]->contents, padata->contents, padata->length);
     }
+    pa = malloc(sizeof(*pa));
+    if (pa == NULL) {
+        free(buf);
+        return ENOMEM;
+    }
+    pa->magic = KV5M_PA_DATA;
+    pa->pa_type = pa_type;
+    pa->length = len;
+    pa->contents = buf;
+    *out = pa;
+    return 0;
+}
 
+/* Add pa to list, claiming its memory.  Free pa on failure. */
+krb5_error_code
+add_pa_data_element(krb5_pa_data ***list, krb5_pa_data *pa)
+{
+    size_t count;
+    krb5_pa_data **newlist;
+
+    for (count = 0; *list != NULL && (*list)[count] != NULL; count++);
+
+    newlist = realloc(*list, (count + 2) * sizeof(*newlist));
+    if (newlist == NULL) {
+        free(pa->contents);
+        free(pa);
+        return ENOMEM;
+    }
+    newlist[count] = pa;
+    newlist[count + 1] = NULL;
+    *list = newlist;
     return 0;
 }
 
@@ -1850,38 +1844,47 @@ kdc_handle_protected_negotiation(krb5_context context,
 {
     krb5_error_code retval = 0;
     krb5_checksum checksum;
-    krb5_data *out = NULL;
-    krb5_pa_data pa, *pa_in;
+    krb5_data *der_cksum = NULL;
+    krb5_pa_data *pa, *pa_in;
+
+    memset(&checksum, 0, sizeof(checksum));
+
     pa_in = krb5int_find_pa_data(context, request->padata,
                                  KRB5_ENCPADATA_REQ_ENC_PA_REP);
     if (pa_in == NULL)
         return 0;
-    pa.magic = KV5M_PA_DATA;
-    pa.pa_type = KRB5_ENCPADATA_REQ_ENC_PA_REP;
-    memset(&checksum, 0, sizeof(checksum));
-    retval = krb5_c_make_checksum(context,0, reply_key,
-                                  KRB5_KEYUSAGE_AS_REQ, req_pkt, &checksum);
+
+    /* Compute and encode a checksum over the AS-REQ. */
+    retval = krb5_c_make_checksum(context, 0, reply_key, KRB5_KEYUSAGE_AS_REQ,
+                                  req_pkt, &checksum);
     if (retval != 0)
         goto cleanup;
-    retval = encode_krb5_checksum(&checksum, &out);
+    retval = encode_krb5_checksum(&checksum, &der_cksum);
     if (retval != 0)
         goto cleanup;
-    pa.contents = (krb5_octet *) out->data;
-    pa.length = out->length;
-    retval = add_pa_data_element(context, &pa, out_enc_padata, FALSE);
+
+    /* Add a pa-data element to the list, stealing memory from der_cksum. */
+    retval = alloc_pa_data(KRB5_ENCPADATA_REQ_ENC_PA_REP, 0, &pa);
     if (retval)
         goto cleanup;
-    out->data = NULL;
-    pa.magic = KV5M_PA_DATA;
-    pa.pa_type = KRB5_PADATA_FX_FAST;
-    pa.length = 0;
-    pa.contents = NULL;
-    retval = add_pa_data_element(context, &pa, out_enc_padata, FALSE);
+    pa->length = der_cksum->length;
+    pa->contents = (uint8_t *)der_cksum->data;
+    der_cksum->data = NULL;
+    /* add_pa_data_element() claims pa on success or failure. */
+    retval = add_pa_data_element(out_enc_padata, pa);
+    if (retval)
+        goto cleanup;
+
+    /* Add a zero-length PA-FX-FAST element to the list. */
+    retval = alloc_pa_data(KRB5_PADATA_FX_FAST, 0, &pa);
+    if (retval)
+        goto cleanup;
+    /* add_pa_data_element() claims pa on success or failure. */
+    retval = add_pa_data_element(out_enc_padata, pa);
+
 cleanup:
-    if (checksum.contents)
-        krb5_free_checksum_contents(context, &checksum);
-    if (out != NULL)
-        krb5_free_data(context, out);
+    krb5_free_checksum_contents(context, &checksum);
+    krb5_free_data(context, der_cksum);
     return retval;
 }
 
diff --git a/src/kdc/kdc_util.h b/src/kdc/kdc_util.h
index c57d48f73..198eab9c4 100644
--- a/src/kdc/kdc_util.h
+++ b/src/kdc/kdc_util.h
@@ -202,10 +202,10 @@ void
 free_padata_context(krb5_context context, void *padata_context);
 
 krb5_error_code
-add_pa_data_element (krb5_context context,
-                     krb5_pa_data *padata,
-                     krb5_pa_data ***out_padata,
-                     krb5_boolean copy);
+alloc_pa_data(krb5_preauthtype pa_type, size_t len, krb5_pa_data **out);
+
+krb5_error_code
+add_pa_data_element(krb5_pa_data ***list, krb5_pa_data *pa);
 
 /* kdc_preauth_ec.c */
 krb5_error_code