See extended comment in the patch.
Fixes issue #137
Simo.
ACK
On Sat, 2015-03-21 at 14:41 -0400, 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.
gss-proxy mailing list gss-proxy@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/gss-proxy
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@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@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".
LS
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@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@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.
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@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@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 :-)
LS
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@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@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.
gss-proxy@lists.fedorahosted.org