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.

diff --git a/MANIFEST b/MANIFEST
index e0f137af4e6026ae285e702cf0d9d6d335bb4241..e6eef4bdeea44894a264bdb08f0bd36a7f6c16b8 100644 (file)
--- 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 d1a5c7e21a717011e6573896d4300871e31de1d9..783e887a10c7cc3d0bfe6a0325e921b0501a31b3 100644 (file)
--- 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 (file)
index 0000000..c04d6b2
--- /dev/null
+++ b/common/gidlist.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) 2013 Todd C. Miller <Todd.Miller@courtesan.com>
+ *
+ * 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 <config.h>
+
+#include <sys/types.h>
+
+#include <stdio.h>
+#ifdef STDC_HEADERS
+# include <stdlib.h>
+# include <stddef.h>
+#else
+# ifdef HAVE_STDLIB_H
+#  include <stdlib.h>
+# endif
+#endif /* STDC_HEADERS */
+#include <grp.h>
+
+#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 6f5af0a2e3f6d8684eb466926c31a727577d8715..802ad94607e38108068248f28e382af630a9e53d 100644 (file)
--- 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 80dda05401fc202f758ac093fbc274c495ce8bf6..fbe984129abf87112c0f4c260b2ca7aa263c5ea1 100644 (file)
--- 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 42570a25585a7db2287cc55e2d864b45504709d4..f156d9a41f2747fbb156b6e00f2c7e36f2c6cdcd 100644 (file)
--- 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 37ae96fbbee92b9ab9651dfdd276f65d556c22c4..69bf65163154e5e067ebb4e28b2fcf303a7c3ab9 100644 (file)
--- 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 */