Replace sequence number-based cycle detection in visudo with a

"used" flag in struct alias.  The caller is required to call
alias_put() when it is done with the alias.  Inspired by a patch
from Daniel Kopecek.

MANIFEST

@@ -341,6 +341,8 @@ plugins/sudoers/regress/visudo/test2.sh
 plugins/sudoers/regress/visudo/test3.err.ok
 plugins/sudoers/regress/visudo/test3.out.ok
 plugins/sudoers/regress/visudo/test3.sh
+plugins/sudoers/regress/visudo/test4.out.ok
+plugins/sudoers/regress/visudo/test4.sh
 plugins/sudoers/set_perms.c
 plugins/sudoers/sha2.c
 plugins/sudoers/sha2.h

plugins/sudoers/alias.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004-2005, 2007-2012
+ * Copyright (c) 2004-2005, 2007-2013
  *	Todd C. Miller <Todd.Miller@courtesan.com>
  *
  * Permission to use, copy, modify, and distribute this software for any
@@ -49,7 +49,6 @@
  * Globals
  */
 struct rbtree *aliases;
-unsigned int alias_seqno;
 
 /*
  * Comparison function for the red-black tree.
@@ -75,35 +74,48 @@ alias_compare(const void *v1, const void *v2)
 /*
  * Search the tree for an alias with the specified name and type.
  * Returns a pointer to the alias structure or NULL if not found.
+ * Caller is responsible for calling alias_put() on the returned
+ * alias to mark it as unused.
  */
 struct alias *
-alias_find(char *name, int type)
+alias_get(char *name, int type)
 {
     struct alias key;
     struct rbnode *node;
     struct alias *a = NULL;
-    debug_decl(alias_find, SUDO_DEBUG_ALIAS)
+    debug_decl(alias_get, SUDO_DEBUG_ALIAS)
 
     key.name = name;
     key.type = type;
     if ((node = rbfind(aliases, &key)) != NULL) {
 	/*
-	 * Compare the global sequence number with the one stored
+	 * Check whether this alias is already in use.
-	 * in the alias.  If they match then we've seen this alias
+	 * If so, we've detected a loop.  If not, set the flag,
-	 * before and found a loop.
+	 * which the caller should clear with a call to alias_put().
 	 */
 	a = node->data;
-	if (a->seqno == alias_seqno) {
+	if (a->used) {
 	    errno = ELOOP;
 	    debug_return_ptr(NULL);
 	}
-	a->seqno = alias_seqno;
+	a->used = true;
     } else {
 	errno = ENOENT;
     }
     debug_return_ptr(a);
 }
 
+/*
+ * Clear the "used" flag in an alias once the caller is done with it.
+ */
+void
+alias_put(struct alias *a)
+{
+    debug_decl(alias_put, SUDO_DEBUG_ALIAS)
+    a->used = false;
+    debug_return;
+}
+
 /*
  * Add an alias to the aliases redblack tree.
  * Returns NULL on success and an error string on failure.
@@ -118,7 +130,7 @@ alias_add(char *name, int type, struct member *members)
     a = ecalloc(1, sizeof(*a));
     a->name = name;
     a->type = type;
-    /* a->seqno = 0; */
+    /* a->used = false; */
     list2tq(&a->members, members);
     if (rbinsert(aliases, a)) {
 	snprintf(errbuf, sizeof(errbuf), N_("Alias `%s' already defined"), name);

plugins/sudoers/match.c

@@ -109,13 +109,13 @@ static bool command_matches_normal(char *, char *, struct sudo_digest *);
  * Check for user described by pw in a list of members.
  * Returns ALLOW, DENY or UNSPEC.
  */
-static int
+int
-_userlist_matches(struct passwd *pw, struct member_list *list)
+userlist_matches(struct passwd *pw, struct member_list *list)
 {
     struct member *m;
     struct alias *a;
     int rval, matched = UNSPEC;
-    debug_decl(_userlist_matches, SUDO_DEBUG_MATCH)
+    debug_decl(userlist_matches, SUDO_DEBUG_MATCH)
 
     tq_foreach_rev(list, m) {
 	switch (m->type) {
@@ -131,10 +131,11 @@ _userlist_matches(struct passwd *pw, struct member_list *list)
 		    matched = !m->negated;
 		break;
 	    case ALIAS:
-		if ((a = alias_find(m->name, USERALIAS)) != NULL) {
+		if ((a = alias_get(m->name, USERALIAS)) != NULL) {
-		    rval = _userlist_matches(pw, &a->members);
+		    rval = userlist_matches(pw, &a->members);
 		    if (rval != UNSPEC)
 			matched = m->negated ? !rval : rval;
+		    alias_put(a);
 		    break;
 		}
 		/* FALLTHROUGH */
@@ -149,20 +150,13 @@ _userlist_matches(struct passwd *pw, struct member_list *list)
     debug_return_bool(matched);
 }
 
-int
-userlist_matches(struct passwd *pw, struct member_list *list)
-{
-    alias_seqno++;
-    return _userlist_matches(pw, list);
-}
-
 /*
  * Check for user described by pw in a list of members.
  * If both lists are empty compare against def_runas_default.
  * Returns ALLOW, DENY or UNSPEC.
  */
-static int
+int
-_runaslist_matches(struct member_list *user_list,
+runaslist_matches(struct member_list *user_list,
     struct member_list *group_list, struct member **matching_user,
     struct member **matching_group)
 {
@@ -171,7 +165,7 @@ _runaslist_matches(struct member_list *user_list,
     int rval;
     int user_matched = UNSPEC;
     int group_matched = UNSPEC;
-    debug_decl(_runaslist_matches, SUDO_DEBUG_MATCH)
+    debug_decl(runaslist_matches, SUDO_DEBUG_MATCH)
 
     if (runas_pw != NULL) {
 	/* If no runas user or runas group listed in sudoers, use default. */
@@ -192,11 +186,12 @@ _runaslist_matches(struct member_list *user_list,
 			user_matched = !m->negated;
 		    break;
 		case ALIAS:
-		    if ((a = alias_find(m->name, RUNASALIAS)) != NULL) {
+		    if ((a = alias_get(m->name, RUNASALIAS)) != NULL) {
-			rval = _runaslist_matches(&a->members, &empty,
+			rval = runaslist_matches(&a->members, &empty,
 			    matching_user, NULL);
 			if (rval != UNSPEC)
 			    user_matched = m->negated ? !rval : rval;
+			alias_put(a);
 			break;
 		    }
 		    /* FALLTHROUGH */
@@ -229,11 +224,12 @@ _runaslist_matches(struct member_list *user_list,
 		    group_matched = !m->negated;
 		    break;
 		case ALIAS:
-		    if ((a = alias_find(m->name, RUNASALIAS)) != NULL) {
+		    if ((a = alias_get(m->name, RUNASALIAS)) != NULL) {
-			rval = _runaslist_matches(&empty, &a->members,
+			rval = runaslist_matches(&empty, &a->members,
 			    NULL, matching_group);
 			if (rval != UNSPEC)
 			    group_matched = m->negated ? !rval : rval;
+			alias_put(a);
 			break;
 		    }
 		    /* FALLTHROUGH */
@@ -261,27 +257,17 @@ _runaslist_matches(struct member_list *user_list,
     debug_return_int(UNSPEC);
 }
 
-int
-runaslist_matches(struct member_list *user_list,
-    struct member_list *group_list, struct member **matching_user,
-    struct member **matching_group)
-{
-    alias_seqno++;
-    return _runaslist_matches(user_list ? user_list : &empty,
-	group_list ? group_list : &empty, matching_user, matching_group);
-}
-
 /*
  * Check for host and shost in a list of members.
  * Returns ALLOW, DENY or UNSPEC.
  */
-static int
+int
-_hostlist_matches(struct member_list *list)
+hostlist_matches(struct member_list *list)
 {
     struct member *m;
     struct alias *a;
     int rval, matched = UNSPEC;
-    debug_decl(_hostlist_matches, SUDO_DEBUG_MATCH)
+    debug_decl(hostlist_matches, SUDO_DEBUG_MATCH)
 
     tq_foreach_rev(list, m) {
 	switch (m->type) {
@@ -297,10 +283,11 @@ _hostlist_matches(struct member_list *list)
 		    matched = !m->negated;
 		break;
 	    case ALIAS:
-		if ((a = alias_find(m->name, HOSTALIAS)) != NULL) {
+		if ((a = alias_get(m->name, HOSTALIAS)) != NULL) {
-		    rval = _hostlist_matches(&a->members);
+		    rval = hostlist_matches(&a->members);
 		    if (rval != UNSPEC)
 			matched = m->negated ? !rval : rval;
+		    alias_put(a);
 		    break;
 		}
 		/* FALLTHROUGH */
@@ -315,23 +302,16 @@ _hostlist_matches(struct member_list *list)
     debug_return_bool(matched);
 }
 
-int
-hostlist_matches(struct member_list *list)
-{
-    alias_seqno++;
-    return _hostlist_matches(list);
-}
-
 /*
  * Check for cmnd and args in a list of members.
  * Returns ALLOW, DENY or UNSPEC.
  */
-static int
+int
-_cmndlist_matches(struct member_list *list)
+cmndlist_matches(struct member_list *list)
 {
     struct member *m;
     int matched = UNSPEC;
-    debug_decl(_cmndlist_matches, SUDO_DEBUG_MATCH)
+    debug_decl(cmndlist_matches, SUDO_DEBUG_MATCH)
 
     tq_foreach_rev(list, m) {
 	matched = cmnd_matches(m);
@@ -341,13 +321,6 @@ _cmndlist_matches(struct member_list *list)
     debug_return_bool(matched);
 }
 
-int
-cmndlist_matches(struct member_list *list)
-{
-    alias_seqno++;
-    return _cmndlist_matches(list);
-}
-
 /*
  * Check cmnd and args.
  * Returns ALLOW, DENY or UNSPEC.
@@ -365,11 +338,11 @@ cmnd_matches(struct member *m)
 	    matched = !m->negated;
 	    break;
 	case ALIAS:
-	    alias_seqno++;
+	    if ((a = alias_get(m->name, CMNDALIAS)) != NULL) {
-	    if ((a = alias_find(m->name, CMNDALIAS)) != NULL) {
+		rval = cmndlist_matches(&a->members);
-		rval = _cmndlist_matches(&a->members);
 		if (rval != UNSPEC)
 		    matched = m->negated ? !rval : rval;
+		alias_put(a);
 	    }
 	    break;
 	case COMMAND:

plugins/sudoers/parse.c

@@ -754,7 +754,7 @@ _print_member(struct lbuf *lbuf, char *name, int type, int negated,
 	    }
 	    break;
 	case ALIAS:
-	    if ((a = alias_find(name, alias_type)) != NULL) {
+	    if ((a = alias_get(name, alias_type)) != NULL) {
 		tq_foreach_fwd(&a->members, m) {
 		    if (m != tq_first(&a->members))
 			lbuf_append(lbuf, "%s", separator);
@@ -762,6 +762,7 @@ _print_member(struct lbuf *lbuf, char *name, int type, int negated,
 			negated ? !m->negated : m->negated, separator,
 			alias_type);
 		}
+		alias_put(a);
 		break;
 	    }
 	    /* FALLTHROUGH */
@@ -775,7 +776,6 @@ _print_member(struct lbuf *lbuf, char *name, int type, int negated,
 static void
 print_member(struct lbuf *lbuf, struct member *m, int alias_type)
 {
-    alias_seqno++;
     _print_member(lbuf, m->name, m->type, m->negated, ", ", alias_type);
 }
 
@@ -783,6 +783,5 @@ static void
 print_member2(struct lbuf *lbuf, struct member *m, const char *separator,
     int alias_type)
 {
-    alias_seqno++;
     _print_member(lbuf, m->name, m->type, m->negated, separator, alias_type);
 }

plugins/sudoers/parse.h

@@ -162,7 +162,7 @@ struct runascontainer {
 struct alias {
     char *name;				/* alias name */
     unsigned short type;		/* {USER,HOST,RUNAS,CMND}ALIAS */
-    unsigned short seqno;		/* sequence number */
+    bool used;				/* "used" flag for cycle detection */
     struct member_list members;		/* list of alias members */
 };
 
@@ -184,37 +184,43 @@ struct defaults {
 extern struct userspec_list userspecs;
 extern struct defaults_list defaults;
 
-/*
+/* alias.c */
- * Alias sequence number to avoid loops.
- */
-extern unsigned int alias_seqno;
-
-/*
- * Prototypes
- */
-char *alias_add(char *, int, struct member *);
-bool addr_matches(char *);
-int cmnd_matches(struct member *);
-int cmndlist_matches(struct member_list *);
-bool command_matches(char *, char *, struct sudo_digest *);
-int hostlist_matches(struct member_list *);
-bool hostname_matches(char *, char *, char *);
-bool netgr_matches(char *, char *, char *, char *);
 bool no_aliases(void);
-int runaslist_matches(struct member_list *, struct member_list *, struct member **, struct member **);
+char *alias_add(char *name, int type, struct member *members);
-int userlist_matches(struct passwd *, struct member_list *);
+int alias_compare(const void *a1, const void *a2);
-bool usergr_matches(char *, char *, struct passwd *);
+struct alias *alias_get(char *name, int type);
-bool userpw_matches(char *, char *, struct passwd *);
+struct alias *alias_remove(char *name, int type);
-bool group_matches(char *, struct group *);
+void alias_apply(int (*func)(void *, void *), void *cookie);
-struct alias *alias_find(char *, int);
+void alias_free(void *a);
-struct alias *alias_remove(char *, int);
+void alias_put(struct alias *a);
-void alias_free(void *);
-void alias_apply(int (*)(void *, void *), void *);
 void init_aliases(void);
-void init_lexer(void);
+
+/* gram.c */
 void init_parser(const char *, bool);
-int alias_compare(const void *, const void *);
+
+/* match_addr.c */
+bool addr_matches(char *n);
+
+/* match.c */
+bool command_matches(char *sudoers_cmnd, char *sudoers_args, struct sudo_digest *digest);
+bool group_matches(char *sudoers_group, struct group *gr);
+bool hostname_matches(char *shost, char *lhost, char *pattern);
+bool netgr_matches(char *netgr, char *lhost, char *shost, char *user);
+bool usergr_matches(char *group, char *user, struct passwd *pw);
+bool userpw_matches(char *sudoers_user, char *user, struct passwd *pw);
+int cmnd_matches(struct member *m);
+int cmndlist_matches(struct member_list *list);
+int hostlist_matches(struct member_list *list);
+int runaslist_matches(struct member_list *user_list, struct member_list *group_list, struct member **matching_user, struct member **matching_group);
+int userlist_matches(struct passwd *pw, struct member_list *list);
+
+/* toke.c */
+void init_lexer(void);
+
+/* hexchar.c */
 int hexchar(const char *s);
+
+/* base64.c */
 size_t base64_decode(const char *str, unsigned char *dst, size_t dsize);
 
 #endif /* _SUDOERS_PARSE_H */

plugins/sudoers/regress/visudo/test4.out.ok

@@ -0,0 +1 @@
+stdin: parsed OK

plugins/sudoers/regress/visudo/test4.sh

@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# Test cycle detection and duplicate entries.
+# Prior to sudo 1.8.7 this resulted in a false positive.
+#
+
+./visudo -csf - <<EOF
+Host_Alias H1 = host1
+Host_Alias H2 = H1, host2
+Host_Alias H3 = H1, H2
+root H3 = ALL
+EOF
+
+exit 0

plugins/sudoers/visudo.c

@@ -1075,7 +1075,6 @@ alias_remove_recursive(char *name, int type)
 	}
 	rbinsert(alias_freelist, a);
     }
-    alias_seqno++;
     debug_return_bool(rval);
 }
 
@@ -1087,12 +1086,13 @@ check_alias(char *name, int type, int strict, int quiet)
     int errors = 0;
     debug_decl(check_alias, SUDO_DEBUG_ALIAS)
 
-    if ((a = alias_find(name, type)) != NULL) {
+    if ((a = alias_get(name, type)) != NULL) {
 	/* check alias contents */
 	tq_foreach_fwd(&a->members, m) {
 	    if (m->type == ALIAS)
 		errors += check_alias(m->name, type, strict, quiet);
 	}
+	alias_put(a);
     } else {
 	if (!quiet) {
 	    char *fmt;
@@ -1137,26 +1137,22 @@ check_aliases(bool strict, bool quiet)
     tq_foreach_fwd(&userspecs, us) {
 	tq_foreach_fwd(&us->users, m) {
 	    if (m->type == ALIAS) {
-		alias_seqno++;
 		errors += check_alias(m->name, USERALIAS, strict, quiet);
 	    }
 	}
 	tq_foreach_fwd(&us->privileges, priv) {
 	    tq_foreach_fwd(&priv->hostlist, m) {
 		if (m->type == ALIAS) {
-		    alias_seqno++;
 		    errors += check_alias(m->name, HOSTALIAS, strict, quiet);
 		}
 	    }
 	    tq_foreach_fwd(&priv->cmndlist, cs) {
 		tq_foreach_fwd(&cs->runasuserlist, m) {
 		    if (m->type == ALIAS) {
-			alias_seqno++;
 			errors += check_alias(m->name, RUNASALIAS, strict, quiet);
 		    }
 		}
 		if ((m = cs->cmnd)->type == ALIAS) {
-		    alias_seqno++;
 		    errors += check_alias(m->name, CMNDALIAS, strict, quiet);
 		}
 	    }
@@ -1167,7 +1163,6 @@ check_aliases(bool strict, bool quiet)
     tq_foreach_fwd(&userspecs, us) {
 	tq_foreach_fwd(&us->users, m) {
 	    if (m->type == ALIAS) {
-		alias_seqno++;
 		if (!alias_remove_recursive(m->name, USERALIAS))
 		    errors++;
 	    }
@@ -1175,7 +1170,6 @@ check_aliases(bool strict, bool quiet)
 	tq_foreach_fwd(&us->privileges, priv) {
 	    tq_foreach_fwd(&priv->hostlist, m) {
 		if (m->type == ALIAS) {
-		    alias_seqno++;
 		    if (!alias_remove_recursive(m->name, HOSTALIAS))
 			errors++;
 		}
@@ -1183,13 +1177,11 @@ check_aliases(bool strict, bool quiet)
 	    tq_foreach_fwd(&priv->cmndlist, cs) {
 		tq_foreach_fwd(&cs->runasuserlist, m) {
 		    if (m->type == ALIAS) {
-			alias_seqno++;
 			if (!alias_remove_recursive(m->name, RUNASALIAS))
 			    errors++;
 		    }
 		}
 		if ((m = cs->cmnd)->type == ALIAS) {
-		    alias_seqno++;
 		    if (!alias_remove_recursive(m->name, CMNDALIAS))
 			errors++;
 		}
@@ -1216,7 +1208,6 @@ check_aliases(bool strict, bool quiet)
 	tq_foreach_fwd(&d->binding, binding) {
 	    for (m = binding; m != NULL; m = m->next) {
 		if (m->type == ALIAS) {
-		    alias_seqno++;
 		    if (!alias_remove_recursive(m->name, atype))
 			errors++;
 		}