On Wed, 2015-03-25 at 14:42 +0100, Lukas Slebodnik wrote:
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>
>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>
>---
> 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 :-)
These are minor problems, send patches, I can release a 0.4.1 in a few
weeks if we have enough patches.
Simo.
--
Simo Sorce * Red Hat, Inc * New York