two-level getgrent bug in samba-2.0.6 (PATCH#11)

[Jeremy Allison]

leon at eatworms.swmed.edu wrote:
> 
> I ran into a bug in samba-2.0.6: an infinite loop caused by two-level
> getgrent calls.  validate_group, in the middle of a getgrent loop, may
> indirectly call user_in_group_list, which resets getgrent.  As a result,
> validate_group never exits.  The attached patch works around this.  It's a
> quick and dirty patch which I'm including only to illustrate the problem,
> not as a suggested permanent fix.
> 

Ok, how about the following as a more permanent fix.
Can you try it out and let me know. I have tested it
here and it seems to be ok.

Thanks for pointing out the bug, this will be fixed
in 2.0.7.

Cheers,

	Jeremy Allison,
	Samba Team.

-------------------------cut here------------------------
Index: smbd/password.c
===================================================================
RCS file: /data/cvs/samba/source/smbd/password.c,v
retrieving revision 1.110.2.25
diff -u -r1.110.2.25 password.c
--- password.c	1999/12/10 00:50:01	1.110.2.25
+++ password.c	2000/01/04 00:47:35
@@ -618,24 +618,61 @@
 #ifdef HAVE_GETGRENT
 	{
 		struct group *gptr;
-		char **member;
 		setgrent();
 		while ((gptr = (struct group *)getgrent())) {
-			if (!strequal(gptr->gr_name,group))
-				continue;
-			member = gptr->gr_mem;
-			while (member && *member) {
+			if (strequal(gptr->gr_name,group))
+				break;
+		}
+
+		/*
+		 * As user_ok can recurse doing a getgrent(), we must
+		 * copy the member list into a pstring on the stack before
+		 * use. Bug pointed out by leon at eatworms.swmed.edu.
+		 */
+
+		if (gptr) {
+			pstring member_list;
+			char *member;
+			size_t copied_len = 0;
+			int i;
+
+			*member_list = '\0';
+			member = member_list;
+
+			for(i = 0; gptr->gr_mem && gptr->gr_mem[i]; i++) {
+				size_t member_len = strlen(gptr->gr_mem[i]) + 1;
+				if( copied_len + member_len < sizeof(pstring)) { 
+
+					DEBUG(10,("validate_group: = gr_mem = %s\n", gptr->gr_mem[i]));
+
+					safe_strcpy(member, gptr->gr_mem[i], sizeof(pstring) - copied_len - 1);
+					copied_len += member_len;
+					member += copied_len;
+				} else {
+					*member = '\0';
+				}
+			}
+
+			endgrent();
+
+			member = member_list;
+			while (*member) {
 				static fstring name;
-				fstrcpy(name,*member);
+				fstrcpy(name,member);
 				if (user_ok(name,snum) &&
 				    password_ok(name,password,pwlen,NULL)) {
 					endgrent();
 					return(&name[0]);
 				}
-				member++;
+
+				DEBUG(10,("validate_group = member = %s\n", member));
+
+				member += strlen(member) + 1;
 			}
+		} else {
+			endgrent();
+			return NULL;
 		}
-		endgrent();
 	}
 #endif
 	return(NULL);
-------------------------cut here------------------------
-- 
--------------------------------------------------------
Buying an operating system without source is like buying
a self-assembly Space Shuttle with no instructions.
--------------------------------------------------------