Fix for a potential problem reported by a user I bumped into on IRC

last night:

_gr_breakout_yp() doesn't check for badly formatted NIS group entries.
For example, a bogus entry like this:

bootp::user1,user2,user3

will lead to a null pointer dereference and a SEGV (note that the GID
field is missing -- this results in one of the strsep(&result, ":")
returning NULL). The symtpom of this problem is programs dumping
core left and right the moment you add a + entry to /etc/group.
Note that while this is similar to an earlier bug, it's caused by a
different set of circumstances.

The fix is to check for the NULL pointers and have _gr_breakout_yp()
punt and return a failure code if it catches one. This is more or
less the behavior of SunOS: if a bad NIS group entry is encountered,
it's silently ignored. I don't think our standard (non-NIS) group
parsing code behaves the same way. It doesn't crash though, so I'm
citing the 'it ain't broken, don't fix it' rule and leaving it alone.

I'll probably have to add similar checks to _pw_breakout_yp() in
getpwent.c to ward off the same problems. It's rare that bad NIS
map entries like this occur, but we should handle them gracefully
when they do.

1 files changed, 14 insertions, 9 deletions

diff --git a/lib/libc/gen/getgrent.c b/lib/libc/gen/getgrent.c
index e9b9d1f..3a4a067b 100644
--- a/lib/libc/gen/getgrent.c
+++ b/lib/libc/gen/getgrent.c
@@ -283,22 +283,27 @@ grscan(search, gid, name)
 
 #ifdef YP
 
-static void
+static int
 _gr_breakout_yp(struct group *gr, char *result)
 {
 	char *s, *cp;
 	char **m;
 
-	s = strsep(&result, ":"); /* name */
+	/*
+	 * XXX If 's' ends up being a NULL pointer, punt on this group.
+	 * It means the NIS group entry is badly formatted and should
+	 * be skipped.
+	 */
+	if ((s = strsep(&result, ":")) == NULL) return 0; /* name */
 	gr->gr_name = s;
 
-	s = strsep(&result, ":"); /* password */
+	if ((s = strsep(&result, ":")) == NULL) return 0; /* password */
 	gr->gr_passwd = s;
 
-	s = strsep(&result, ":"); /* gid */
+	if ((s = strsep(&result, ":")) == NULL) return 0; /* gid */
 	gr->gr_gid = atoi(s);
 
-	s = result;
+	if ((s = result) == NULL) return 0;
 	cp = 0;
 
 	for (m = _gr_group.gr_mem = members; /**/; s++) {
@@ -322,6 +327,8 @@ _gr_breakout_yp(struct group *gr, char *result)
 		}
 	}
 	*m = NULL;
+
+	return 1;
 }
 
 static char *_gr_yp_domain;
@@ -348,9 +355,8 @@ _getypgroup(struct group *gr, const char *name, char *map)
 	if(resultlen >= sizeof resultbuf) return 0;
 	strcpy(resultbuf, result);
 	result = resultbuf;
-	_gr_breakout_yp(gr, resultbuf);
+	return(_gr_breakout_yp(gr, resultbuf));
 
-	return 1;
 }
 
 
@@ -398,9 +404,8 @@ unpack:
 		strcpy(resultbuf, result);
 		free(result);
 		if(result = strchr(resultbuf, '\n')) *result = '\0';
-		_gr_breakout_yp(gr, resultbuf);
+		return(_gr_breakout_yp(gr, resultbuf));
 	}
-	return 1;
 }
 
 #endif /* YP */