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().

ticket: 8734
tags: pullup
target_version: 1.16-next
target_version: 1.15-next

diff --git a/src/lib/gssapi/mechglue/g_acquire_cred.c b/src/lib/gssapi/mechglue/g_acquire_cred.c
index 5e82495a2774143417b67e2264cd8b6f7bccc0e6..cf452fc2f37d9e1282d57c345c967dc124613cb0 100644 (file)
--- 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);
     }
 
     /* for default credentials we will use GSS_C_NO_NAME */
@@ -438,30 +532,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) {
@@ -469,17 +561,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)
@@ -488,52 +586,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. */
 
@@ -541,16 +612,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);
 
@@ -559,10 +624,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 d59fde9679fa2622acd7242fd19e5042e429d7ef..7ae4dd8909068ac43d8fbdcbc3309e3680fef970 100644 (file)
--- 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;
 
     /* Check that we get the expected error if we pass neither an input nor an
@@ -92,7 +92,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;
 }