[30288] in CVS-changelog-for-Kerberos-V5
krb5 commit: Fix memory bugs in gss_add_cred() extension case
daemon@ATHENA.MIT.EDU (Greg Hudson)
Wed Sep 19 12:20:30 2018
Date: Wed, 19 Sep 2018 12:20:22 -0400
From: Greg Hudson <ghudson@mit.edu>
Message-Id: <201809191620.w8JGKMa5019563@drugstore.mit.edu>
To: cvs-krb5@mit.edu
Reply-To: krbdev@mit.edu
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cvs-krb5-bounces@mit.edu
https://github.com/krb5/krb5/commit/288cbada833dc6af7d43dd308563b48b73347dfb
commit 288cbada833dc6af7d43dd308563b48b73347dfb
Author: Greg Hudson <ghudson@mit.edu>
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().
ticket: 8734
tags: pullup
target_version: 1.16-next
target_version: 1.15-next
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 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;
}
_______________________________________________
cvs-krb5 mailing list
cvs-krb5@mit.edu
https://mailman.mit.edu/mailman/listinfo/cvs-krb5
