krb5 commit [krb5-1.16]: Fix memory bugs in gss_add_cred() extension case

https://github.com/krb5/krb5/commit/71d28787a8f70186909141c8b6de2e7798ab5e41
commit 71d28787a8f70186909141c8b6de2e7798ab5e41
Author: Greg Hudson <[email protected]>
Date:   Thu Sep 13 16:31:36 2018 -0400

    Fix memory bugs in gss_add_cred() extension case
    
    If gss_add_cred() is called with both an input_cred_handle and an
    output_cred_handle, it creates a new credential with the elements of
    the input credential plus the requested element.  Making a shallow
    copy of mechs_array and cred_array from the old credential creates
    aliased pointers which become invalid when one of the two credentials
    is released, leading to use-after-free and double-free errors.
    
    Instead, make a full copy of the input cred for this case.  Make this
    copy at the beginning so that union_cred can always be modified in
    place (and freed on error using gss_release_cred() if we created it),
    removing the need for new_union_cred, new_mechs_array, and
    new_cred_array.  Use a stack object for target_mechs to simplify
    cleanup and reduce the number of failure cases.
    
    GSSAPI provides no facility for copying a credential; since we mostly
    use the GSSAPI as our SPI for mechanisms, we have no simple way to
    copy mechanism creds when copying the union cred.  Use
    gss_export_cred() and gss_import_cred() if the mechanism provides
    them; otherwise fall back to gss_inquire_cred() and
    gss_acquire_cred().
    
    (cherry picked from commit 288cbada833dc6af7d43dd308563b48b73347dfb)
    
    ticket: 8734
    version_fixed: 1.16.2

 src/lib/gssapi/mechglue/g_acquire_cred.c |  207 ++++++++++++++++++++----------
 src/tests/gssapi/t_add_cred.c            |   31 +++++-
 2 files changed, 167 insertions(+), 71 deletions(-)

diff --git a/src/lib/gssapi/mechglue/g_acquire_cred.c b/src/lib/gssapi/mechglue/g_acquire_cred.c
index e7b3ed6..f03ab9e 100644
--- a/src/lib/gssapi/mechglue/g_acquire_cred.c
+++ b/src/lib/gssapi/mechglue/g_acquire_cred.c
@@ -308,6 +308,92 @@ val_add_cred_args(
     return (GSS_S_COMPLETE);
 }
 
+/* Copy a mechanism credential (with the mechanism given by mech_oid) as
+ * faithfully as possible. */
+static OM_uint32
+copy_mech_cred(OM_uint32 *minor_status, gss_cred_id_t cred_in,
+	       gss_OID mech_oid, gss_cred_id_t *cred_out)
+{
+    OM_uint32 status, tmpmin;
+    gss_mechanism mech;
+    gss_buffer_desc buf;
+    gss_name_t name;
+    OM_uint32 life;
+    gss_cred_usage_t usage;
+    gss_OID_set_desc oidset;
+
+    mech = gssint_get_mechanism(mech_oid);
+    if (mech == NULL)
+	return (GSS_S_BAD_MECH);
+    if (mech->gss_export_cred != NULL && mech->gss_import_cred != NULL) {
+	status = mech->gss_export_cred(minor_status, cred_in, &buf);
+	if (status != GSS_S_COMPLETE)
+	    return (status);
+	status = mech->gss_import_cred(minor_status, &buf, cred_out);
+	(void) gss_release_buffer(&tmpmin, &buf);
+    } else if (mech->gss_inquire_cred != NULL &&
+	       mech->gss_acquire_cred != NULL) {
+	status = mech->gss_inquire_cred(minor_status, cred_in, &name, &life,
+					&usage, NULL);
+	if (status != GSS_S_COMPLETE)
+	    return (status);
+	oidset.count = 1;
+	oidset.elements = gssint_get_public_oid(mech_oid);
+	status = mech->gss_acquire_cred(minor_status, name, life, &oidset,
+					usage, cred_out, NULL, NULL);
+	gss_release_name(&tmpmin, &name);
+    } else {
+	status = GSS_S_UNAVAILABLE;
+    }
+    return (status);
+}
+
+/* Copy a union credential from cred_in to *cred_out. */
+static OM_uint32
+copy_union_cred(OM_uint32 *minor_status, gss_cred_id_t cred_in,
+		gss_union_cred_t *cred_out)
+{
+    OM_uint32 status, tmpmin;
+    gss_union_cred_t cred = (gss_union_cred_t)cred_in;
+    gss_union_cred_t ncred = NULL;
+    gss_cred_id_t tmpcred;
+    int i;
+
+    ncred = calloc(1, sizeof (*ncred));
+    if (ncred == NULL)
+	goto oom;
+    ncred->mechs_array = calloc(cred->count, sizeof (*ncred->mechs_array));
+    ncred->cred_array = calloc(cred->count, sizeof (*ncred->cred_array));
+    if (ncred->mechs_array == NULL || ncred->cred_array == NULL)
+	goto oom;
+    ncred->count = cred->count;
+
+    for (i = 0; i < cred->count; i++) {
+	/* Copy this element's mechanism OID. */
+	ncred->mechs_array[i].elements = malloc(cred->mechs_array[i].length);
+	if (ncred->mechs_array[i].elements == NULL)
+	    goto oom;
+	g_OID_copy(&ncred->mechs_array[i], &cred->mechs_array[i]);
+
+	/* Copy this element's mechanism cred. */
+	status = copy_mech_cred(minor_status, cred->cred_array[i],
+				&cred->mechs_array[i], &ncred->cred_array[i]);
+	if (status != GSS_S_COMPLETE)
+	    goto error;
+    }
+
+    ncred->loopback = ncred;
+    *cred_out = ncred;
+    return GSS_S_COMPLETE;
+
+oom:
+    status = GSS_S_FAILURE;
+    *minor_status = ENOMEM;
+error:
+    tmpcred = (gss_cred_id_t)ncred;
+    (void) gss_release_cred(&tmpmin, &tmpcred);
+    return status;
+}
 
 /* V2 KRB5_CALLCONV */
 OM_uint32 KRB5_CALLCONV
@@ -359,14 +445,13 @@ gss_add_cred_from(minor_status, input_cred_handle,
     OM_uint32		status, temp_minor_status;
     OM_uint32		time_req, time_rec = 0, *time_recp = NULL;
     gss_union_name_t	union_name;
-    gss_union_cred_t	new_union_cred, union_cred;
+    gss_union_cred_t	union_cred;
     gss_name_t		internal_name = GSS_C_NO_NAME;
     gss_name_t		allocated_name = GSS_C_NO_NAME;
     gss_mechanism	mech;
-    gss_cred_id_t	cred = NULL;
-    gss_OID		new_mechs_array = NULL;
-    gss_cred_id_t *	new_cred_array = NULL;
-    gss_OID_set		target_mechs = GSS_C_NO_OID_SET;
+    gss_cred_id_t	cred = NULL, tmpcred;
+    void		*newptr, *oidbuf = NULL;
+    gss_OID_set_desc	target_mechs;
     gss_OID		selected_mech = GSS_C_NO_OID;
 
     status = val_add_cred_args(minor_status,
@@ -396,16 +481,25 @@ gss_add_cred_from(minor_status, input_cred_handle,
 	return (GSS_S_UNAVAILABLE);
 
     if (input_cred_handle == GSS_C_NO_CREDENTIAL) {
+	/* Create a new credential handle. */
 	union_cred = malloc(sizeof (gss_union_cred_desc));
 	if (union_cred == NULL)
 	    return (GSS_S_FAILURE);
 
 	(void) memset(union_cred, 0, sizeof (gss_union_cred_desc));
-    } else {
+	union_cred->loopback = union_cred;
+    } else if (output_cred_handle == NULL) {
+	/* Add to the existing handle. */
 	union_cred = (gss_union_cred_t)input_cred_handle;
 	if (gssint_get_mechanism_cred(union_cred, selected_mech) !=
 	    GSS_C_NO_CREDENTIAL)
 	    return (GSS_S_DUPLICATE_ELEMENT);
+    } else {
+	/* Create a new credential handle with the mechanism credentials of the
+	 * input handle plus the acquired mechanism credential. */
+	status = copy_union_cred(minor_status, input_cred_handle, &union_cred);
+	if (status != GSS_S_COMPLETE)
+	    return (status);
     }
 
     /* We may need to create a mechanism specific name. */
@@ -436,30 +530,28 @@ gss_add_cred_from(minor_status, input_cred_handle,
     else
 	time_req = 0;
 
-    status = gss_create_empty_oid_set(minor_status, &target_mechs);
-    if (status != GSS_S_COMPLETE)
-	goto errout;
-
-    status = gss_add_oid_set_member(minor_status,
-				    gssint_get_public_oid(selected_mech),
-				    &target_mechs);
-    if (status != GSS_S_COMPLETE)
+    target_mechs.count = 1;
+    target_mechs.elements = gssint_get_public_oid(selected_mech);
+    if (target_mechs.elements == NULL) {
+	status = GSS_S_FAILURE;
 	goto errout;
+    }
 
     if (initiator_time_rec != NULL || acceptor_time_rec != NULL)
 	time_recp = &time_rec;
 
     if (mech->gss_acquire_cred_from) {
 	status = mech->gss_acquire_cred_from(minor_status, internal_name,
-					     time_req, target_mechs,
+					     time_req, &target_mechs,
 					     cred_usage, cred_store, &cred,
 					     NULL, time_recp);
     } else if (cred_store == GSS_C_NO_CRED_STORE) {
 	status = mech->gss_acquire_cred(minor_status, internal_name, time_req,
-					target_mechs, cred_usage, &cred, NULL,
+					&target_mechs, cred_usage, &cred, NULL,
 					time_recp);
     } else {
-	return GSS_S_UNAVAILABLE;
+	status = GSS_S_UNAVAILABLE;
+	goto errout;
     }
 
     if (status != GSS_S_COMPLETE) {
@@ -467,17 +559,23 @@ gss_add_cred_from(minor_status, input_cred_handle,
 	goto errout;
     }
 
-    /* now add the new credential elements */
-    new_mechs_array = (gss_OID)
-	malloc(sizeof (gss_OID_desc) * (union_cred->count+1));
+    /* Extend the arrays in the union cred. */
 
-    new_cred_array = (gss_cred_id_t *)
-	malloc(sizeof (gss_cred_id_t) * (union_cred->count+1));
+    newptr = realloc(union_cred->mechs_array,
+		     (union_cred->count + 1) * sizeof (gss_OID_desc));
+    if (newptr == NULL) {
+	status = GSS_S_FAILURE;
+	goto errout;
+    }
+    union_cred->mechs_array = newptr;
 
-    if (!new_mechs_array || !new_cred_array) {
+    newptr = realloc(union_cred->cred_array,
+		     (union_cred->count + 1) * sizeof (gss_cred_id_t));
+    if (newptr == NULL) {
 	status = GSS_S_FAILURE;
 	goto errout;
     }
+    union_cred->cred_array = newptr;
 
     if (acceptor_time_rec)
 	if (cred_usage == GSS_C_ACCEPT || cred_usage == GSS_C_BOTH)
@@ -486,52 +584,25 @@ gss_add_cred_from(minor_status, input_cred_handle,
 	if (cred_usage == GSS_C_INITIATE || cred_usage == GSS_C_BOTH)
 	    *initiator_time_rec = time_rec;
 
-    /*
-     * OK, expand the mechanism array and the credential array
-     */
-    (void) memcpy(new_mechs_array, union_cred->mechs_array,
-		  sizeof (gss_OID_desc) * union_cred->count);
-    (void) memcpy(new_cred_array, union_cred->cred_array,
-		  sizeof (gss_cred_id_t) * union_cred->count);
-
-    new_cred_array[union_cred->count] = cred;
-    if ((new_mechs_array[union_cred->count].elements =
-	 malloc(selected_mech->length)) == NULL)
+    oidbuf = malloc(selected_mech->length);
+    if (oidbuf == NULL)
 	goto errout;
-
-    g_OID_copy(&new_mechs_array[union_cred->count], selected_mech);
+    union_cred->mechs_array[union_cred->count].elements = oidbuf;
+    g_OID_copy(&union_cred->mechs_array[union_cred->count], selected_mech);
 
     if (actual_mechs != NULL) {
-	status = gssint_make_public_oid_set(minor_status, new_mechs_array,
+	status = gssint_make_public_oid_set(minor_status,
+					    union_cred->mechs_array,
 					    union_cred->count + 1,
 					    actual_mechs);
-	if (GSS_ERROR(status)) {
-	    free(new_mechs_array[union_cred->count].elements);
+	if (GSS_ERROR(status))
 	    goto errout;
-	}
     }
 
-    if (output_cred_handle == NULL) {
-	free(union_cred->mechs_array);
-	free(union_cred->cred_array);
-	new_union_cred = union_cred;
-    } else if (input_cred_handle == GSS_C_NO_CREDENTIAL) {
-	new_union_cred = union_cred;
-	*output_cred_handle = (gss_cred_id_t)new_union_cred;
-    } else {
-	new_union_cred = malloc(sizeof (gss_union_cred_desc));
-	if (new_union_cred == NULL) {
-	    free(new_mechs_array[union_cred->count].elements);
-	    goto errout;
-	}
-	*new_union_cred = *union_cred;
-	*output_cred_handle = (gss_cred_id_t)new_union_cred;
-    }
-
-    new_union_cred->mechs_array = new_mechs_array;
-    new_union_cred->cred_array = new_cred_array;
-    new_union_cred->count++;
-    new_union_cred->loopback = new_union_cred;
+    union_cred->cred_array[union_cred->count] = cred;
+    union_cred->count++;
+    if (output_cred_handle != NULL)
+	*output_cred_handle = (gss_cred_id_t)union_cred;
 
     /* We're done with the internal name. Free it if we allocated it. */
 
@@ -539,16 +610,10 @@ gss_add_cred_from(minor_status, input_cred_handle,
 	(void) gssint_release_internal_name(&temp_minor_status,
 					   selected_mech,
 					   &allocated_name);
-    (void) generic_gss_release_oid_set(&temp_minor_status, &target_mechs);
 
     return (GSS_S_COMPLETE);
 
 errout:
-    if (new_mechs_array)
-	free(new_mechs_array);
-    if (new_cred_array)
-	free(new_cred_array);
-
     if (cred != NULL && mech->gss_release_cred)
 	mech->gss_release_cred(&temp_minor_status, &cred);
 
@@ -557,10 +622,12 @@ errout:
 					   selected_mech,
 					   &allocated_name);
 
-    if (input_cred_handle == GSS_C_NO_CREDENTIAL && union_cred)
-	free(union_cred);
+    if (output_cred_handle != NULL && union_cred != NULL) {
+	tmpcred = union_cred;
+	(void) gss_release_cred(&temp_minor_status, &tmpcred);
+    }
 
-    (void) generic_gss_release_oid_set(&temp_minor_status, &target_mechs);
+    free(oidbuf);
 
     return (status);
 }
diff --git a/src/tests/gssapi/t_add_cred.c b/src/tests/gssapi/t_add_cred.c
index 1407e67..b1142b6 100644
--- a/src/tests/gssapi/t_add_cred.c
+++ b/src/tests/gssapi/t_add_cred.c
@@ -46,7 +46,7 @@ int
 main()
 {
     OM_uint32 minor, major;
-    gss_cred_id_t cred1;
+    gss_cred_id_t cred1, cred2;
     gss_cred_usage_t usage;
     gss_name_t name;
 
@@ -102,7 +102,36 @@ main()
                                      NULL, &usage);
     assert(major == GSS_S_COMPLETE && usage == GSS_C_ACCEPT);
 
+    /* Start over with another new cred. */
     gss_release_cred(&minor, &cred1);
+    major = gss_add_cred(&minor, GSS_C_NO_CREDENTIAL, GSS_C_NO_NAME,
+                         &mech_krb5, GSS_C_ACCEPT, GSS_C_INDEFINITE,
+                         GSS_C_INDEFINITE, &cred1, NULL, NULL, NULL);
+    assert(major == GSS_S_COMPLETE);
+
+    /* Create an expanded cred by passing both an output handle and an input
+     * handle. */
+    major = gss_add_cred(&minor, cred1, GSS_C_NO_NAME, &mech_iakerb,
+                         GSS_C_INITIATE, GSS_C_INDEFINITE, GSS_C_INDEFINITE,
+                         &cred2, NULL, NULL, NULL);
+    assert(major == GSS_S_COMPLETE);
+
+    /* Verify mechanism creds in cred1 and cred2. */
+    major = gss_inquire_cred_by_mech(&minor, cred1, &mech_krb5, NULL, NULL,
+                                     NULL, &usage);
+    assert(major == GSS_S_COMPLETE && usage == GSS_C_ACCEPT);
+    major = gss_inquire_cred_by_mech(&minor, cred1, &mech_iakerb, NULL, NULL,
+                                     NULL, &usage);
+    assert(major == GSS_S_NO_CRED);
+    major = gss_inquire_cred_by_mech(&minor, cred2, &mech_krb5, NULL, NULL,
+                                     NULL, &usage);
+    assert(major == GSS_S_COMPLETE && usage == GSS_C_ACCEPT);
+    major = gss_inquire_cred_by_mech(&minor, cred2, &mech_iakerb, NULL, NULL,
+                                     NULL, &usage);
+    assert(major == GSS_S_COMPLETE && usage == GSS_C_INITIATE);
+
+    gss_release_cred(&minor, &cred1);
+    gss_release_cred(&minor, &cred2);
 
     return 0;
 }