On Wed, 2015-03-25 at 14:30 +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/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".
I am ok with a patch to silence this error.
Simo.
--
Simo Sorce * Red Hat, Inc * New York