From 8b4fbc5cc055b4b46581d8a29b256f86e682dfad Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Thu, 8 Aug 2013 11:40:36 -0600 Subject: [PATCH] Refactor code to parse list of gids into its own function that is shared by the sudo front-end and the sudoers module. Make uid/gid parse error be fatal, not just a warning. --- MANIFEST | 1 + common/Makefile.in | 6 ++- common/gidlist.c | 91 +++++++++++++++++++++++++++++++++++++++ plugins/sudoers/policy.c | 39 +++-------------- plugins/sudoers/sudoers.h | 3 ++ src/sudo.c | 68 ++++++++--------------------- src/sudo.h | 3 ++ 7 files changed, 128 insertions(+), 83 deletions(-) create mode 100644 common/gidlist.c diff --git a/MANIFEST b/MANIFEST index e0f137af4..e6eef4bde 100644 --- a/MANIFEST +++ b/MANIFEST @@ -15,6 +15,7 @@ common/atoid.c common/error.c common/fileops.c common/fmt_string.c +common/gidlist.c common/lbuf.c common/list.c common/regress/sudo_conf/conf_test.c diff --git a/common/Makefile.in b/common/Makefile.in index d1a5c7e21..783e887a1 100644 --- a/common/Makefile.in +++ b/common/Makefile.in @@ -67,7 +67,7 @@ DEFS = @OSDEFS@ -D_PATH_SUDO_CONF=\"$(sysconfdir)/sudo.conf\" SHELL = @SHELL@ LTOBJS = alloc.lo atobool.lo atoid.lo error.lo fileops.lo fmt_string.lo \ - lbuf.lo list.lo secure_path.lo setgroups.lo sudo_conf.lo \ + gidlist.lo lbuf.lo list.lo secure_path.lo setgroups.lo sudo_conf.lo \ sudo_debug.lo sudo_printf.lo term.lo ttysize.lo @COMMON_OBJS@ PARSELN_TEST_OBJS = parseln_test.lo @@ -182,6 +182,10 @@ fileops.lo: $(srcdir)/fileops.c $(top_builddir)/config.h \ fmt_string.lo: $(srcdir)/fmt_string.c $(top_builddir)/config.h \ $(incdir)/missing.h $(incdir)/sudo_debug.h $(LIBTOOL) --mode=compile $(CC) -c -o $@ $(CPPFLAGS) $(CFLAGS) $(PIE_CFLAGS) $(SSP_CFLAGS) $(DEFS) $(srcdir)/fmt_string.c +gidlist.lo: $(srcdir)/gidlist.c $(top_builddir)/config.h $(incdir)/gettext.h \ + $(incdir)/missing.h $(incdir)/alloc.h $(incdir)/error.h \ + $(incdir)/sudo_debug.h + $(LIBTOOL) --mode=compile $(CC) -c -o $@ $(CPPFLAGS) $(CFLAGS) $(PIE_CFLAGS) $(SSP_CFLAGS) $(DEFS) $(srcdir)/gidlist.c lbuf.lo: $(srcdir)/lbuf.c $(top_builddir)/config.h $(incdir)/missing.h \ $(incdir)/alloc.h $(incdir)/error.h $(incdir)/lbuf.h \ $(incdir)/sudo_debug.h diff --git a/common/gidlist.c b/common/gidlist.c new file mode 100644 index 000000000..c04d6b240 --- /dev/null +++ b/common/gidlist.c @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2013 Todd C. Miller + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include + +#include + +#include +#ifdef STDC_HEADERS +# include +# include +#else +# ifdef HAVE_STDLIB_H +# include +# endif +#endif /* STDC_HEADERS */ +#include + +#define DEFAULT_TEXT_DOMAIN "sudo" +#include "gettext.h" + +#include "missing.h" +#include "alloc.h" +#include "error.h" +#include "sudo_debug.h" + +extern id_t atoid(const char *p, const char *sep, char **endp, const char **errstr); + +/* + * Parse a comma-separated list of gids into an allocated array of GETGROUPS_T. + * If a pointer to the base gid is specified, it is stored as the first element + * in the array. + * Returns the number of gids in the allocated array. + * Calls fatalx() on error. + */ +int +parse_gid_list(const char *gidstr, const gid_t *basegid, GETGROUPS_T **gidsp) +{ + int ngids = 0; + GETGROUPS_T *gids; + const char *cp = gidstr; + const char *errstr; + char *ep; + debug_decl(atoid, SUDO_DEBUG_UTIL) + + /* Count groups. */ + if (*cp != '\0') { + ngids++; + do { + if (*cp++ == ',') + ngids++; + } while (*cp != '\0'); + } + /* Base gid is optional. */ + if (basegid != NULL) + ngids++; + /* Allocate and fill in array. */ + if (ngids != 0) { + gids = emalloc2(ngids, sizeof(GETGROUPS_T)); + ngids = 0; + if (basegid != NULL) + gids[ngids++] = *basegid; + cp = gidstr; + do { + gids[ngids] = (GETGROUPS_T) atoid(cp, ",", &ep, &errstr); + if (errstr != NULL) { + warningx(_("%s: %s"), cp, _(errstr)); + free(gids); + debug_return_int(-1); + } + if (basegid == NULL || gids[ngids] != *basegid) + ngids++; + cp = ep + 1; + } while (*ep != '\0'); + *gidsp = gids; + } + debug_return_int(ngids); +} diff --git a/plugins/sudoers/policy.c b/plugins/sudoers/policy.c index 6f5af0a2e..802ad9460 100644 --- a/plugins/sudoers/policy.c +++ b/plugins/sudoers/policy.c @@ -105,14 +105,14 @@ sudoers_policy_deserialize_info(void *v, char **runas_user, char **runas_group) p = *cur + sizeof("sudoers_uid=") - 1; sudoers_uid = (uid_t) atoid(p, NULL, NULL, &errstr); if (errstr != NULL) - fatalx("%s: %s", *cur, _(errstr)); + fatalx(_("%s: %s"), *cur, _(errstr)); continue; } if (MATCHES(*cur, "sudoers_gid=")) { p = *cur + sizeof("sudoers_gid=") - 1; sudoers_gid = (gid_t) atoid(p, NULL, NULL, &errstr); if (errstr != NULL) - fatalx("%s: %s", *cur, _(errstr)); + fatalx(_("%s: %s"), *cur, _(errstr)); continue; } if (MATCHES(*cur, "sudoers_mode=")) { @@ -263,14 +263,14 @@ sudoers_policy_deserialize_info(void *v, char **runas_user, char **runas_group) p = *cur + sizeof("uid=") - 1; user_uid = (uid_t) atoid(p, NULL, NULL, &errstr); if (errstr != NULL) - fatalx("%s: %s", *cur, _(errstr)); + fatalx(_("%s: %s"), *cur, _(errstr)); continue; } if (MATCHES(*cur, "gid=")) { p = *cur + sizeof("gid=") - 1; user_gid = (gid_t) atoid(p, NULL, NULL, &errstr); if (errstr != NULL) - fatalx("%s: %s", *cur, _(errstr)); + fatalx(_("%s: %s"), *cur, _(errstr)); continue; } if (MATCHES(*cur, "groups=")) { @@ -305,7 +305,7 @@ sudoers_policy_deserialize_info(void *v, char **runas_user, char **runas_group) p = *cur + sizeof("sid=") - 1; sudo_user.sid = (pid_t) atoid(p, NULL, NULL, &errstr); if (errstr != NULL) - fatalx("%s: %s", *cur, _(errstr)); + fatalx(_("%s: %s"), *cur, _(errstr)); continue; } } @@ -315,33 +315,8 @@ sudoers_policy_deserialize_info(void *v, char **runas_user, char **runas_group) user_tty = "unknown"; /* user_ttypath remains NULL */ if (groups != NULL && groups[0] != '\0') { - const char *cp; - GETGROUPS_T *gids; - int ngids; - - /* Count number of groups, including passwd gid. */ - ngids = 2; - for (cp = groups; *cp != '\0'; cp++) { - if (*cp == ',') - ngids++; - } - - /* The first gid in the list is the passwd group gid. */ - gids = emalloc2(ngids, sizeof(GETGROUPS_T)); - gids[0] = user_gid; - ngids = 1; - cp = groups; - for (;;) { - gids[ngids] = atoi(cp); - if (gids[0] != gids[ngids]) - ngids++; - cp = strchr(cp, ','); - if (cp == NULL) - break; - cp++; /* skip over comma */ - } - user_gids = gids; - user_ngids = ngids; + /* parse_gid_list() will call fatalx() on error. */ + user_ngids = parse_gid_list(groups, &user_gid, &user_gids); } /* Stash initial umask for later use. */ diff --git a/plugins/sudoers/sudoers.h b/plugins/sudoers/sudoers.h index 80dda0540..fbe984129 100644 --- a/plugins/sudoers/sudoers.h +++ b/plugins/sudoers/sudoers.h @@ -373,6 +373,9 @@ int group_plugin_query(const char *user, const char *group, /* setgroups.c */ int sudo_setgroups(int ngids, const GETGROUPS_T *gids); +/* gidlist.c */ +int parse_gid_list(const char *gidstr, const gid_t *basegid, GETGROUPS_T **gidsp); + #ifndef _SUDO_MAIN extern struct sudo_user sudo_user; extern struct passwd *list_pw; diff --git a/src/sudo.c b/src/sudo.c index 42570a255..f156d9a41 100644 --- a/src/sudo.c +++ b/src/sudo.c @@ -612,75 +612,43 @@ command_info_to_details(char * const info[], struct command_details *details) if (strncmp("runas_egid=", info[i], sizeof("runas_egid=") - 1) == 0) { cp = info[i] + sizeof("runas_egid=") - 1; id = atoid(cp, NULL, NULL, &errstr); - if (errstr != NULL) { - warningx(_("%s: %s"), info[i], _(errstr)); - } else { - details->egid = (gid_t)id; - SET(details->flags, CD_SET_EGID); - } + if (errstr != NULL) + fatalx(_("%s: %s"), info[i], _(errstr)); + details->egid = (gid_t)id; + SET(details->flags, CD_SET_EGID); break; } if (strncmp("runas_euid=", info[i], sizeof("runas_euid=") - 1) == 0) { cp = info[i] + sizeof("runas_euid=") - 1; id = atoid(cp, NULL, NULL, &errstr); - if (errstr != NULL) { - warningx(_("%s: %s"), info[i], _(errstr)); - } else { - details->euid = (uid_t)id; - SET(details->flags, CD_SET_EUID); - } + if (errstr != NULL) + fatalx(_("%s: %s"), info[i], _(errstr)); + details->euid = (uid_t)id; + SET(details->flags, CD_SET_EUID); break; } if (strncmp("runas_gid=", info[i], sizeof("runas_gid=") - 1) == 0) { cp = info[i] + sizeof("runas_gid=") - 1; id = atoid(cp, NULL, NULL, &errstr); - if (errstr != NULL) { - warningx(_("%s: %s"), info[i], _(errstr)); - } else { - details->gid = (gid_t)id; - SET(details->flags, CD_SET_GID); - } + if (errstr != NULL) + fatalx(_("%s: %s"), info[i], _(errstr)); + details->gid = (gid_t)id; + SET(details->flags, CD_SET_GID); break; } if (strncmp("runas_groups=", info[i], sizeof("runas_groups=") - 1) == 0) { - int j; - - /* count groups, alloc and fill in */ + /* parse_gid_list() will call fatalx() on error. */ cp = info[i] + sizeof("runas_groups=") - 1; - if (*cp == '\0') - break; - for (;;) { - details->ngroups++; - if ((cp = strchr(cp, ',')) == NULL) - break; - cp++; - } - if (details->ngroups != 0) { - details->groups = - emalloc2(details->ngroups, sizeof(GETGROUPS_T)); - cp = info[i] + sizeof("runas_groups=") - 1; - for (j = 0; j < details->ngroups;) { - id = atoid(cp, ",", &ep, &errstr); - if (errstr != NULL) { - warningx(_("%s: %s"), cp, _(errstr)); - break; - } - details->groups[j++] = (gid_t)id; - cp = ep + 1; - } - details->ngroups = j; - } + details->ngroups = parse_gid_list(cp, NULL, &details->groups); break; } if (strncmp("runas_uid=", info[i], sizeof("runas_uid=") - 1) == 0) { cp = info[i] + sizeof("runas_uid=") - 1; id = atoid(cp, NULL, NULL, &errstr); - if (errstr != NULL) { - warningx(_("%s: %s"), info[i], _(errstr)); - } else { - details->uid = (uid_t)id; - SET(details->flags, CD_SET_UID); - } + if (errstr != NULL) + fatalx(_("%s: %s"), info[i], _(errstr)); + details->uid = (uid_t)id; + SET(details->flags, CD_SET_UID); break; } #ifdef HAVE_PRIV_SET diff --git a/src/sudo.h b/src/sudo.h index 37ae96fbb..69bf65163 100644 --- a/src/sudo.h +++ b/src/sudo.h @@ -264,4 +264,7 @@ void init_signals(void); void restore_signals(void); void save_signals(void); +/* gidlist.c */ +int parse_gid_list(const char *gidstr, const gid_t *basegid, GETGROUPS_T **gidsp); + #endif /* _SUDO_SUDO_H */