>From 14d02a392aa01136a05cedd6e403d9bc3704204a Mon Sep 17 00:00:00 2001
From: Simo Sorce <simo@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 fund by NFS folks @ Red Hat

Signed-off-by: Simo Sorce <simo@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/client/gpm_accept_sec_context.c b/proxy/src/client/gpm_accept_sec_context.c
index 5f8fb06297dfb87631d9af261ac94c3d6267d8f8..dc034434498912db83af1bfcfe8f413f7e3afcf1 100644
--- a/proxy/src/client/gpm_accept_sec_context.c
+++ b/proxy/src/client/gpm_accept_sec_context.c
@@ -116,12 +116,6 @@ OM_uint32 gpm_accept_sec_context(OM_uint32 *minor_status,
         goto done;
     }
 
-    /* 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 (mech_type) {
         *mech_type = mech;
     }
@@ -157,6 +151,7 @@ done:
     arg->context_handle = NULL;
     arg->cred_handle = NULL;
     gpm_free_xdrs(GSSX_ACCEPT_SEC_CONTEXT, &uarg, &ures);
+
     if (ret) {
         if (ctx) {
             xdr_free((xdrproc_t)xdr_gssx_ctx, (char *)ctx);
@@ -178,6 +173,13 @@ done:
         return GSS_S_FAILURE;
     }
 
+    /* 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;
+
     return ret_maj;
 }
 
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;
+
     *minor_status = ret_min;
     return ret_maj;
 }
diff --git a/proxy/src/mechglue/gpp_accept_sec_context.c b/proxy/src/mechglue/gpp_accept_sec_context.c
index d91728e675c7499859dbda55bab8658f17055bb3..a40f23be33fef6457267646b24e3f76493623db8 100644
--- a/proxy/src/mechglue/gpp_accept_sec_context.c
+++ b/proxy/src/mechglue/gpp_accept_sec_context.c
@@ -138,7 +138,12 @@ OM_uint32 gssi_accept_sec_context(OM_uint32 *minor_status,
 done:
     *minor_status = gpp_map_error(min);
     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;
+        }
         free(deleg_cred);
         free(name);
     } else {
@@ -148,8 +153,11 @@ done:
         if (delegated_cred_handle) {
             *delegated_cred_handle = (gss_cred_id_t)deleg_cred;
         }
-        *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 (acceptor_cred_handle == GSS_C_NO_CREDENTIAL) {
         (void)gssi_release_cred(&min, (gss_cred_id_t *)&cred_handle);
     }
diff --git a/proxy/src/mechglue/gpp_context.c b/proxy/src/mechglue/gpp_context.c
index bb16a93a39b5892388ba9e963fff9b0066283cf4..83272b309758e8e4de2c01554ea118b051cc7232 100644
--- a/proxy/src/mechglue/gpp_context.c
+++ b/proxy/src/mechglue/gpp_context.c
@@ -364,6 +364,11 @@ OM_uint32 gssi_delete_sec_context(OM_uint32 *minor_status,
 
     *context_handle = GSS_C_NO_CONTEXT;
 
+    if (ctx == NULL) {
+        *minor_status = 0;
+        return GSS_S_COMPLETE;
+    }
+
     if (ctx->local) {
         maj = gss_delete_sec_context(&min, &ctx->local, output_token);
         if (maj != GSS_S_COMPLETE) {
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;
-        }
-    }
-
     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);
     }
-- 
2.1.0

