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

Fixes: https://fedorahosted.org/gss-proxy/ticket/137

Signed-off-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Nathaniel McCallum <npmccallum@redhat.com>

5 files changed, 62 insertions, 39 deletions

diff --git a/proxy/src/client/gpm_accept_sec_context.c b/proxy/src/client/gpm_accept_sec_context.c
index 5f8fb06..dc03443 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 bd88055..14c65e4 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 d91728e..a40f23b 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 bb16a93..83272b3 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 e70e8fc..c80937c 100644
--- a/proxy/src/mechglue/gpp_init_sec_context.c
+++ b/proxy/src/mechglue/gpp_init_sec_context.c
@@ -91,35 +91,18 @@ 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;
     }
 
-    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;
+        return GSS_S_BAD_MECH;
     }
 
-    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;
-        }
-    }
+    tmaj = GSS_S_COMPLETE;
+    tmin = 0;
 
     if (*context_handle) {
         ctx_handle = (struct gpp_context_handle *)*context_handle;
@@ -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);
     }