| Message ID | 20170211134149.8135-1-jlayton@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-02-11 at 08:41 -0500, Jeff Layton wrote: > 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. > Meant to put a Reported-by: here for Chad. I'll do that before merging. > Signed-off-by: Jeff Layton <jlayton@samba.org> > --- > cifs.upcall.8.in | 10 ++++ > cifs.upcall.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 152 insertions(+), 6 deletions(-) > FWIW, this patch works as expected in cursory, light testing. This should be superior to the earlier method where we scanned in /tmp as well -- more efficient, and also able to find non-FILE: credcaches, as well as ones that lie in other places besides /tmp. My main concern is that we have to do the environ file scraping while privileged, and that we end up trusting some info from a userland process that triggered the upcall in KRB5CCNAME. Does this open any potential attack vectors that I'm not considering? 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 8f146c92b4a5..1b2c6c19e39e 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> > @@ -154,11 +155,126 @@ 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? > + * > + * YMMV here. > + */ > +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: %d\n", __func__, errno); > + 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); > + } > + free(buf); > +out_close: > + 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) { > @@ -166,6 +282,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; > @@ -173,7 +297,6 @@ get_default_cc(void) > return cc; > } > > - > static krb5_ccache > init_cc_from_keytab(const char *keytab_name, const char *user) > { > @@ -664,10 +787,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'}, > @@ -685,13 +809,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; > > hostbuf[0] = '\0'; > @@ -699,11 +824,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; > @@ -794,6 +923,12 @@ int main(const int argc, char *const argv[]) > goto out; > } > > + /* > + * Must do this before setuid, as we need ptrace perms to look at > + * 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)); > @@ -806,7 +941,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); > @@ -959,6 +1094,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; > }
On Sat, 2017-02-11 at 10:16 -0500, Jeff Layton wrote: > On Sat, 2017-02-11 at 08:41 -0500, Jeff Layton wrote: > > 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. > > > > Meant to put a Reported-by: here for Chad. I'll do that before > merging. > > > Signed-off-by: Jeff Layton <jlayton@samba.org> > > --- > > cifs.upcall.8.in | 10 ++++ > > cifs.upcall.c | 148 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 152 insertions(+), 6 deletions(-) > > > > FWIW, this patch works as expected in cursory, light testing. This > should be superior to the earlier method where we scanned in /tmp as > well -- more efficient, and also able to find non-FILE: credcaches, > as > well as ones that lie in other places besides /tmp. > > My main concern is that we have to do the environ file scraping while > privileged, and that we end up trusting some info from a userland > process that triggered the upcall in KRB5CCNAME. Does this open any > potential attack vectors that I'm not considering? Well you are operating as the user however I have not seen a setgid operation, does this mean cifs.upcall is running with group id 0 ? This may give access to files the original user should not be able to access or cause files to be created with improper permission. Another thing people may need to pay attention to is SELinux context, I am not sure what selinux context cifs.upcall runs into the difference may matter in some cases, but in those I think proper SELinux policy should be devised, I see no need to change how cifs.upcall operates. Simo. > 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 8f146c92b4a5..1b2c6c19e39e 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> > > @@ -154,11 +155,126 @@ err_cache: > > return credtime; > > } > > > > +#define ENV_PATH_FMT "/proc/%d/envir > > on" > > +#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? > > + * > > + * YMMV here. > > + */ > > +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: %d\n", > > __func__, errno); > > + 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); > > + } > > + free(buf); > > +out_close: > > + 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) { > > @@ -166,6 +282,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; > > @@ -173,7 +297,6 @@ get_default_cc(void) > > return cc; > > } > > > > - > > static krb5_ccache > > init_cc_from_keytab(const char *keytab_name, const char *user) > > { > > @@ -664,10 +787,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'}, > > @@ -685,13 +809,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; > > > > hostbuf[0] = '\0'; > > @@ -699,11 +824,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; > > @@ -794,6 +923,12 @@ int main(const int argc, char *const argv[]) > > goto out; > > } > > > > + /* > > + * Must do this before setuid, as we need ptrace perms to > > look at > > + * 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)); > > @@ -806,7 +941,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); > > @@ -959,6 +1094,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; > > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-02-13 at 05:02 -0500, Simo Sorce wrote: > On Sat, 2017-02-11 at 10:16 -0500, Jeff Layton wrote: > > On Sat, 2017-02-11 at 08:41 -0500, Jeff Layton wrote: > > > 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. > > > > > > > Meant to put a Reported-by: here for Chad. I'll do that before > > merging. > > > > > > > > Signed-off-by: Jeff Layton <jlayton@samba.org> > > > --- > > > cifs.upcall.8.in | 10 ++++ > > > cifs.upcall.c | 148 > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > > 2 files changed, 152 insertions(+), 6 deletions(-) > > > > > > > FWIW, this patch works as expected in cursory, light testing. This > > should be superior to the earlier method where we scanned in /tmp as > > well -- more efficient, and also able to find non-FILE: credcaches, > > as > > well as ones that lie in other places besides /tmp. > > > > My main concern is that we have to do the environ file scraping while > > privileged, and that we end up trusting some info from a userland > > process that triggered the upcall in KRB5CCNAME. Does this open any > > potential attack vectors that I'm not considering? > > Well you are operating as the user however I have not seen a setgid > operation, does this mean cifs.upcall is running with group id 0 ? > > This may give access to files the original user should not be able to > access or cause files to be created with improper permission. > Yeah, ick. The kernel doesn't send down the gid, so we can't just add that in directly... We could do a getpwuid(uid), and then setgid(pw_gid). That might be problematic for applications that have done a setgid(), but those ought to be very rare. > Another thing people may need to pay attention to is SELinux context, I > am not sure what selinux context cifs.upcall runs into the difference > may matter in some cases, but in those I think proper SELinux policy > should be devised, I see no need to change how cifs.upcall operates. > Yeah, that's a different matter. It would be nice to confine cifs.upcall, but we'd need an SELinux policy for that. Honestly, I'd rather see us move to gssproxy before spending time on that.
On Mon, 2017-02-13 at 07:52 -0500, Jeff Layton wrote: > On Mon, 2017-02-13 at 05:02 -0500, Simo Sorce wrote: > > On Sat, 2017-02-11 at 10:16 -0500, Jeff Layton wrote: > > > On Sat, 2017-02-11 at 08:41 -0500, Jeff Layton wrote: > > > > 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. > > > > > > > > > > Meant to put a Reported-by: here for Chad. I'll do that before > > > merging. > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@samba.org> > > > > > > > > --- > > > > cifs.upcall.8.in | 10 ++++ > > > > cifs.upcall.c | 148 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > 2 files changed, 152 insertions(+), 6 deletions(-) > > > > > > > > > > FWIW, this patch works as expected in cursory, light testing. > > > This > > > should be superior to the earlier method where we scanned in /tmp > > > as > > > well -- more efficient, and also able to find non-FILE: > > > credcaches, > > > as > > > well as ones that lie in other places besides /tmp. > > > > > > My main concern is that we have to do the environ file scraping > > > while > > > privileged, and that we end up trusting some info from a userland > > > process that triggered the upcall in KRB5CCNAME. Does this open > > > any > > > potential attack vectors that I'm not considering? > > > > Well you are operating as the user however I have not seen a setgid > > operation, does this mean cifs.upcall is running with group id 0 ? > > > > This may give access to files the original user should not be able > > to > > access or cause files to be created with improper permission. > > > > Yeah, ick. The kernel doesn't send down the gid, so we can't just add > that in directly... > > We could do a getpwuid(uid), and then setgid(pw_gid). That might be > problematic for applications that have done a setgid(), but those > ought to be very rare. For the CIFS/NFS use case I think this is good enough, if you did setgid and the credential file can only be accessed with that gid... well tough luck, I mean it must be *exceedingly* rare. For tht matter we do not have either all the secondary gids, and the ccache may be (only) readable by one of them too ... > > Another thing people may need to pay attention to is SELinux > > context, I > > am not sure what selinux context cifs.upcall runs into the > > difference > > may matter in some cases, but in those I think proper SELinux > > policy > > should be devised, I see no need to change how cifs.upcall > > operates. > > > > Yeah, that's a different matter. It would be nice to confine > cifs.upcall, but we'd need an SELinux policy for that. Honestly, I'd > rather see us move to gssproxy before spending time on that. Yes, I think the above approach is good enough. Simo. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 8f146c92b4a5..1b2c6c19e39e 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> @@ -154,11 +155,126 @@ 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? + * + * YMMV here. + */ +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: %d\n", __func__, errno); + 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); + } + free(buf); +out_close: + 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) { @@ -166,6 +282,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; @@ -173,7 +297,6 @@ get_default_cc(void) return cc; } - static krb5_ccache init_cc_from_keytab(const char *keytab_name, const char *user) { @@ -664,10 +787,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'}, @@ -685,13 +809,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; hostbuf[0] = '\0'; @@ -699,11 +824,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; @@ -794,6 +923,12 @@ int main(const int argc, char *const argv[]) goto out; } + /* + * Must do this before setuid, as we need ptrace perms to look at + * 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)); @@ -806,7 +941,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); @@ -959,6 +1094,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; }
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. Signed-off-by: Jeff Layton <jlayton@samba.org> --- cifs.upcall.8.in | 10 ++++ cifs.upcall.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 152 insertions(+), 6 deletions(-)