Adding support for multi-threaded gssd and allow for parallel upcalls.
For each upcall create a thread, detach to allow for automatic cleanup.
Communicate location of credential cache with gss api function instead
of an environmental variable. Use syscall api instead of libc setuid()
to change user identity.
Previously parent thread after detaching from child thread would call
yield to let the child thread run and consume the event from the fd.
Instead, use synchronization variables to signal parent after reading
from fd and have parent thread wait for the event.
Olga Kornievskaia (3):
gssd: use pthreads to handle upcalls
gssd: using syscalls directly to change thread's identity
gssd: always call gss_krb5_ccache_name
aclocal/libpthread.m4 | 13 ++++++++++
configure.ac | 3 +++
utils/gssd/Makefile.am | 3 ++-
utils/gssd/gssd.c | 44 ++++++++++++++++++++++++++++++---
utils/gssd/gssd.h | 5 ++++
utils/gssd/gssd_proc.c | 66 +++++++++++++++++++++++---------------------------
utils/gssd/krb5_util.c | 59 ++++++++++----------------------------------
utils/gssd/krb5_util.h | 1 -
8 files changed, 106 insertions(+), 88 deletions(-)
create mode 100644 aclocal/libpthread.m4
--
1.8.3.1
For the threaded version we have to set uid,gid per thread instead
of per process. glibc setresuid() when called from a thread, it'll
send a signal to all other threads to synchronize the uid in all
other threads. To bypass this, we have to call syscall() directly.
Signed-off-by: Olga Kornievskaia <[email protected]>
Reviewed-by: Steve Dickson <[email protected]>
---
utils/gssd/gssd_proc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 581a125..5d9a6db 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -69,6 +69,7 @@
#include <netdb.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <syscall.h>
#include "gssd.h"
#include "err_util.h"
@@ -457,7 +458,12 @@ change_identity(uid_t uid)
* Switch the GIDs. Note that we leave the saved-set-gid alone in an
* attempt to prevent attacks via ptrace()
*/
- if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
+ /* For the threaded version we have to set uid,gid per thread instead
+ * of per process. glibc setresuid() when called from a thread, it'll
+ * send a signal to all other threads to synchronize the uid in all
+ * other threads. To bypass this, we have to call syscall() directly.
+ */
+ if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
return errno;
}
@@ -466,7 +472,7 @@ change_identity(uid_t uid)
* Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
* other processes running with this uid.
*/
- if (setresuid(uid, uid, -1) != 0) {
+ if (syscall(SYS_setresuid, uid) != 0) {
printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
uid);
return errno;
--
1.8.3.1
Previously the location of the credential cache was passed in either
using environment variable KRB5CCNAME or gss_krb5_ccache_name() if
supported. For threaded-gssd, we can't use an environment variable
as it's shared among all thread. Thus always use the api call.
Signed-off-by: Olga Kornievskaia <[email protected]>
Reviewed-by: Steve Dickson <[email protected]>
---
utils/gssd/gssd_proc.c | 12 +++++++++--
utils/gssd/krb5_util.c | 56 +++++++++-----------------------------------------
utils/gssd/krb5_util.h | 1 -
3 files changed, 20 insertions(+), 49 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 5d9a6db..b8cd648 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -554,7 +554,15 @@ krb5_use_machine_creds(struct clnt_info *clp, uid_t uid, char *tgtname,
goto out;
}
for (ccname = credlist; ccname && *ccname; ccname++) {
- gssd_setup_krb5_machine_gss_ccache(*ccname);
+ u_int min_stat;
+
+ if (gss_krb5_ccache_name(&min_stat, *ccname, NULL) !=
+ GSS_S_COMPLETE) {
+ printerr(1, "WARNING: gss_krb5_ccache_name "
+ "with name '%s' failed (%s)\n",
+ *ccname, error_message(min_stat));
+ continue;
+ }
if ((create_auth_rpc_client(clp, tgtname, rpc_clnt,
&auth, uid,
AUTHTYPE_KRB5,
@@ -575,7 +583,7 @@ krb5_use_machine_creds(struct clnt_info *clp, uid_t uid, char *tgtname,
"recreate cache for server %s\n",
clp->servername);
} else {
- printerr(1, "WARNING: Failed to create machine"
+ printerr(0, "ERROR: Failed to create machine"
"krb5 context with any credentials"
"cache for server %s\n",
clp->servername);
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 3328696..7b74ab3 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -468,37 +468,6 @@ gssd_get_single_krb5_cred(krb5_context context,
}
/*
- * Depending on the version of Kerberos, we either need to use
- * a private function, or simply set the environment variable.
- */
-static void
-gssd_set_krb5_ccache_name(char *ccname)
-{
-#ifdef USE_GSS_KRB5_CCACHE_NAME
- u_int maj_stat, min_stat;
-
- printerr(3, "using gss_krb5_ccache_name to select krb5 ccache %s\n",
- ccname);
- maj_stat = gss_krb5_ccache_name(&min_stat, ccname, NULL);
- if (maj_stat != GSS_S_COMPLETE) {
- printerr(0, "WARNING: gss_krb5_ccache_name with "
- "name '%s' failed (%s)\n",
- ccname, error_message(min_stat));
- }
-#else
- /*
- * Set the KRB5CCNAME environment variable to tell the krb5 code
- * which credentials cache to use. (Instead of using the private
- * function above for which there is no generic gssapi
- * equivalent.)
- */
- printerr(3, "using environment variable to select krb5 ccache %s\n",
- ccname);
- setenv("KRB5CCNAME", ccname, 1);
-#endif
-}
-
-/*
* Given a principal, find a matching ple structure
*/
static struct gssd_k5_kt_princ *
@@ -1094,6 +1063,7 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
const char *cctype;
struct dirent *d;
int err, i, j;
+ u_int maj_stat, min_stat;
printerr(3, "looking for client creds with uid %u for "
"server %s in %s\n", uid, servername, dirpattern);
@@ -1129,22 +1099,16 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
printerr(2, "using %s as credentials cache for client with "
"uid %u for server %s\n", buf, uid, servername);
- gssd_set_krb5_ccache_name(buf);
- return 0;
-}
-/*
- * Let the gss code know where to find the machine credentials ccache.
- *
- * Returns:
- * void
- */
-void
-gssd_setup_krb5_machine_gss_ccache(char *ccname)
-{
- printerr(2, "using %s as credentials cache for machine creds\n",
- ccname);
- gssd_set_krb5_ccache_name(ccname);
+ printerr(3, "using gss_krb5_ccache_name to select krb5 ccache %s\n",
+ buf);
+ maj_stat = gss_krb5_ccache_name(&min_stat, buf, NULL);
+ if (maj_stat != GSS_S_COMPLETE) {
+ printerr(0, "ERROR: unable to get user cred cache '%s' "
+ "failed (%s)\n", buf, error_message(min_stat));
+ return maj_stat;
+ }
+ return 0;
}
/*
diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
index a319588..d3b0777 100644
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -27,7 +27,6 @@ int gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername,
char *dirname);
int gssd_get_krb5_machine_cred_list(char ***list);
void gssd_free_krb5_machine_cred_list(char **list);
-void gssd_setup_krb5_machine_gss_ccache(char *servername);
void gssd_destroy_krb5_machine_creds(void);
int gssd_refresh_krb5_machine_credential(char *hostname,
struct gssd_k5_kt_princ *ple,
--
1.8.3.1
Currently, to persevere global data over multiple mounts,
the root process does not fork when handling an upcall.
Instead on not-forking create a pthread to handle the
upcall since global data can be shared among threads.
Signed-off-by: Steve Dickson <[email protected]>
Signed-off-by: Olga Kornievskaia <[email protected]>
---
aclocal/libpthread.m4 | 13 +++++++++++++
configure.ac | 3 +++
utils/gssd/Makefile.am | 3 ++-
utils/gssd/gssd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
utils/gssd/gssd.h | 5 +++++
utils/gssd/gssd_proc.c | 44 ++++++++++++--------------------------------
utils/gssd/krb5_util.c | 3 +++
7 files changed, 78 insertions(+), 37 deletions(-)
create mode 100644 aclocal/libpthread.m4
diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
new file mode 100644
index 0000000..e87d2a0
--- /dev/null
+++ b/aclocal/libpthread.m4
@@ -0,0 +1,13 @@
+dnl Checks for pthreads library and headers
+dnl
+AC_DEFUN([AC_LIBPTHREAD], [
+
+ dnl Check for library, but do not add -lpthreads to LIBS
+ AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
+ [AC_MSG_ERROR([libpthread not found.])])
+ AC_SUBST(LIBPTHREAD)
+
+ AC_CHECK_HEADERS([pthread.h], ,
+ [AC_MSG_ERROR([libpthread headers not found.])])
+
+])dnl
diff --git a/configure.ac b/configure.ac
index 25d2ba4..e0ecd61 100644
--- a/configure.ac
+++ b/configure.ac
@@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
AC_LIBRPCSECGSS
+ dnl Check for pthreads
+ AC_LIBPTHREAD
+
dnl librpcsecgss already has a dependency on libgssapi,
dnl but we need to make sure we get the right version
if test "$enable_gss" = yes; then
diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
index cb040b3..3f5f59a 100644
--- a/utils/gssd/Makefile.am
+++ b/utils/gssd/Makefile.am
@@ -49,7 +49,8 @@ gssd_LDADD = \
$(RPCSECGSS_LIBS) \
$(KRBLIBS) \
$(GSSAPI_LIBS) \
- $(LIBTIRPC)
+ $(LIBTIRPC) \
+ $(LIBPTHREAD)
gssd_LDFLAGS = \
$(KRBLDFLAGS)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index e7cb07f..a41b5ad 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
/* Avoid DNS reverse lookups on server names */
static bool avoid_dns = true;
-
+int thread_started = false;
+pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
@@ -361,20 +363,54 @@ gssd_destroy_client(struct clnt_info *clp)
static void gssd_scan(void);
+/* For each upcall create a thread, detach from the main process so that
+ * resources are released back into the system without the need for a join.
+ * We need to wait for the child thread to start and consume the event from
+ * the file descriptor.
+ */
static void
gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
-
- handle_gssd_upcall(clp);
+ pthread_t th;
+ int ret;
+
+ ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
+ (void *)clp);
+ if (ret != 0) {
+ printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
+ ret, strerror(errno));
+ return;
+ }
+ pthread_mutex_lock(&pmutex);
+ while(!thread_started)
+ pthread_cond_wait(&pcond, &pmutex);
+ thread_started = false;
+ pthread_mutex_unlock(&pmutex);
+ pthread_detach(th);
}
static void
gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
+ pthread_t th;
+ int ret;
+
+ ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
+ (void *)clp);
+ if (ret != 0) {
+ printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
+ ret, strerror(errno));
+ return;
+ }
- handle_krb5_upcall(clp);
+ pthread_mutex_lock(&pmutex);
+ while(!thread_started)
+ pthread_cond_wait(&pcond, &pmutex);
+ thread_started = false;
+ pthread_mutex_unlock(&pmutex);
+ pthread_detach(th);
}
static struct clnt_info *
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index c6937c5..565bce3 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -36,6 +36,7 @@
#include <gssapi/gssapi.h>
#include <event.h>
#include <stdbool.h>
+#include <pthread.h>
#ifndef GSSD_PIPEFS_DIR
#define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
@@ -61,6 +62,10 @@ extern int root_uses_machine_creds;
extern unsigned int context_timeout;
extern unsigned int rpc_timeout;
extern char *preferred_realm;
+extern pthread_mutex_t ple_lock;
+extern pthread_cond_t pcond;
+extern pthread_mutex_t pmutex;
+extern int thread_started;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 1ef68d8..581a125 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -629,36 +629,6 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
service == NULL)) {
- /* already running as uid 0 */
- if (uid == 0)
- goto no_fork;
-
- pid = fork();
- switch(pid) {
- case 0:
- /* Child: fall through to rest of function */
- childpid = getpid();
- unsetenv("KRB5CCNAME");
- printerr(2, "CHILD forked pid %d \n", childpid);
- break;
- case -1:
- /* fork() failed! */
- printerr(0, "WARNING: unable to fork() to handle"
- "upcall: %s\n", strerror(errno));
- return;
- default:
- /* Parent: just wait on child to exit and return */
- do {
- pid = wait(&err);
- } while(pid == -1 && errno != -ECHILD);
-
- if (WIFSIGNALED(err))
- printerr(0, "WARNING: forked child was killed"
- "with signal %d\n", WTERMSIG(err));
- return;
- }
-no_fork:
-
auth = krb5_not_machine_creds(clp, uid, tgtname, &downcall_err,
&err, &rpc_clnt);
if (err)
@@ -739,8 +709,14 @@ void
handle_krb5_upcall(struct clnt_info *clp)
{
uid_t uid;
-
- if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
+ int status;
+
+ status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
+ pthread_mutex_lock(&pmutex);
+ thread_started = true;
+ pthread_cond_signal(&pcond);
+ pthread_mutex_unlock(&pmutex);
+ if (status) {
printerr(0, "WARNING: failed reading uid from krb5 "
"upcall pipe: %s\n", strerror(errno));
return;
@@ -765,6 +741,10 @@ handle_gssd_upcall(struct clnt_info *clp)
char *enctypes = NULL;
lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
+ pthread_mutex_lock(&pmutex);
+ thread_started = true;
+ pthread_cond_signal(&pcond);
+ pthread_mutex_unlock(&pmutex);
if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
printerr(0, "WARNING: handle_gssd_upcall: "
"failed reading request\n");
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 8ef8184..3328696 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -128,6 +128,7 @@
/* Global list of principals/cache file names for machine credentials */
struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
+pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
int limit_to_legacy_enctypes = 0;
@@ -586,10 +587,12 @@ get_ple_by_princ(krb5_context context, krb5_principal princ)
/* Need to serialize list if we ever become multi-threaded! */
+ pthread_mutex_lock(&ple_lock);
ple = find_ple_by_princ(context, princ);
if (ple == NULL) {
ple = new_ple(context, princ);
}
+ pthread_mutex_unlock(&ple_lock);
return ple;
}
--
1.8.3.1
Hello,
On 04/20/2016 05:11 PM, Olga Kornievskaia wrote:
> Currently, to persevere global data over multiple mounts,
> the root process does not fork when handling an upcall.
> Instead on not-forking create a pthread to handle the
> upcall since global data can be shared among threads.
>
> Signed-off-by: Steve Dickson <[email protected]>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> aclocal/libpthread.m4 | 13 +++++++++++++
> configure.ac | 3 +++
> utils/gssd/Makefile.am | 3 ++-
> utils/gssd/gssd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
> utils/gssd/gssd.h | 5 +++++
> utils/gssd/gssd_proc.c | 44 ++++++++++++--------------------------------
> utils/gssd/krb5_util.c | 3 +++
> 7 files changed, 78 insertions(+), 37 deletions(-)
> create mode 100644 aclocal/libpthread.m4
>
> diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
> new file mode 100644
> index 0000000..e87d2a0
> --- /dev/null
> +++ b/aclocal/libpthread.m4
> @@ -0,0 +1,13 @@
> +dnl Checks for pthreads library and headers
> +dnl
> +AC_DEFUN([AC_LIBPTHREAD], [
> +
> + dnl Check for library, but do not add -lpthreads to LIBS
> + AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
> + [AC_MSG_ERROR([libpthread not found.])])
> + AC_SUBST(LIBPTHREAD)
> +
> + AC_CHECK_HEADERS([pthread.h], ,
> + [AC_MSG_ERROR([libpthread headers not found.])])
> +
> +])dnl
> diff --git a/configure.ac b/configure.ac
> index 25d2ba4..e0ecd61 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
> dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
> AC_LIBRPCSECGSS
>
> + dnl Check for pthreads
> + AC_LIBPTHREAD
> +
> dnl librpcsecgss already has a dependency on libgssapi,
> dnl but we need to make sure we get the right version
> if test "$enable_gss" = yes; then
> diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
> index cb040b3..3f5f59a 100644
> --- a/utils/gssd/Makefile.am
> +++ b/utils/gssd/Makefile.am
> @@ -49,7 +49,8 @@ gssd_LDADD = \
> $(RPCSECGSS_LIBS) \
> $(KRBLIBS) \
> $(GSSAPI_LIBS) \
> - $(LIBTIRPC)
> + $(LIBTIRPC) \
> + $(LIBPTHREAD)
>
> gssd_LDFLAGS = \
> $(KRBLDFLAGS)
> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> index e7cb07f..a41b5ad 100644
> --- a/utils/gssd/gssd.c
> +++ b/utils/gssd/gssd.c
> @@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
> char *preferred_realm = NULL;
> /* Avoid DNS reverse lookups on server names */
> static bool avoid_dns = true;
> -
> +int thread_started = false;
> +pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
> +pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
>
> TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
>
> @@ -361,20 +363,54 @@ gssd_destroy_client(struct clnt_info *clp)
>
> static void gssd_scan(void);
>
> +/* For each upcall create a thread, detach from the main process so that
> + * resources are released back into the system without the need for a join.
> + * We need to wait for the child thread to start and consume the event from
> + * the file descriptor.
> + */
> static void
> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
> {
> struct clnt_info *clp = data;
> -
> - handle_gssd_upcall(clp);
> + pthread_t th;
> + int ret;
> +
> + ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
> + (void *)clp);
> + if (ret != 0) {
> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> + ret, strerror(errno));
> + return;
> + }
> + pthread_mutex_lock(&pmutex);
> + while(!thread_started)
> + pthread_cond_wait(&pcond, &pmutex);
> + thread_started = false;
> + pthread_mutex_unlock(&pmutex);
> + pthread_detach(th);
This synchronization works! I'm no longer seeing any read errors.
But I would like document what they are doing.
So we don't have the same comments in two different
places, I suggest you put all this calls in a macro, like
wait_for_cthread() or something... whatever you thinks
works..
Then where the macro is define you can document what is
doing and way its needed... Plus this condenses 6
lines into one (hopefully) meaningful line.
> }
>
> static void
> gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
> {
> struct clnt_info *clp = data;
> + pthread_t th;
> + int ret;
> +
> + ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
> + (void *)clp);
> + if (ret != 0) {
> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> + ret, strerror(errno));
> + return;
> + }
>
> - handle_krb5_upcall(clp);
> + pthread_mutex_lock(&pmutex);
> + while(!thread_started)
> + pthread_cond_wait(&pcond, &pmutex);
> + thread_started = false;
> + pthread_mutex_unlock(&pmutex);
> + pthread_detach(th);
The same macro here
> }
>
> static struct clnt_info *
> diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> index c6937c5..565bce3 100644
> --- a/utils/gssd/gssd.h
> +++ b/utils/gssd/gssd.h
> @@ -36,6 +36,7 @@
> #include <gssapi/gssapi.h>
> #include <event.h>
> #include <stdbool.h>
> +#include <pthread.h>
>
> #ifndef GSSD_PIPEFS_DIR
> #define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
> @@ -61,6 +62,10 @@ extern int root_uses_machine_creds;
> extern unsigned int context_timeout;
> extern unsigned int rpc_timeout;
> extern char *preferred_realm;
> +extern pthread_mutex_t ple_lock;
> +extern pthread_cond_t pcond;
> +extern pthread_mutex_t pmutex;
> +extern int thread_started;
>
> struct clnt_info {
> TAILQ_ENTRY(clnt_info) list;
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 1ef68d8..581a125 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -629,36 +629,6 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
> service == NULL)) {
>
> - /* already running as uid 0 */
> - if (uid == 0)
> - goto no_fork;
> -
> - pid = fork();
> - switch(pid) {
> - case 0:
> - /* Child: fall through to rest of function */
> - childpid = getpid();
> - unsetenv("KRB5CCNAME");
> - printerr(2, "CHILD forked pid %d \n", childpid);
> - break;
> - case -1:
> - /* fork() failed! */
> - printerr(0, "WARNING: unable to fork() to handle"
> - "upcall: %s\n", strerror(errno));
> - return;
> - default:
> - /* Parent: just wait on child to exit and return */
> - do {
> - pid = wait(&err);
> - } while(pid == -1 && errno != -ECHILD);
> -
> - if (WIFSIGNALED(err))
> - printerr(0, "WARNING: forked child was killed"
> - "with signal %d\n", WTERMSIG(err));
> - return;
> - }
> -no_fork:
> -
> auth = krb5_not_machine_creds(clp, uid, tgtname, &downcall_err,
> &err, &rpc_clnt);
> if (err)
> @@ -739,8 +709,14 @@ void
> handle_krb5_upcall(struct clnt_info *clp)
> {
> uid_t uid;
> -
> - if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
> + int status;
> +
> + status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
> + pthread_mutex_lock(&pmutex);
> + thread_started = true;
> + pthread_cond_signal(&pcond);
> + pthread_mutex_unlock(&pmutex);
A second macro here like phone_home() 8-) That's a joke! :-)
> + if (status) {
> printerr(0, "WARNING: failed reading uid from krb5 "
> "upcall pipe: %s\n", strerror(errno));
> return;
> @@ -765,6 +741,10 @@ handle_gssd_upcall(struct clnt_info *clp)
> char *enctypes = NULL;
>
> lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
> + pthread_mutex_lock(&pmutex);
> + thread_started = true;
> + pthread_cond_signal(&pcond);
> + pthread_mutex_unlock(&pmutex);
The same second macro here as well...
Nice work!!
steved.
> if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
> printerr(0, "WARNING: handle_gssd_upcall: "
> "failed reading request\n");
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index 8ef8184..3328696 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -128,6 +128,7 @@
>
> /* Global list of principals/cache file names for machine credentials */
> struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
> +pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
>
> #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> int limit_to_legacy_enctypes = 0;
> @@ -586,10 +587,12 @@ get_ple_by_princ(krb5_context context, krb5_principal princ)
>
> /* Need to serialize list if we ever become multi-threaded! */
>
> + pthread_mutex_lock(&ple_lock);
> ple = find_ple_by_princ(context, princ);
> if (ple == NULL) {
> ple = new_ple(context, princ);
> }
> + pthread_mutex_unlock(&ple_lock);
>
> return ple;
> }
>
On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
> For the threaded version we have to set uid,gid per thread instead
> of per process. glibc setresuid() when called from a thread, it'll
> send a signal to all other threads to synchronize the uid in all
> other threads. To bypass this, we have to call syscall() directly.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> Reviewed-by: Steve Dickson <[email protected]>
> ---
> utils/gssd/gssd_proc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 581a125..5d9a6db 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -69,6 +69,7 @@
> #include <netdb.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> +#include <syscall.h>
>
> #include "gssd.h"
> #include "err_util.h"
> @@ -457,7 +458,12 @@ change_identity(uid_t uid)
> * Switch the GIDs. Note that we leave the saved-set-gid alone in an
> * attempt to prevent attacks via ptrace()
> */
> - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> + /* For the threaded version we have to set uid,gid per thread instead
> + * of per process. glibc setresuid() when called from a thread, it'll
> + * send a signal to all other threads to synchronize the uid in all
> + * other threads. To bypass this, we have to call syscall() directly.
> + */
> + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
> printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
> return errno;
> }
> @@ -466,7 +472,7 @@ change_identity(uid_t uid)
> * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
> * other processes running with this uid.
> */
> - if (setresuid(uid, uid, -1) != 0) {
> + if (syscall(SYS_setresuid, uid) != 0) {
> printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
> uid);
> return errno;
>
We also have to do the same thing to the setgroups() call at the
top of change_identity(). So add the following diff to this
patch and we are good to go...
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e651d71..2f9f8ab 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -437,7 +437,7 @@ change_identity(uid_t uid)
struct passwd *pw;
/* drop list of supplimentary groups first */
- if (setgroups(0, NULL) != 0) {
+ if (syscall(SYS_setgroups, 0) != 0) {
printerr(0, "WARNING: unable to drop supplimentary groups!");
return errno;
}
steved.
On Fri, Apr 22, 2016 at 2:38 PM, Steve Dickson <[email protected]> wrote:
>
>
> On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
>> For the threaded version we have to set uid,gid per thread instead
>> of per process. glibc setresuid() when called from a thread, it'll
>> send a signal to all other threads to synchronize the uid in all
>> other threads. To bypass this, we have to call syscall() directly.
>>
>> Signed-off-by: Olga Kornievskaia <[email protected]>
>> Reviewed-by: Steve Dickson <[email protected]>
>> ---
>> utils/gssd/gssd_proc.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>> index 581a125..5d9a6db 100644
>> --- a/utils/gssd/gssd_proc.c
>> +++ b/utils/gssd/gssd_proc.c
>> @@ -69,6 +69,7 @@
>> #include <netdb.h>
>> #include <sys/types.h>
>> #include <sys/wait.h>
>> +#include <syscall.h>
>>
>> #include "gssd.h"
>> #include "err_util.h"
>> @@ -457,7 +458,12 @@ change_identity(uid_t uid)
>> * Switch the GIDs. Note that we leave the saved-set-gid alone in an
>> * attempt to prevent attacks via ptrace()
>> */
>> - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
>> + /* For the threaded version we have to set uid,gid per thread instead
>> + * of per process. glibc setresuid() when called from a thread, it'll
>> + * send a signal to all other threads to synchronize the uid in all
>> + * other threads. To bypass this, we have to call syscall() directly.
>> + */
>> + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
>> printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
>> return errno;
>> }
>> @@ -466,7 +472,7 @@ change_identity(uid_t uid)
>> * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
>> * other processes running with this uid.
>> */
>> - if (setresuid(uid, uid, -1) != 0) {
>> + if (syscall(SYS_setresuid, uid) != 0) {
>> printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
>> uid);
>> return errno;
>>
> We also have to do the same thing to the setgroups() call at the
> top of change_identity(). So add the following diff to this
> patch and we are good to go...
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index e651d71..2f9f8ab 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -437,7 +437,7 @@ change_identity(uid_t uid)
> struct passwd *pw;
>
> /* drop list of supplimentary groups first */
> - if (setgroups(0, NULL) != 0) {
> + if (syscall(SYS_setgroups, 0) != 0) {
> printerr(0, "WARNING: unable to drop supplimentary groups!");
> return errno;
> }
>
Do you know why we do this before getting the passwd entry?
Doing as you suggest causes problems as in it it fails to drop the
supplementary groups.
> steved.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/22/2016 04:41 PM, Olga Kornievskaia wrote:
> On Fri, Apr 22, 2016 at 2:38 PM, Steve Dickson <[email protected]> wrote:
>>
>>
>> On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
>>> For the threaded version we have to set uid,gid per thread instead
>>> of per process. glibc setresuid() when called from a thread, it'll
>>> send a signal to all other threads to synchronize the uid in all
>>> other threads. To bypass this, we have to call syscall() directly.
>>>
>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>> Reviewed-by: Steve Dickson <[email protected]>
>>> ---
>>> utils/gssd/gssd_proc.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>>> index 581a125..5d9a6db 100644
>>> --- a/utils/gssd/gssd_proc.c
>>> +++ b/utils/gssd/gssd_proc.c
>>> @@ -69,6 +69,7 @@
>>> #include <netdb.h>
>>> #include <sys/types.h>
>>> #include <sys/wait.h>
>>> +#include <syscall.h>
>>>
>>> #include "gssd.h"
>>> #include "err_util.h"
>>> @@ -457,7 +458,12 @@ change_identity(uid_t uid)
>>> * Switch the GIDs. Note that we leave the saved-set-gid alone in an
>>> * attempt to prevent attacks via ptrace()
>>> */
>>> - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
>>> + /* For the threaded version we have to set uid,gid per thread instead
>>> + * of per process. glibc setresuid() when called from a thread, it'll
>>> + * send a signal to all other threads to synchronize the uid in all
>>> + * other threads. To bypass this, we have to call syscall() directly.
>>> + */
>>> + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
>>> printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
>>> return errno;
>>> }
>>> @@ -466,7 +472,7 @@ change_identity(uid_t uid)
>>> * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
>>> * other processes running with this uid.
>>> */
>>> - if (setresuid(uid, uid, -1) != 0) {
>>> + if (syscall(SYS_setresuid, uid) != 0) {
>>> printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
>>> uid);
>>> return errno;
>>>
>> We also have to do the same thing to the setgroups() call at the
>> top of change_identity(). So add the following diff to this
>> patch and we are good to go...
>>
>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>> index e651d71..2f9f8ab 100644
>> --- a/utils/gssd/gssd_proc.c
>> +++ b/utils/gssd/gssd_proc.c
>> @@ -437,7 +437,7 @@ change_identity(uid_t uid)
>> struct passwd *pw;
>>
>> /* drop list of supplimentary groups first */
>> - if (setgroups(0, NULL) != 0) {
>> + if (syscall(SYS_setgroups, 0) != 0) {
>> printerr(0, "WARNING: unable to drop supplimentary groups!");
>> return errno;
>> }
>>
>
> Do you know why we do this before getting the passwd entry?
It came in with commit 6b53fc9c which changed the way gssd
changed the uid/gid from Jeff Layton...
Jeff, what was the thought behind dropping supplementary groups?
I believe it was some type of security thing.
>
> Doing as you suggest causes problems as in it it fails to drop the
> supplementary groups.
hmm... what are you seeing that I'm not?
steved.
On Sat, 2016-04-23 at 17:14 -0400, Steve Dickson wrote:
>
> On 04/22/2016 04:41 PM, Olga Kornievskaia wrote:
> >
> > On Fri, Apr 22, 2016 at 2:38 PM, Steve Dickson <[email protected]>
> > wrote:
> > >
> > >
> > >
> > > On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
> > > >
> > > > For the threaded version we have to set uid,gid per thread
> > > > instead
> > > > of per process. glibc setresuid() when called from a thread,
> > > > it'll
> > > > send a signal to all other threads to synchronize the uid in
> > > > all
> > > > other threads. To bypass this, we have to call syscall()
> > > > directly.
> > > >
> > > > Signed-off-by: Olga Kornievskaia <[email protected]>
> > > > Reviewed-by: Steve Dickson <[email protected]>
> > > > ---
> > > > utils/gssd/gssd_proc.c | 10 ++++++++--
> > > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > > index 581a125..5d9a6db 100644
> > > > --- a/utils/gssd/gssd_proc.c
> > > > +++ b/utils/gssd/gssd_proc.c
> > > > @@ -69,6 +69,7 @@
> > > > #include <netdb.h>
> > > > #include <sys/types.h>
> > > > #include <sys/wait.h>
> > > > +#include <syscall.h>
> > > >
> > > > #include "gssd.h"
> > > > #include "err_util.h"
> > > > @@ -457,7 +458,12 @@ change_identity(uid_t uid)
> > > > * Switch the GIDs. Note that we leave the saved-set-gid
> > > > alone in an
> > > > * attempt to prevent attacks via ptrace()
> > > > */
> > > > - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> > > > + /* For the threaded version we have to set uid,gid per
> > > > thread instead
> > > > + * of per process. glibc setresuid() when called from a
> > > > thread, it'll
> > > > + * send a signal to all other threads to synchronize the
> > > > uid in all
> > > > + * other threads. To bypass this, we have to call
> > > > syscall() directly.
> > > > + */
> > > > + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
> > > > printerr(0, "WARNING: failed to set gid to
> > > > %u!\n", pw->pw_gid);
> > > > return errno;
> > > > }
> > > > @@ -466,7 +472,7 @@ change_identity(uid_t uid)
> > > > * Switch UIDs, but leave saved-set-uid alone to prevent
> > > > ptrace() by
> > > > * other processes running with this uid.
> > > > */
> > > > - if (setresuid(uid, uid, -1) != 0) {
> > > > + if (syscall(SYS_setresuid, uid) != 0) {
> > > > printerr(0, "WARNING: Failed to setuid for user
> > > > with uid %u\n",
> > > > uid);
> > > > return errno;
> > > >
> > > We also have to do the same thing to the setgroups() call at the
> > > top of change_identity(). So add the following diff to this
> > > patch and we are good to go...
> > >
> > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > index e651d71..2f9f8ab 100644
> > > --- a/utils/gssd/gssd_proc.c
> > > +++ b/utils/gssd/gssd_proc.c
> > > @@ -437,7 +437,7 @@ change_identity(uid_t uid)
> > > struct passwd *pw;
> > >
> > > /* drop list of supplimentary groups first */
> > > - if (setgroups(0, NULL) != 0) {
> > > + if (syscall(SYS_setgroups, 0) != 0) {
> > > printerr(0, "WARNING: unable to drop
> > > supplimentary groups!");
> > > return errno;
> > > }
> > >
> > Do you know why we do this before getting the passwd entry?
> It came in with commit 6b53fc9c which changed the way gssd
> changed the uid/gid from Jeff Layton...
>
> Jeff, what was the thought behind dropping supplementary groups?
> I believe it was some type of security thing.
>
We do that so that the child doesn't run with supplementary groups from
the main process.
As to why we do that before getting the passwd entry instead of after?
No real reason for the order that I can recall. If you need to do it
after the pw entry, I don't think it'll matter.
That said, since this has to run as root anyway, I guess you could drop
the supplementary groups in the main process. Then all of the children
should get a zeroed out grouplist, and it's one less syscall...
> >
> >
> > Doing as you suggest causes problems as in it it fails to drop the
> > supplementary groups.
> hmm... what are you seeing that I'm not?
>
> steved.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
On Sat, Apr 23, 2016 at 11:24 PM, Jeff Layton <[email protected]> wrote:
> On Sat, 2016-04-23 at 17:14 -0400, Steve Dickson wrote:
>>
>> On 04/22/2016 04:41 PM, Olga Kornievskaia wrote:
>> >
>> > On Fri, Apr 22, 2016 at 2:38 PM, Steve Dickson <[email protected]>
>> > wrote:
>> > >
>> > >
>> > >
>> > > On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
>> > > >
>> > > > For the threaded version we have to set uid,gid per thread
>> > > > instead
>> > > > of per process. glibc setresuid() when called from a thread,
>> > > > it'll
>> > > > send a signal to all other threads to synchronize the uid in
>> > > > all
>> > > > other threads. To bypass this, we have to call syscall()
>> > > > directly.
>> > > >
>> > > > Signed-off-by: Olga Kornievskaia <[email protected]>
>> > > > Reviewed-by: Steve Dickson <[email protected]>
>> > > > ---
>> > > > utils/gssd/gssd_proc.c | 10 ++++++++--
>> > > > 1 file changed, 8 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>> > > > index 581a125..5d9a6db 100644
>> > > > --- a/utils/gssd/gssd_proc.c
>> > > > +++ b/utils/gssd/gssd_proc.c
>> > > > @@ -69,6 +69,7 @@
>> > > > #include <netdb.h>
>> > > > #include <sys/types.h>
>> > > > #include <sys/wait.h>
>> > > > +#include <syscall.h>
>> > > >
>> > > > #include "gssd.h"
>> > > > #include "err_util.h"
>> > > > @@ -457,7 +458,12 @@ change_identity(uid_t uid)
>> > > > * Switch the GIDs. Note that we leave the saved-set-gid
>> > > > alone in an
>> > > > * attempt to prevent attacks via ptrace()
>> > > > */
>> > > > - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
>> > > > + /* For the threaded version we have to set uid,gid per
>> > > > thread instead
>> > > > + * of per process. glibc setresuid() when called from a
>> > > > thread, it'll
>> > > > + * send a signal to all other threads to synchronize the
>> > > > uid in all
>> > > > + * other threads. To bypass this, we have to call
>> > > > syscall() directly.
>> > > > + */
>> > > > + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
>> > > > printerr(0, "WARNING: failed to set gid to
>> > > > %u!\n", pw->pw_gid);
>> > > > return errno;
>> > > > }
>> > > > @@ -466,7 +472,7 @@ change_identity(uid_t uid)
>> > > > * Switch UIDs, but leave saved-set-uid alone to prevent
>> > > > ptrace() by
>> > > > * other processes running with this uid.
>> > > > */
>> > > > - if (setresuid(uid, uid, -1) != 0) {
>> > > > + if (syscall(SYS_setresuid, uid) != 0) {
>> > > > printerr(0, "WARNING: Failed to setuid for user
>> > > > with uid %u\n",
>> > > > uid);
>> > > > return errno;
>> > > >
>> > > We also have to do the same thing to the setgroups() call at the
>> > > top of change_identity(). So add the following diff to this
>> > > patch and we are good to go...
>> > >
>> > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>> > > index e651d71..2f9f8ab 100644
>> > > --- a/utils/gssd/gssd_proc.c
>> > > +++ b/utils/gssd/gssd_proc.c
>> > > @@ -437,7 +437,7 @@ change_identity(uid_t uid)
>> > > struct passwd *pw;
>> > >
>> > > /* drop list of supplimentary groups first */
>> > > - if (setgroups(0, NULL) != 0) {
>> > > + if (syscall(SYS_setgroups, 0) != 0) {
>> > > printerr(0, "WARNING: unable to drop
>> > > supplimentary groups!");
>> > > return errno;
>> > > }
>> > >
>> > Do you know why we do this before getting the passwd entry?
>> It came in with commit 6b53fc9c which changed the way gssd
>> changed the uid/gid from Jeff Layton...
>>
>> Jeff, what was the thought behind dropping supplementary groups?
>> I believe it was some type of security thing.
>>
>
> We do that so that the child doesn't run with supplementary groups from
> the main process.
>
> As to why we do that before getting the passwd entry instead of after?
> No real reason for the order that I can recall. If you need to do it
> after the pw entry, I don't think it'll matter.
>
> That said, since this has to run as root anyway, I guess you could drop
> the supplementary groups in the main process. Then all of the children
> should get a zeroed out grouplist, and it's one less syscall...
Thank Jeff, I thought that since we are setting the effective uid/gid
of the process (or with the patches, thread), then the child process
(thread) would have the supplementary groups associated with the given
uid.
>> > Doing as you suggest causes problems as in it it fails to drop the
>> > supplementary groups.
>> hmm... what are you seeing that I'm not?
It was my bad. I decided to change the order of when the call is done
(ie., doing it after the setting the uid and gid) and that causes an
error. Perhaps it's becuase once the uid is changed to some user's
uid, then that uid no longer has the ability to drop the supplementary
groups.
Going to send a new version of patches...
>>
>> steved.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
>> in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Jeff Layton <[email protected]>
>
On Mon, 2016-04-25 at 12:56 -0400, Olga Kornievskaia wrote:
> On Sat, Apr 23, 2016 at 11:24 PM, Jeff Layton wrote:
> >
> > On Sat, 2016-04-23 at 17:14 -0400, Steve Dickson wrote:
> > >
> > >
> > > On 04/22/2016 04:41 PM, Olga Kornievskaia wrote:
> > > >
> > > >
> > > > On Fri, Apr 22, 2016 at 2:38 PM, Steve Dickson
> > > > wrote:
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On 04/20/2016 05:12 PM, Olga Kornievskaia wrote:
> > > > > >
> > > > > >
> > > > > > For the threaded version we have to set uid,gid per thread
> > > > > > instead
> > > > > > of per process. glibc setresuid() when called from a thread,
> > > > > > it'll
> > > > > > send a signal to all other threads to synchronize the uid in
> > > > > > all
> > > > > > other threads. To bypass this, we have to call syscall()
> > > > > > directly.
> > > > > >
> > > > > > Signed-off-by: Olga Kornievskaia <[email protected]>
> > > > > > Reviewed-by: Steve Dickson <[email protected]>
> > > > > > ---
> > > > > > utils/gssd/gssd_proc.c | 10 ++++++++--
> > > > > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > > > > index 581a125..5d9a6db 100644
> > > > > > --- a/utils/gssd/gssd_proc.c
> > > > > > +++ b/utils/gssd/gssd_proc.c
> > > > > > @@ -69,6 +69,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > >
> > > > > > #include "gssd.h"
> > > > > > #include "err_util.h"
> > > > > > @@ -457,7 +458,12 @@ change_identity(uid_t uid)
> > > > > > * Switch the GIDs. Note that we leave the saved-set-gid
> > > > > > alone in an
> > > > > > * attempt to prevent attacks via ptrace()
> > > > > > */
> > > > > > - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> > > > > > + /* For the threaded version we have to set uid,gid per
> > > > > > thread instead
> > > > > > + * of per process. glibc setresuid() when called from a
> > > > > > thread, it'll
> > > > > > + * send a signal to all other threads to synchronize the
> > > > > > uid in all
> > > > > > + * other threads. To bypass this, we have to call
> > > > > > syscall() directly.
> > > > > > + */
> > > > > > + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
> > > > > > printerr(0, "WARNING: failed to set gid to
> > > > > > %u!\n", pw->pw_gid);
> > > > > > return errno;
> > > > > > }
> > > > > > @@ -466,7 +472,7 @@ change_identity(uid_t uid)
> > > > > > * Switch UIDs, but leave saved-set-uid alone to prevent
> > > > > > ptrace() by
> > > > > > * other processes running with this uid.
> > > > > > */
> > > > > > - if (setresuid(uid, uid, -1) != 0) {
> > > > > > + if (syscall(SYS_setresuid, uid) != 0) {
> > > > > > printerr(0, "WARNING: Failed to setuid for user
> > > > > > with uid %u\n",
> > > > > > uid);
> > > > > > return errno;
> > > > > >
> > > > > We also have to do the same thing to the setgroups() call at the
> > > > > top of change_identity(). So add the following diff to this
> > > > > patch and we are good to go...
> > > > >
> > > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > > > index e651d71..2f9f8ab 100644
> > > > > --- a/utils/gssd/gssd_proc.c
> > > > > +++ b/utils/gssd/gssd_proc.c
> > > > > @@ -437,7 +437,7 @@ change_identity(uid_t uid)
> > > > > struct passwd *pw;
> > > > >
> > > > > /* drop list of supplimentary groups first */
> > > > > - if (setgroups(0, NULL) != 0) {
> > > > > + if (syscall(SYS_setgroups, 0) != 0) {
> > > > > printerr(0, "WARNING: unable to drop
> > > > > supplimentary groups!");
> > > > > return errno;
> > > > > }
> > > > >
> > > > Do you know why we do this before getting the passwd entry?
> > > It came in with commit 6b53fc9c which changed the way gssd
> > > changed the uid/gid from Jeff Layton...
> > >
> > > Jeff, what was the thought behind dropping supplementary groups?
> > > I believe it was some type of security thing.
> > >
> > We do that so that the child doesn't run with supplementary groups from
> > the main process.
> >
> > As to why we do that before getting the passwd entry instead of after?
> > No real reason for the order that I can recall. If you need to do it
> > after the pw entry, I don't think it'll matter.
> >
> > That said, since this has to run as root anyway, I guess you could drop
> > the supplementary groups in the main process. Then all of the children
> > should get a zeroed out grouplist, and it's one less syscall...
> Thank Jeff, I thought that since we are setting the effective uid/gid
> of the process (or with the patches, thread), then the child process
> (thread) would have the supplementary groups associated with the given
> uid.
>
No. Calling seteuid should have no effect on the supplementary groups
list, AFAIK. Oh, and yeah...the manpage says:
setgroups() sets the supplementary group IDs for the calling process.
Appropriate privileges (Linux: the CAP_SETGID capability) are required.
So you do need to clean out the groupslist and gids first and only
afterward set the uids.
> > >
> > > >
> > > > Doing as you suggest causes problems as in it it fails to drop the
> > > > supplementary groups.
> > > hmm... what are you seeing that I'm not?
> It was my bad. I decided to change the order of when the call is done
> (ie., doing it after the setting the uid and gid) and that causes an
> error. Perhaps it's becuase once the uid is changed to some user's
> uid, then that uid no longer has the ability to drop the supplementary
> groups.
>
> Going to send a new version of patches...
>
> >
> > >
> > >
> > > steved.
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> > > in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > Jeff Layton <[email protected]>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>