From 1d78d1af3da7eeb15aa1f054b740f31a12f48f31 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Sat, 16 Nov 2013 17:08:06 -0500 Subject: [PATCH 1/3] config: Do not modify const strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Take a copy here, the option string is const and strtok_r() is not a safe function as it may change the string it manipulates. Reviewed-by: Günther Deschner --- proxy/src/gp_config.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/proxy/src/gp_config.c b/proxy/src/gp_config.c index e21e70d..63f264e 100644 --- a/proxy/src/gp_config.c +++ b/proxy/src/gp_config.c @@ -209,6 +209,7 @@ static int load_services(struct gp_config *cfg, struct gp_ini_context *ctx) int num_sec; char *secname = NULL; const char *value; + char *vcopy; char *token; char *handle; int valnum; @@ -318,7 +319,12 @@ static int load_services(struct gp_config *cfg, struct gp_ini_context *ctx) goto done; } - token = strtok_r(no_const(value), ", ", &handle); + vcopy = strdup(value); + if (!vcopy) { + ret = ENOMEM; + goto done; + } + token = strtok_r(vcopy, ", ", &handle); do { ret = strcmp(value, "krb5"); @@ -329,6 +335,7 @@ static int load_services(struct gp_config *cfg, struct gp_ini_context *ctx) } else { GPERROR("Failed to read krb5 config for %s.\n", secname); + safefree(vcopy); return ret; } } else { @@ -338,6 +345,7 @@ static int load_services(struct gp_config *cfg, struct gp_ini_context *ctx) token = strtok_r(NULL, ", ", &handle); } while (token != NULL); + safefree(vcopy); if (cfg->svcs[n]->mechs == 0) { GPDEBUG("No mechs found for [%s], ignoring.\n", secname); -- 1.8.3.1 From a272091dfd568cb96738cc96ea01bbf7f24ee62c Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Sat, 16 Nov 2013 18:54:28 -0500 Subject: [PATCH 2/3] creds: Allow admins to define only client creds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a service is configured with cred_usage = initiate it is ok to allow only client credentials to be defined. Reviewed-by: Günther Deschner --- proxy/src/gp_creds.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/proxy/src/gp_creds.c b/proxy/src/gp_creds.c index 60c4e12..1ac1fac 100644 --- a/proxy/src/gp_creds.c +++ b/proxy/src/gp_creds.c @@ -376,7 +376,12 @@ static int gp_get_cred_environment(struct gp_call_ctx *gpcall, * if any. */ if (use_service_keytab) { if (k_num == -1) { - ret = EINVAL; + if (ck_num == -1) { + ret = EINVAL; + } else { + /* allow a service to define only the client keytab */ + ret = 0; + } goto done; } if (ck_num == -1) { -- 1.8.3.1 From 23f4ee4359d10f66e1938ce6b1d92d3cc77865ff Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Wed, 20 Nov 2013 11:58:22 -0500 Subject: [PATCH 3/3] Use secure_getenv in client and mechglue module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit proxymehc.so may be used in setuid binaries so follow best security practices and use secure_getenv() if available. Fallback to poorman emulation when secure_getenv() is not available. Resolves: https://fedorahosted.org/gss-proxy/ticket/110 Reviewed-by: Günther Deschner --- proxy/Makefile.am | 7 ++++--- proxy/configure.ac | 2 ++ proxy/src/client/gpm_common.c | 2 +- proxy/src/gp_common.h | 1 + proxy/src/gp_util.c | 20 ++++++++++++++++++++ proxy/src/mechglue/gss_plugin.c | 4 ++-- 6 files changed, 30 insertions(+), 6 deletions(-) diff --git a/proxy/Makefile.am b/proxy/Makefile.am index 065be6e..c946421 100644 --- a/proxy/Makefile.am +++ b/proxy/Makefile.am @@ -102,7 +102,9 @@ GP_RPCCLI_OBJ = \ src/client/gpm_wrap.c \ src/client/gpm_unwrap.c \ src/client/gpm_wrap_size_limit.c \ - src/client/gpm_common.c + src/client/gpm_common.c \ + src/gp_util.c + GP_MECHGLUE_OBJ = \ src/mechglue/gpp_accept_sec_context.c \ src/mechglue/gpp_acquire_cred.c \ @@ -114,8 +116,7 @@ GP_MECHGLUE_OBJ = \ src/mechglue/gpp_indicate_mechs.c \ src/mechglue/gpp_priv_integ.c \ src/mechglue/gpp_misc.c \ - src/mechglue/gss_plugin.c \ - src/gp_util.c + src/mechglue/gss_plugin.c dist_noinst_HEADERS = \ rpcgen/gp_rpc.h \ diff --git a/proxy/configure.ac b/proxy/configure.ac index b75a1ef..a0cc4ef 100644 --- a/proxy/configure.ac +++ b/proxy/configure.ac @@ -149,6 +149,8 @@ AC_CHECK_LIB(gssrpc, gssrpc_xdrmem_create,, [$GSSAPI_LIBS $GSSRPC_LIBS]) AC_SUBST([GSSRPC_LIBS]) +AC_CHECK_FUNCS([__secure_getenv secure_getenv]) + WITH_INITSCRIPT if test x$initscript = xsystemd; then WITH_SYSTEMD_UNIT_DIR diff --git a/proxy/src/client/gpm_common.c b/proxy/src/client/gpm_common.c index df1f5a1..74296da 100644 --- a/proxy/src/client/gpm_common.c +++ b/proxy/src/client/gpm_common.c @@ -68,7 +68,7 @@ static int get_pipe_name(struct gpm_ctx *gpmctx, char *name) const char *socket; int ret; - socket = getenv("GSSPROXY_SOCKET"); + socket = gp_getenv("GSSPROXY_SOCKET"); if (!socket) { socket = GP_SOCKET_NAME; } diff --git a/proxy/src/gp_common.h b/proxy/src/gp_common.h index 9e4ae81..b5c525f 100644 --- a/proxy/src/gp_common.h +++ b/proxy/src/gp_common.h @@ -67,6 +67,7 @@ bool gp_same(const char *a, const char *b); bool gp_boolean_is_true(const char *s); +char *gp_getenv(const char *name); #include "rpcgen/gss_proxy.h" diff --git a/proxy/src/gp_util.c b/proxy/src/gp_util.c index 8400da1..a6c870f 100644 --- a/proxy/src/gp_util.c +++ b/proxy/src/gp_util.c @@ -23,8 +23,10 @@ DEALINGS IN THE SOFTWARE. */ +#include "config.h" #include #include +#include bool gp_same(const char *a, const char *b) { @@ -46,3 +48,21 @@ bool gp_boolean_is_true(const char *s) return false; } + +char *gp_getenv(const char *name) +{ +#if HAVE_SECURE_GETENV + return secure_getenv(name); +#elif HAVE___SECURE_GETENV + return __secure_getenv(name); +#else +#include +#include +#warning secure_getenv not available, falling back to poorman emulation + if ((getuid() == geteuid()) && + (getgid() == getegid())) { + return getenv(name); + } + return NULL; +#endif +} diff --git a/proxy/src/mechglue/gss_plugin.c b/proxy/src/mechglue/gss_plugin.c index 5b40df9..372ab2e 100644 --- a/proxy/src/mechglue/gss_plugin.c +++ b/proxy/src/mechglue/gss_plugin.c @@ -64,7 +64,7 @@ enum gpp_behavior gpp_get_behavior(void) char *envval; if (behavior == GPP_UNINITIALIZED) { - envval = getenv("GSSPROXY_BEHAVIOR"); + envval = gp_getenv("GSSPROXY_BEHAVIOR"); if (envval) { if (strcmp(envval, "LOCAL_ONLY") == 0) { behavior = GPP_LOCAL_ONLY; @@ -102,7 +102,7 @@ gss_OID_set gss_mech_interposer(gss_OID mech_type) /* avoid looping in the gssproxy daemon by avoiding to interpose * any mechanism */ - envval = getenv("GSS_USE_PROXY"); + envval = gp_getenv("GSS_USE_PROXY"); if (!envval) { return NULL; } -- 1.8.3.1