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

@@ -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

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

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);
+}

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;
+	/* parse_gid_list() will call fatalx() on error. */
-	GETGROUPS_T *gids;
+	user_ngids = parse_gid_list(groups, &user_gid, &user_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;
     }
 
     /* Stash initial umask for later use. */

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;

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) {
+		    if (errstr != NULL)
-			warningx(_("%s: %s"), info[i], _(errstr));
+			fatalx(_("%s: %s"), info[i], _(errstr));
-		    } else {
+		    details->egid = (gid_t)id;
-			details->egid = (gid_t)id;
+		    SET(details->flags, CD_SET_EGID);
-			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) {
+		    if (errstr != NULL)
-			warningx(_("%s: %s"), info[i], _(errstr));
+			fatalx(_("%s: %s"), info[i], _(errstr));
-		    } else {
+		    details->euid = (uid_t)id;
-			details->euid = (uid_t)id;
+		    SET(details->flags, CD_SET_EUID);
-			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) {
+		    if (errstr != NULL)
-			warningx(_("%s: %s"), info[i], _(errstr));
+			fatalx(_("%s: %s"), info[i], _(errstr));
-		    } else {
+		    details->gid = (gid_t)id;
-			details->gid = (gid_t)id;
+		    SET(details->flags, CD_SET_GID);
-			SET(details->flags, CD_SET_GID);
-		    }
 		    break;
 		}
 		if (strncmp("runas_groups=", info[i], sizeof("runas_groups=") - 1) == 0) {
-		    int j;
+		    /* parse_gid_list() will call fatalx() on error. */
-
-		    /* count groups, alloc and fill in */
 		    cp = info[i] + sizeof("runas_groups=") - 1;
-		    if (*cp == '\0')
+		    details->ngroups = parse_gid_list(cp, NULL, &details->groups);
-			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;
-		    }
 		    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) {
+		    if (errstr != NULL)
-			warningx(_("%s: %s"), info[i], _(errstr));
+			fatalx(_("%s: %s"), info[i], _(errstr));
-		    } else {
+		    details->uid = (uid_t)id;
-			details->uid = (uid_t)id;
+		    SET(details->flags, CD_SET_UID);
-			SET(details->flags, CD_SET_UID);
-		    }
 		    break;
 		}
 #ifdef HAVE_PRIV_SET

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 */