samba - Feb 2017 - [PATCH v2 0/2] cifs.upcall: allow cifs.upcall to grab $KRB5CCNAME from initiating process

Small respin of the patches that I posted a few days ago. The main
difference is the reordering of the series to make it do the group
and grouplist manipulation first, and then the patch that makes
it grab the KRB5CCNAME from the initiating process.

I think the code is sound, my main question is whether we really
need the command-line switch for this. Should this just be the
default mode of operation?

Jeff Layton (2):
  cifs.upcall: switch group IDs when handling an upcall
  cifs.upcall: allow scraping of KRB5CCNAME out of initiating task's
    /proc/<pid>/environ file

 cifs.upcall.8.in |  10 +++
 cifs.upcall.c    | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 189 insertions(+), 6 deletions(-)

-- 
2.9.3

Currently, we leave the group ID alone, but in a later patch we'll be
changing cifs.upcall to scrape $KRB5CCNAME out of the originating
process. At that point, we want to be a little more careful with the
process credentials we'll be using.

After we get the uid, do a getpwuid and grab the default gid for the
user. Then use setgid to set it before calling setuid.

Signed-off-by: Jeff Layton <jlayton at samba.org>
---
 cifs.upcall.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/cifs.upcall.c b/cifs.upcall.c
index 8f146c92b4a5..122b6a7a324a 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -46,6 +46,8 @@
 #include <netdb.h>
 #include <arpa/inet.h>
 #include <ctype.h>
+#include <pwd.h>
+#include <grp.h>
 
 #include "replace.h"
 #include "data_blob.h"
@@ -693,6 +695,7 @@ int main(const int argc, char *const argv[])
 	uid_t uid;
 	char *keytab_name = NULL;
 	krb5_ccache ccache = NULL;
+	struct passwd *pw;
 
 	hostbuf[0] = '\0';
 	memset(&arg, 0, sizeof(arg));
@@ -794,15 +797,49 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
+	/*
+	 * The kernel doesn't pass down the gid, so we resort here to scraping
+	 * one out of the passwd nss db. Note that this might not reflect the
+	 * actual gid of the process that initiated the upcall. While we could
+	 * scrape that out of /proc, relying on that is a bit more risky.
+	 */
+	pw = getpwuid(uid);
+	if (!pw) {
+		syslog(LOG_ERR, "Unable to find pw entry for uid %d: %s\n",
+			uid, strerror(errno));
+		rc = 1;
+		goto out;
+	}
+
+	/*
+	 * The kernel should send down a zero-length grouplist already, but
+	 * just to be on the safe side...
+	 */
+	rc = setgroups(0, NULL);
+	if (rc == -1) {
+		syslog(LOG_ERR, "setgroups: %s", strerror(errno));
+		rc = 1;
+		goto out;
+	}
+
+	rc = setgid(pw->pw_gid);
+	if (rc == -1) {
+		syslog(LOG_ERR, "setgid: %s", strerror(errno));
+		rc = 1;
+		goto out;
+	}
+
 	rc = setuid(uid);
 	if (rc == -1) {
 		syslog(LOG_ERR, "setuid: %s", strerror(errno));
+		rc = 1;
 		goto out;
 	}
 
 	rc = krb5_init_context(&context);
 	if (rc) {
 		syslog(LOG_ERR, "unable to init krb5 context: %ld", rc);
+		rc = 1;
 		goto out;
 	}
 
-- 
2.9.3

Chad reported that he was seeing a regression in cifs-utils-6.6. Prior
to that, cifs.upcall was able to find credcaches in non-default FILE:
locations, but with the rework of that code, that ability was lost.

Unfortunately, the krb5 library design doesn't really take into account
the fact that we might need to find a credcache in a process that isn't
descended from the session.

When the kernel does an upcall, it passes several bits of info about the
task that initiated the upcall. One of those things is the PID (the
tgid, in particular). We can use that info to reach into the
/proc/<pid>/environ file for the process, and grab whatever value of
KRB5CCNAME is there.  This patch adds this ability to cifs.upcall.

I'm not 100% convinced that this is a good idea however, so for now,
this is disabled unless the command line has a '-e' switch. Anyone
wishing to play with this should edit their /etc/request-key.conf files
accordingly.

Reported-by: Chad William Seys <cwseys at physics.wisc.edu>
Signed-off-by: Jeff Layton <jlayton at samba.org>
---
 cifs.upcall.8.in |  10 ++++
 cifs.upcall.c    | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 152 insertions(+), 6 deletions(-)

diff --git a/cifs.upcall.8.in b/cifs.upcall.8.in
index 50f79d18331d..06c6d3a02c2f 100644
--- a/cifs.upcall.8.in
+++ b/cifs.upcall.8.in
@@ -38,6 +38,16 @@ for a particular key type\&. While it can be run directly
from the command\-line
 This option is deprecated and is currently ignored\&.
 .RE
 .PP
+\-\-env-probe|\-e
+.RS 4
+Allow cifs.upcall to do an environment probe to help find the credential
+cache. This can assist the program with finding credential caches in
+non-default locations. If this is set, then the program will attempt to
+retrieve the KRB5CCNAME environment variable from the process that initiated
+the upcall, and then set that in the upcall process for use when searching for
+a credcache.
+.RE
+.PP
 \--krb5conf=/path/to/krb5.conf|-k /path/to/krb5.conf
 .RS 4
 This option allows administrators to set an alternate location for the
diff --git a/cifs.upcall.c b/cifs.upcall.c
index 122b6a7a324a..cc9ecf0d1fed 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -40,6 +40,7 @@
 #include <dirent.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <fcntl.h>
 #include <unistd.h>
 #include <keyutils.h>
 #include <time.h>
@@ -156,11 +157,125 @@ err_cache:
 	return credtime;
 }
 
+#define	ENV_PATH_FMT			"/proc/%d/environ"
+#define	ENV_PATH_MAXLEN			(6 + 10 + 8 + 1)
+
+#define	ENV_NAME			"KRB5CCNAME"
+#define	ENV_PREFIX			"KRB5CCNAME="
+#define	ENV_PREFIX_LEN			11
+
+#define	ENV_BUF_START			(4096)
+#define	ENV_BUF_MAX			(131072)
+
+/**
+ * get_cachename_from_process_env - scrape value of $KRB5CCNAME out of the
+ * 				    initiating process' environment.
+ * @pid: initiating pid value from the upcall string
+ *
+ * Open the /proc/<pid>/environ file for the given pid, and scrape it for
+ * KRB5CCNAME entries.
+ *
+ * We start with a page-size buffer, and then progressively double it until
+ * we can slurp in the whole thing.
+ *
+ * Note that this is not entirely reliable. If the process is sitting in a
+ * container or something, then this is almost certainly not going to point
+ * where you expect.
+ *
+ * Probably it just won't work, but could a user use this to trick
cifs.upcall
+ * into reading a file outside the container, by setting KRB5CCNAME in a
+ * crafty way?
+ */
+static char *
+get_cachename_from_process_env(pid_t pid)
+{
+	int fd, ret;
+	ssize_t buflen;
+	ssize_t bufsize = ENV_BUF_START;
+	char pathname[ENV_PATH_MAXLEN];
+	char *cachename = NULL;
+	char *buf = NULL, *pos;
+
+	if (!pid) {
+		syslog(LOG_DEBUG, "%s: pid == 0\n", __func__);
+		return NULL;
+	}
+
+	pathname[ENV_PATH_MAXLEN - 1] = '\0';
+	ret = snprintf(pathname, ENV_PATH_MAXLEN, ENV_PATH_FMT, pid);
+	if (ret >= ENV_PATH_MAXLEN) {
+		syslog(LOG_DEBUG, "%s: unterminated path!\n", __func__);
+		return NULL;
+	}
+
+	syslog(LOG_DEBUG, "%s: pathname=%s\n", __func__, pathname);
+	fd = open(pathname, O_RDONLY);
+	if (fd < 0) {
+		syslog(LOG_DEBUG, "%s: open failed: %d\n", __func__, errno);
+		return NULL;
+	}
+retry:
+	if (bufsize > ENV_BUF_MAX) {
+		syslog(LOG_DEBUG, "%s: buffer too big: %zd\n",
+							__func__, bufsize);
+		goto out_close;
+	}
+
+	buf = malloc(bufsize);
+	if (!buf) {
+		syslog(LOG_DEBUG, "%s: malloc failure\n", __func__);
+		goto out_close;
+	}
+
+	buflen = read(fd, buf, bufsize);
+	if (buflen < 0) {
+		syslog(LOG_DEBUG, "%s: read failed: %d\n", __func__, errno);
+		goto out_close;
+	}
+
+	if (buflen >= bufsize) {
+		/* We read to the end of the buffer. Double and try again */
+		syslog(LOG_DEBUG, "%s: read to end of buffer (%zu bytes)\n",
+					__func__, bufsize);
+		free(buf);
+		bufsize *= 2;
+		if (lseek(fd, 0, SEEK_SET) < 0)
+			goto out_close;
+		goto retry;
+	}
+
+	pos = buf;
+	while (buflen > 0) {
+		size_t len = strnlen(pos, buflen);
+
+		if (len > ENV_PREFIX_LEN &&
+		    !memcmp(pos, ENV_PREFIX, ENV_PREFIX_LEN)) {
+			cachename = strndup(pos + ENV_PREFIX_LEN,
+							len - ENV_PREFIX_LEN);
+			syslog(LOG_DEBUG, "%s: cachename = %s\n",
+							__func__, cachename);
+			break;
+		}
+		buflen -= (len + 1);
+		pos += (len + 1);
+	}
+out_close:
+	free(buf);
+	close(fd);
+	return cachename;
+}
+
 static krb5_ccache
-get_default_cc(void)
+get_existing_cc(const char *env_cachename)
 {
 	krb5_error_code ret;
 	krb5_ccache cc;
+	char *cachename;
+
+	if (env_cachename) {
+		if (setenv(ENV_NAME, env_cachename, 1))
+			syslog(LOG_DEBUG, "%s: failed to setenv %d\n", __func__, errno);
+	}
 
 	ret = krb5_cc_default(context, &cc);
 	if (ret) {
@@ -168,6 +283,14 @@ get_default_cc(void)
 		return NULL;
 	}
 
+	ret = krb5_cc_get_full_name(context, cc, &cachename);
+	if (ret) {
+		syslog(LOG_DEBUG, "%s: krb5_cc_get_full_name failed: %d\n",
__func__, ret);
+	} else {
+		syslog(LOG_DEBUG, "%s: default ccache is %s\n", __func__,
cachename);
+		krb5_free_string(context, cachename);
+	}
+
 	if (!get_tgt_time(cc)) {
 		krb5_cc_close(context, cc);
 		cc = NULL;
@@ -175,7 +298,6 @@ get_default_cc(void)
 	return cc;
 }
 
-
 static krb5_ccache
 init_cc_from_keytab(const char *keytab_name, const char *user)
 {
@@ -666,10 +788,11 @@ lowercase_string(char *c)
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: %s [ -K /path/to/keytab] [-k /path/to/krb5.conf]
[-t] [-v] [-l] key_serial\n", prog);
+	fprintf(stderr, "Usage: %s [ -K /path/to/keytab] [-k /path/to/krb5.conf]
[-e] [-t] [-v] [-l] key_serial\n", prog);
 }
 
 static const struct option long_options[] = {
+	{"env-probe", 0, NULL, 'e'},
 	{"krb5conf", 1, NULL, 'k'},
 	{"legacy-uid", 0, NULL, 'l'},
 	{"trust-dns", 0, NULL, 't'},
@@ -687,13 +810,14 @@ int main(const int argc, char *const argv[])
 	size_t datalen;
 	unsigned int have;
 	long rc = 1;
-	int c, try_dns = 0, legacy_uid = 0;
+	int c, try_dns = 0, legacy_uid = 0, env_probe = 0;
 	char *buf;
 	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg;
 	const char *oid;
 	uid_t uid;
 	char *keytab_name = NULL;
+	char *env_cachename = NULL;
 	krb5_ccache ccache = NULL;
 	struct passwd *pw;
 
@@ -702,11 +826,15 @@ int main(const int argc, char *const argv[])
 
 	openlog(prog, 0, LOG_DAEMON);
 
-	while ((c = getopt_long(argc, argv, "ck:K:ltv", long_options, NULL))
!= -1) {
+	while ((c = getopt_long(argc, argv, "cek:K:ltv", long_options,
NULL)) != -1) {
 		switch (c) {
 		case 'c':
 			/* legacy option -- skip it */
 			break;
+		case 'e':
+			/* do an environmental probe to get the credcache */
+			env_probe++;
+			break;
 		case 't':
 			try_dns++;
 			break;
@@ -829,6 +957,13 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
+	/*
+	 * Must do this before setuid, as we need elevated capabilities to
+	 * look at the environ file.
+	 */
+	env_cachename +		get_cachename_from_process_env(env_probe ?  arg.pid : 0);
+
 	rc = setuid(uid);
 	if (rc == -1) {
 		syslog(LOG_ERR, "setuid: %s", strerror(errno));
@@ -843,7 +978,7 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
-	ccache = get_default_cc();
+	ccache = get_existing_cc(env_cachename);
 	/* Couldn't find credcache? Try to use keytab */
 	if (ccache == NULL && arg.username != NULL)
 		ccache = init_cc_from_keytab(keytab_name, arg.username);
@@ -996,6 +1131,7 @@ out:
 	SAFE_FREE(arg.ip);
 	SAFE_FREE(arg.username);
 	SAFE_FREE(keydata);
+	SAFE_FREE(env_cachename);
 	syslog(LOG_DEBUG, "Exit status %ld", rc);
 	return rc;
 }
-- 
2.9.3

On Tue, 2017-02-14 at 13:23 -0500, Jeff Layton wrote:
> Small respin of the patches that I posted a few days ago. The main
> difference is the reordering of the series to make it do the group
> and grouplist manipulation first, and then the patch that makes
> it grab the KRB5CCNAME from the initiating process.
> 
> I think the code is sound, my main question is whether we really
> need the command-line switch for this. Should this just be the
> default mode of operation?
> 
> Jeff Layton (2):
>   cifs.upcall: switch group IDs when handling an upcall
>   cifs.upcall: allow scraping of KRB5CCNAME out of initiating task's
>     /proc/<pid>/environ file
> 
>  cifs.upcall.8.in |  10 +++
>  cifs.upcall.c    | 185
> +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 189 insertions(+), 6 deletions(-)
I think I'd make it the default, but maybe leave a way to disable env
scraping with a reverse flag.

Simo.