On (21/03/15 14:41), Simo Sorce wrote: >On Thu, 2015-02-26 at 17:41 -0500, Simo Sorce wrote: >> See extended comment in the patch. >> >> Fixes issue #137 > >Fixed typo in comment. > >Bump up for review. >Simo. > >-- >Simo Sorce * Red Hat, Inc * New York >From 5d5d6cb98a42a09d02b8427bfc70ba1e9505eb50 Mon Sep 17 00:00:00 2001 >From: Simo Sorce <simo(a)redhat.com&gt; >Date: Thu, 26 Feb 2015 15:49:59 -0500 >Subject: [PATCH] Properly handle security contexts on error > >On error we need to make sure we do not return a pointer to a >security context that may have been already freed. >So make sure to always unconditionally return the context that we've >been returned by our callees. >Also reorganize the code so we do not accidently wipe the context >and leak memoy on error. > >This fixed a double-free bug found by NFS folks @ Red Hat > >Signed-off-by: Simo Sorce <simo(a)redhat.com&gt; >--- > proxy/src/client/gpm_accept_sec_context.c | 14 ++++--- > proxy/src/client/gpm_init_sec_context.c | 13 ++++--- > proxy/src/mechglue/gpp_accept_sec_context.c | 12 +++++- > proxy/src/mechglue/gpp_context.c | 5 +++ > proxy/src/mechglue/gpp_init_sec_context.c | 59 ++++++++++++++++------------- > 5 files changed, 63 insertions(+), 40 deletions(-) > >diff --git a/proxy/src/client/gpm_init_sec_context.c b/proxy/src/client/gpm_init_sec_context.c >index bd88055352a1c1194caae347d332a35f27277f78..14c65e405e87213ed3906c308e0a1c42af977f68 100644 >--- a/proxy/src/client/gpm_init_sec_context.c >+++ b/proxy/src/client/gpm_init_sec_context.c >@@ -137,12 +137,6 @@ done: > gpm_free_xdrs(GSSX_INIT_SEC_CONTEXT, &uarg, &ures); > > if (ret_maj == GSS_S_COMPLETE || ret_maj == GSS_S_CONTINUE_NEEDED) { >- /* replace old ctx handle if any */ >- if (*context_handle) { >- xdr_free((xdrproc_t)xdr_gssx_ctx, (char *)*context_handle); >- free(*context_handle); >- } >- *context_handle = ctx; > if (actual_mech_type) { > *actual_mech_type = mech; > } >@@ -171,6 +165,13 @@ done: > } > } > >+ /* always replace old ctx handle and set new */ >+ if (*context_handle) { >+ xdr_free((xdrproc_t)xdr_gssx_ctx, (char *)*context_handle); >+ free(*context_handle); >+ } >+ *context_handle = ctx; >+ I ran some static analysis and this patch introduced warnings. In case of error ctx is released and here is it assigned to output argument context_handle. I know it's not a problem because ret_maj will indicate an error. If you want we can ignore this warning or there is a possibility to change code to silent warnings. Error: CPPCHECK_WARNING (CWE-416): [#def2] gssproxy-0.4.0/src/client/gpm_init_sec_context.c:173: error[deallocuse]: Dereferencing 'ctx' after it is deallocated / released Error: USE_AFTER_FREE (CWE-825): [#def3] gssproxy-0.4.0/src/client/gpm_init_sec_context.c:108: alias: Assigning: "ctx" = "res->context_handle". Now both point to the same storage. gssproxy-0.4.0/src/client/gpm_init_sec_context.c:156: freed_arg: "free" frees "ctx". gssproxy-0.4.0/src/client/gpm_init_sec_context.c:173: use_after_free: Using freed pointer "ctx".

On (21/03/15 14:41), Simo Sorce wrote: >On Thu, 2015-02-26 at 17:41 -0500, Simo Sorce wrote: >> See extended comment in the patch. >> >> Fixes issue #137 > >Fixed typo in comment. > >Bump up for review. >Simo. > >-- >Simo Sorce * Red Hat, Inc * New York >From 5d5d6cb98a42a09d02b8427bfc70ba1e9505eb50 Mon Sep 17 00:00:00 2001 >From: Simo Sorce <simo(a)redhat.com&gt; >Date: Thu, 26 Feb 2015 15:49:59 -0500 >Subject: [PATCH] Properly handle security contexts on error > >On error we need to make sure we do not return a pointer to a >security context that may have been already freed. >So make sure to always unconditionally return the context that we've >been returned by our callees. >Also reorganize the code so we do not accidently wipe the context >and leak memoy on error. > >This fixed a double-free bug found by NFS folks @ Red Hat > >Signed-off-by: Simo Sorce <simo(a)redhat.com&gt; >--- > proxy/src/client/gpm_accept_sec_context.c | 14 ++++--- > proxy/src/client/gpm_init_sec_context.c | 13 ++++--- > proxy/src/mechglue/gpp_accept_sec_context.c | 12 +++++- > proxy/src/mechglue/gpp_context.c | 5 +++ > proxy/src/mechglue/gpp_init_sec_context.c | 59 ++++++++++++++++------------- > 5 files changed, 63 insertions(+), 40 deletions(-) > >diff --git a/proxy/src/mechglue/gpp_init_sec_context.c b/proxy/src/mechglue/gpp_init_sec_context.c >index e70e8fc7179d41b1fadf43cbaf8fe060f95bf64d..c80937c6f2355900f3da3a69c9c30f81a62518d0 100644 >--- a/proxy/src/mechglue/gpp_init_sec_context.c >+++ b/proxy/src/mechglue/gpp_init_sec_context.c >@@ -91,36 +91,19 @@ OM_uint32 gssi_init_sec_context(OM_uint32 *minor_status, > > GSSI_TRACE(); > >+ *minor_status = 0; >+ > if (target_name == GSS_C_NO_NAME) { > return GSS_S_CALL_INACCESSIBLE_READ; > } > >+ if (mech_type == GSS_C_NO_OID || gpp_is_special_oid(mech_type)) { >+ return GSS_S_BAD_MECH; >+ } >+ > tmaj = GSS_S_COMPLETE; > tmin = 0; > >- if (mech_type == GSS_C_NO_OID || gpp_is_special_oid(mech_type)) { >- maj = GSS_S_BAD_MECH; >- min = 0; >- goto done; >- } >- >- if (claimant_cred_handle != GSS_C_NO_CREDENTIAL) { >- cred_handle = (struct gpp_cred_handle *)claimant_cred_handle; >- if (cred_handle->local) { >- /* ok this means a previous call decided to short circuit to the >- * local mech, so let's just re-enter the mechglue here, as we >- * have no way to export creds yet. */ >- behavior = GPP_LOCAL_ONLY; >- } >- } else { >- cred_handle = calloc(1, sizeof(struct gpp_cred_handle)); >- if (!cred_handle) { >- maj = GSS_S_FAILURE; >- min = ENOMEM; >- goto done; >- } >- } >- And yet another warning caused by this patch. The code was moved and ther is a deadcode. Error: DEADCODE (CWE-561): [#def4] gssproxy-0.4.0/src/mechglue/gpp_init_sec_context.c:85: assignment: Assigning: "behavior" = "GPP_UNINITIALIZED". gssproxy-0.4.0/src/mechglue/gpp_init_sec_context.c:113: const: At condition "behavior == GPP_LOCAL_ONLY", the value of "behavior" must be equal to 0. gssproxy-0.4.0/src/mechglue/gpp_init_sec_context.c:113: dead_error_condition: The condition "behavior == GPP_LOCAL_ONLY" cannot be true. gssproxy-0.4.0/src/mechglue/gpp_init_sec_context.c:114: dead_error_begin: Execution cannot reach this statement: "maj = 655360U;". > if (*context_handle) { > ctx_handle = (struct gpp_context_handle *)*context_handle; > if (ctx_handle->local) { >@@ -141,6 +124,23 @@ OM_uint32 gssi_init_sec_context(OM_uint32 *minor_status, > } > } > >+ if (claimant_cred_handle != GSS_C_NO_CREDENTIAL) { >+ cred_handle = (struct gpp_cred_handle *)claimant_cred_handle; >+ if (cred_handle->local) { >+ /* ok this means a previous call decided to short circuit to the >+ * local mech, so let's just re-enter the mechglue here, as we >+ * have no way to export creds yet. */ >+ behavior = GPP_LOCAL_ONLY; >+ } >+ } else { >+ cred_handle = calloc(1, sizeof(struct gpp_cred_handle)); >+ if (!cred_handle) { >+ maj = GSS_S_FAILURE; >+ min = ENOMEM; >+ goto done; >+ } >+ } >+ > name = (struct gpp_name_handle *)target_name; > behavior = gpp_get_behavior(); > >@@ -205,11 +205,18 @@ done: > min = tmin; > } > if (maj != GSS_S_COMPLETE && maj != GSS_S_CONTINUE_NEEDED) { >- free(ctx_handle); >+ if (ctx_handle && >+ ctx_handle->local == GSS_C_NO_CONTEXT && >+ ctx_handle->remote == NULL) { >+ free(ctx_handle); >+ ctx_handle = NULL; >+ } > *minor_status = gpp_map_error(min); >- } else { >- *context_handle = (gss_ctx_id_t)ctx_handle; > } >+ /* always replace the provided context handle to avoid >+ * dangling pointers when a context has been passed in */ >+ *context_handle = (gss_ctx_id_t)ctx_handle; >+ > if (claimant_cred_handle == GSS_C_NO_CREDENTIAL) { > free(cred_handle); > } I'm sorry. I could have ran analysers before release but you were faster :-)