The following 2 patches go in tandem.
One is for libtirpc and the other for nfs-utils.
The standard gss_krb5_export_lucid_sec_context call actually deletes the gss context
after it has been exported, so these patches perform a complete hand over of the
context from libtirpc, so that the context handle can be nulled out in the
gss_krb5_export_lucid_sec_context and the code does not attempt to free it
twice and cause segmentation faults in the unlucky case.
Simo Sorce
On 05/04/13 18:37, Simo Sorce wrote:
> When using GSSAPI's gss_krb5_export_lucid_context the context passed into the
> function is actually deleted during the export (to avoid reuse as the context
> contains state that depends on its usage).
> Change the code to pass in a pointer to the context so that it can be properly
> NULLed if we are using the GSSAPI context and following calls to
> gss_delete_sec_context will not cause double free errors and segfaults.
>
> Signed-off-by: Simo Sorce <[email protected]>
Committed.....
steved.
> ---
> utils/gssd/context.c | 2 +-
> utils/gssd/context.h | 4 ++--
> utils/gssd/context_heimdal.c | 4 ++--
> utils/gssd/context_lucid.c | 4 ++--
> utils/gssd/context_mit.c | 4 ++--
> utils/gssd/gssd_proc.c | 4 ++--
> utils/gssd/svcgssd_proc.c | 2 +-
> 7 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/utils/gssd/context.c b/utils/gssd/context.c
> index fee7da27906e94b990fb7c49d73bf4f27a7003ac..7757a7700d14dd7fbe07f9878d6d79f514467156 100644
> --- a/utils/gssd/context.c
> +++ b/utils/gssd/context.c
> @@ -44,7 +44,7 @@
> #include "context.h"
>
> int
> -serialize_context_for_kernel(gss_ctx_id_t ctx,
> +serialize_context_for_kernel(gss_ctx_id_t *ctx,
> gss_buffer_desc *buf,
> gss_OID mech,
> int32_t *endtime)
> diff --git a/utils/gssd/context.h b/utils/gssd/context.h
> index 0e437f4a34f0862b8f89bd0d6fe62c41a3a1c906..3b55c8e4239cd9e814c8d540f0b3f90ade560107 100644
> --- a/utils/gssd/context.h
> +++ b/utils/gssd/context.h
> @@ -41,9 +41,9 @@
> #define KRB5_CTX_FLAG_CFX 0x00000002
> #define KRB5_CTX_FLAG_ACCEPTOR_SUBKEY 0x00000004
>
> -int serialize_context_for_kernel(gss_ctx_id_t ctx, gss_buffer_desc *buf,
> +int serialize_context_for_kernel(gss_ctx_id_t *ctx, gss_buffer_desc *buf,
> gss_OID mech, int32_t *endtime);
> -int serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf,
> +int serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf,
> int32_t *endtime);
>
> #endif /* _CONTEXT_H_ */
> diff --git a/utils/gssd/context_heimdal.c b/utils/gssd/context_heimdal.c
> index 6f3b8fd03f37e12a048337eed1d0d80e3bdb3224..1e8738aba96b9ad9c8b7017366355cd3001389fb 100644
> --- a/utils/gssd/context_heimdal.c
> +++ b/utils/gssd/context_heimdal.c
> @@ -203,9 +203,9 @@ int write_heimdal_seq_key(char **p, char *end, gss_ctx_id_t ctx)
> */
>
> int
> -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
> +serialize_krb5_ctx(gss_ctx_id_t *_ctx, gss_buffer_desc *buf, int32_t *endtime)
> {
> -
> + gss_ctx_id_t ctx = *_ctx;
> char *p, *end;
> static int constant_one = 1;
> static int constant_zero = 0;
> diff --git a/utils/gssd/context_lucid.c b/utils/gssd/context_lucid.c
> index 64146d7078c9f31a82b2c0435f9ca7a170c29874..badbe88d82ec5c8c957aa880758f9ac51381f5ee 100644
> --- a/utils/gssd/context_lucid.c
> +++ b/utils/gssd/context_lucid.c
> @@ -257,7 +257,7 @@ out_err:
>
>
> int
> -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
> +serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf, int32_t *endtime)
> {
> OM_uint32 maj_stat, min_stat;
> void *return_ctx = 0;
> @@ -266,7 +266,7 @@ serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
> int retcode = 0;
>
> printerr(2, "DEBUG: %s: lucid version!\n", __FUNCTION__);
> - maj_stat = gss_export_lucid_sec_context(&min_stat, &ctx,
> + maj_stat = gss_export_lucid_sec_context(&min_stat, ctx,
> 1, &return_ctx);
> if (maj_stat != GSS_S_COMPLETE) {
> pgsserr("gss_export_lucid_sec_context",
> diff --git a/utils/gssd/context_mit.c b/utils/gssd/context_mit.c
> index e6db9cbb77b858ba91f659b9a7f43ef312317a21..fad67569f47accdc6adc49bb0a59a025c5dd9973 100644
> --- a/utils/gssd/context_mit.c
> +++ b/utils/gssd/context_mit.c
> @@ -152,9 +152,9 @@ typedef struct gss_union_ctx_id_t {
> } gss_union_ctx_id_desc, *gss_union_ctx_id_t;
>
> int
> -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
> +serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf, int32_t *endtime)
> {
> - krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)ctx)->internal_ctx_id;
> + krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)(*ctx))->internal_ctx_id;
> char *p, *end;
> static int constant_zero = 0;
> static int constant_one = 1;
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 21d4e1d78eb54d177626cb0a19b9de4e93e0a20d..afc2076cde74e3cea6764408ec839a6f99b86ea4 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -1091,7 +1091,7 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> printerr(1, "WARNING: Failed to inquire context for lifetme "
> "maj_stat %u\n", maj_stat);
>
> - if (serialize_context_for_kernel(pd.pd_ctx, &token, &krb5oid, NULL)) {
> + if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
> printerr(0, "WARNING: Failed to serialize krb5 context for "
> "user with uid %d for server %s\n",
> uid, clp->servername);
> @@ -1104,7 +1104,7 @@ out:
> if (token.value)
> free(token.value);
> #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
> - if (pd.pd_ctx_hndl.length != 0)
> + if (pd.pd_ctx_hndl.length != 0 || pd.pd_ctx != 0)
> authgss_free_private_data(&pd);
> #endif
> if (auth)
> diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
> index 0d4f78d9668396aaebb05ab97804cfb1e8d5ed5a..3757d5191041b0341ea8ed11db6b26d3bea4f460 100644
> --- a/utils/gssd/svcgssd_proc.c
> +++ b/utils/gssd/svcgssd_proc.c
> @@ -484,7 +484,7 @@ handle_nullreq(FILE *f) {
>
> /* kernel needs ctx to calculate verifier on null response, so
> * must give it context before doing null call: */
> - if (serialize_context_for_kernel(ctx, &ctx_token, mech, &ctx_endtime)) {
> + if (serialize_context_for_kernel(&ctx, &ctx_token, mech, &ctx_endtime)) {
> printerr(0, "WARNING: handle_nullreq: "
> "serialize_context_for_kernel failed\n");
> maj_stat = GSS_S_FAILURE;
>
When using GSSAPI's gss_krb5_export_lucid_context the context passed into the
function is actually deleted during the export (to avoid reuse as the context
contains state that depends on its usage).
Change the code to pass in a pointer to the context so that it can be properly
NULLed if we are using the GSSAPI context and following calls to
gss_delete_sec_context will not cause double free errors and segfaults.
Signed-off-by: Simo Sorce <[email protected]>
---
utils/gssd/context.c | 2 +-
utils/gssd/context.h | 4 ++--
utils/gssd/context_heimdal.c | 4 ++--
utils/gssd/context_lucid.c | 4 ++--
utils/gssd/context_mit.c | 4 ++--
utils/gssd/gssd_proc.c | 4 ++--
utils/gssd/svcgssd_proc.c | 2 +-
7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/utils/gssd/context.c b/utils/gssd/context.c
index fee7da27906e94b990fb7c49d73bf4f27a7003ac..7757a7700d14dd7fbe07f9878d6d79f514467156 100644
--- a/utils/gssd/context.c
+++ b/utils/gssd/context.c
@@ -44,7 +44,7 @@
#include "context.h"
int
-serialize_context_for_kernel(gss_ctx_id_t ctx,
+serialize_context_for_kernel(gss_ctx_id_t *ctx,
gss_buffer_desc *buf,
gss_OID mech,
int32_t *endtime)
diff --git a/utils/gssd/context.h b/utils/gssd/context.h
index 0e437f4a34f0862b8f89bd0d6fe62c41a3a1c906..3b55c8e4239cd9e814c8d540f0b3f90ade560107 100644
--- a/utils/gssd/context.h
+++ b/utils/gssd/context.h
@@ -41,9 +41,9 @@
#define KRB5_CTX_FLAG_CFX 0x00000002
#define KRB5_CTX_FLAG_ACCEPTOR_SUBKEY 0x00000004
-int serialize_context_for_kernel(gss_ctx_id_t ctx, gss_buffer_desc *buf,
+int serialize_context_for_kernel(gss_ctx_id_t *ctx, gss_buffer_desc *buf,
gss_OID mech, int32_t *endtime);
-int serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf,
+int serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf,
int32_t *endtime);
#endif /* _CONTEXT_H_ */
diff --git a/utils/gssd/context_heimdal.c b/utils/gssd/context_heimdal.c
index 6f3b8fd03f37e12a048337eed1d0d80e3bdb3224..1e8738aba96b9ad9c8b7017366355cd3001389fb 100644
--- a/utils/gssd/context_heimdal.c
+++ b/utils/gssd/context_heimdal.c
@@ -203,9 +203,9 @@ int write_heimdal_seq_key(char **p, char *end, gss_ctx_id_t ctx)
*/
int
-serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
+serialize_krb5_ctx(gss_ctx_id_t *_ctx, gss_buffer_desc *buf, int32_t *endtime)
{
-
+ gss_ctx_id_t ctx = *_ctx;
char *p, *end;
static int constant_one = 1;
static int constant_zero = 0;
diff --git a/utils/gssd/context_lucid.c b/utils/gssd/context_lucid.c
index 64146d7078c9f31a82b2c0435f9ca7a170c29874..badbe88d82ec5c8c957aa880758f9ac51381f5ee 100644
--- a/utils/gssd/context_lucid.c
+++ b/utils/gssd/context_lucid.c
@@ -257,7 +257,7 @@ out_err:
int
-serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
+serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf, int32_t *endtime)
{
OM_uint32 maj_stat, min_stat;
void *return_ctx = 0;
@@ -266,7 +266,7 @@ serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
int retcode = 0;
printerr(2, "DEBUG: %s: lucid version!\n", __FUNCTION__);
- maj_stat = gss_export_lucid_sec_context(&min_stat, &ctx,
+ maj_stat = gss_export_lucid_sec_context(&min_stat, ctx,
1, &return_ctx);
if (maj_stat != GSS_S_COMPLETE) {
pgsserr("gss_export_lucid_sec_context",
diff --git a/utils/gssd/context_mit.c b/utils/gssd/context_mit.c
index e6db9cbb77b858ba91f659b9a7f43ef312317a21..fad67569f47accdc6adc49bb0a59a025c5dd9973 100644
--- a/utils/gssd/context_mit.c
+++ b/utils/gssd/context_mit.c
@@ -152,9 +152,9 @@ typedef struct gss_union_ctx_id_t {
} gss_union_ctx_id_desc, *gss_union_ctx_id_t;
int
-serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf, int32_t *endtime)
+serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf, int32_t *endtime)
{
- krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)ctx)->internal_ctx_id;
+ krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)(*ctx))->internal_ctx_id;
char *p, *end;
static int constant_zero = 0;
static int constant_one = 1;
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 21d4e1d78eb54d177626cb0a19b9de4e93e0a20d..afc2076cde74e3cea6764408ec839a6f99b86ea4 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -1091,7 +1091,7 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
printerr(1, "WARNING: Failed to inquire context for lifetme "
"maj_stat %u\n", maj_stat);
- if (serialize_context_for_kernel(pd.pd_ctx, &token, &krb5oid, NULL)) {
+ if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
printerr(0, "WARNING: Failed to serialize krb5 context for "
"user with uid %d for server %s\n",
uid, clp->servername);
@@ -1104,7 +1104,7 @@ out:
if (token.value)
free(token.value);
#ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
- if (pd.pd_ctx_hndl.length != 0)
+ if (pd.pd_ctx_hndl.length != 0 || pd.pd_ctx != 0)
authgss_free_private_data(&pd);
#endif
if (auth)
diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
index 0d4f78d9668396aaebb05ab97804cfb1e8d5ed5a..3757d5191041b0341ea8ed11db6b26d3bea4f460 100644
--- a/utils/gssd/svcgssd_proc.c
+++ b/utils/gssd/svcgssd_proc.c
@@ -484,7 +484,7 @@ handle_nullreq(FILE *f) {
/* kernel needs ctx to calculate verifier on null response, so
* must give it context before doing null call: */
- if (serialize_context_for_kernel(ctx, &ctx_token, mech, &ctx_endtime)) {
+ if (serialize_context_for_kernel(&ctx, &ctx_token, mech, &ctx_endtime)) {
printerr(0, "WARNING: handle_nullreq: "
"serialize_context_for_kernel failed\n");
maj_stat = GSS_S_FAILURE;
--
1.8.1.4
On Tue, 2013-04-09 at 15:41 -0400, Steve Dickson wrote:
>
> On 05/04/13 18:37, Simo Sorce wrote:
> > The following 2 patches go in tandem.
> > One is for libtirpc and the other for nfs-utils.
> How backwards compatible are these patches?
>
> Meaning will a patched nfs-utils work with a non-patch libtirpc and vises versus?
> It appears that is the case... I just want to make sure I'm not missing
> something...
As far as I know fully backwards compatible.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
On 05/04/13 18:37, Simo Sorce wrote:
> The following 2 patches go in tandem.
> One is for libtirpc and the other for nfs-utils.
How backwards compatible are these patches?
Meaning will a patched nfs-utils work with a non-patch libtirpc and vises versus?
It appears that is the case... I just want to make sure I'm not missing
something...
steved.
>
> The standard gss_krb5_export_lucid_sec_context call actually deletes the gss context
> after it has been exported, so these patches perform a complete hand over of the
> context from libtirpc, so that the context handle can be nulled out in the
> gss_krb5_export_lucid_sec_context and the code does not attempt to free it
> twice and cause segmentation faults in the unlucky case.
>
On 05/04/13 18:37, Simo Sorce wrote:
> When the private data is given away the gss context also needs to go,
> because the caller may destroy it, such as when the context is exported
> into a lucid context to hand it to the kernel.
> ---
> src/auth_gss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Committed...
steved.
>
> diff --git a/src/auth_gss.c b/src/auth_gss.c
> index 81ae8aee316c6f42f317f81cd1438369fb2102c5..703bc3f7b42236b0d4cc3ddbd8935df2aaccf85a 100644
> --- a/src/auth_gss.c
> +++ b/src/auth_gss.c
> @@ -269,6 +269,7 @@ authgss_get_private_data(AUTH *auth, struct authgss_private_data *pd)
> * send an RPCSEC_GSS_DESTROY request which might inappropriately
> * destroy the context.
> */
> + gd->ctx = GSS_C_NO_CONTEXT;
> gd->gc.gc_ctx.length = 0;
> gd->gc.gc_ctx.value = NULL;
>
> @@ -284,7 +285,8 @@ authgss_free_private_data(struct authgss_private_data *pd)
> if (!pd)
> return (FALSE);
>
> - pd->pd_ctx = NULL;
> + if (pd->pd_ctx != GSS_C_NO_CONTEXT)
> + gss_delete_sec_context(&min_stat, &pd->pd_ctx, NULL);
> gss_release_buffer(&min_stat, &pd->pd_ctx_hndl);
> memset(&pd->pd_ctx_hndl, 0, sizeof(pd->pd_ctx_hndl));
> pd->pd_seq_win = 0;
>
When the private data is given away the gss context also needs to go,
because the caller may destroy it, such as when the context is exported
into a lucid context to hand it to the kernel.
---
src/auth_gss.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/auth_gss.c b/src/auth_gss.c
index 81ae8aee316c6f42f317f81cd1438369fb2102c5..703bc3f7b42236b0d4cc3ddbd8935df2aaccf85a 100644
--- a/src/auth_gss.c
+++ b/src/auth_gss.c
@@ -269,6 +269,7 @@ authgss_get_private_data(AUTH *auth, struct authgss_private_data *pd)
* send an RPCSEC_GSS_DESTROY request which might inappropriately
* destroy the context.
*/
+ gd->ctx = GSS_C_NO_CONTEXT;
gd->gc.gc_ctx.length = 0;
gd->gc.gc_ctx.value = NULL;
@@ -284,7 +285,8 @@ authgss_free_private_data(struct authgss_private_data *pd)
if (!pd)
return (FALSE);
- pd->pd_ctx = NULL;
+ if (pd->pd_ctx != GSS_C_NO_CONTEXT)
+ gss_delete_sec_context(&min_stat, &pd->pd_ctx, NULL);
gss_release_buffer(&min_stat, &pd->pd_ctx_hndl);
memset(&pd->pd_ctx_hndl, 0, sizeof(pd->pd_ctx_hndl));
pd->pd_seq_win = 0;
--
1.8.1.4