diff options
author | Greg Hudson <ghudson@mit.edu> | 2018-09-13 16:31:36 -0400 |
---|---|---|
committer | Greg Hudson <ghudson@mit.edu> | 2018-09-19 12:00:29 -0400 |
commit | 288cbada833dc6af7d43dd308563b48b73347dfb (patch) | |
tree | 02bb4d68b5dfde3f794b6902dcf20827a44e9194 | |
parent | 9e32161dc307a323fd36fd59e252583fe7b90526 (diff) | |
download | krb5-288cbada833dc6af7d43dd308563b48b73347dfb.zip krb5-288cbada833dc6af7d43dd308563b48b73347dfb.tar.gz krb5-288cbada833dc6af7d43dd308563b48b73347dfb.tar.bz2 |
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
-rw-r--r-- | src/lib/gssapi/mechglue/g_acquire_cred.c | 207 | ||||
-rw-r--r-- | 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 5e82495..cf452fc 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); } /* 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 d59fde9..7ae4dd8 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; /* 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; } |