This patch fixes one of my longest-standing pet hates with Samba :-).

When we look see if a user is in a list, and we try to 'expand' an @group, we
should lookup the user's own list of groups, rather than looking for all the
members of a group.

I'm sure this will fix some nasty performance issues, particularly on large
domains etc.  In particular, this avoids contacting winbind at all, if the
group is not a winbind group.

(This caused a deadlock on my winbind-on-PDC setup).

The groups list always includes the user's primary group, as per the
getgrouplist manpage, and my recent changes to our implementation.

Andrew Bartlett
(This used to be commit 9be21976f7662ebe6eb92fff7cecbdb352eca334)

9 files changed, 81 insertions, 52 deletions

diff --git a/source3/lib/username.c b/source3/lib/username.c
index b1c9ca0f08..b8f33494ee 100644
--- a/source3/lib/username.c
+++ b/source3/lib/username.c
@@ -169,7 +169,7 @@ BOOL map_username(char *user)
 			return False;
 		}
 
-		if (strchr_m(dosname,'*') || user_in_list(user, (const char **)dosuserlist)) {
+		if (strchr_m(dosname,'*') || user_in_list(user, (const char **)dosuserlist, NULL, 0)) {
 			DEBUG(3,("Mapped user %s to %s\n",user,unixname));
 			mapped_user = True;
 			fstrcpy(last_from,user);
@@ -328,11 +328,27 @@ static BOOL user_in_winbind_group_list(const char *user, const char *gname, BOOL
 	int num_groups;
 	int i;
  	gid_t *groups = NULL;
- 	gid_t gid;
+ 	gid_t gid, gid_low, gid_high;
  	BOOL ret = False;
  
  	*winbind_answered = False;
  
+	if ((gid = nametogid(gname)) == (gid_t)-1) {
+ 		DEBUG(0,("user_in_winbind_group_list: nametogid for group %s failed.\n",
+ 			gname ));
+ 		goto err;
+ 	}
+
+	if (!lp_winbind_gid(&gid_low, &gid_high)) {
+		DEBUG(4, ("winbind gid range not configured, therefore %s cannot be a winbind group\n", gname));
+ 		goto err;
+	}
+
+	if (gid < gid_low || gid > gid_high) {
+		DEBUG(4, ("group %s is not a winbind group\n", gname));
+ 		goto err;
+	}
+ 
  	/*
  	 * Get the gid's that this user belongs to.
  	 */
@@ -361,12 +377,6 @@ failed with error %s\n", strerror(errno) ));
 	 * to a gid_t via either winbind or the local UNIX lookup and do the comparison.
 	 */
  
-	if ((gid = nametogid(gname)) == (gid_t)-1) {
- 		DEBUG(0,("user_in_winbind_group_list: winbind_lookup_name for group %s failed.\n",
- 			gname ));
- 		goto err;
- 	}
- 
  	for (i = 0; i < num_groups; i++) {
  		if (gid == groups[i]) {
  			ret = True;
@@ -389,7 +399,7 @@ failed with error %s\n", strerror(errno) ));
  Check if a user is in a UNIX group.
 ****************************************************************************/
 
-static BOOL user_in_unix_group_list(const char *user,const char *gname)
+BOOL user_in_unix_group_list(const char *user,const char *gname)
 {
 	struct passwd *pass = Get_Pwnam(user);
 	struct sys_userlist *user_list;
@@ -432,10 +442,27 @@ static BOOL user_in_unix_group_list(const char *user,const char *gname)
  Check if a user is in a group list. Ask winbind first, then use UNIX.
 ****************************************************************************/
 
-BOOL user_in_group_list(const char *user, const char *gname)
+BOOL user_in_group_list(const char *user, const char *gname, gid_t *groups, size_t n_groups)
 {
 	BOOL winbind_answered = False;
 	BOOL ret;
+	gid_t gid;
+	unsigned i;
+
+	gid = nametogid(gname);
+	if (gid == (gid_t)-1) 
+		return False;
+
+	if (groups && n_groups > 0) {
+		for (i=0; i < n_groups; i++) {
+			if (groups[i] == gid) {
+				return True;
+			}
+		}
+		return False;
+	}
+
+	/* fallback if we don't yet have the group list */
 
 	ret = user_in_winbind_group_list(user, gname, &winbind_answered);
 	if (!winbind_answered)
@@ -451,7 +478,7 @@ BOOL user_in_group_list(const char *user, const char *gname)
  and netgroup lists.
 ****************************************************************************/
 
-BOOL user_in_list(const char *user,const char **list)
+BOOL user_in_list(const char *user,const char **list, gid_t *groups, size_t n_groups)
 {
 	if (!list || !*list)
 		return False;
@@ -480,7 +507,7 @@ BOOL user_in_list(const char *user,const char **list)
 			 */
 			if(user_in_netgroup_list(user, *list +1))
 				return True;
-			if(user_in_group_list(user, *list +1))
+			if(user_in_group_list(user, *list +1, groups, n_groups))
 				return True;
 		} else if (**list == '+') {
 
@@ -488,7 +515,7 @@ BOOL user_in_list(const char *user,const char **list)
 				/*
 				 * Search UNIX list followed by netgroup.
 				 */
-				if(user_in_group_list(user, *list +2))
+				if(user_in_group_list(user, *list +2, groups, n_groups))
 					return True;
 				if(user_in_netgroup_list(user, *list +2))
 					return True;
@@ -499,7 +526,7 @@ BOOL user_in_list(const char *user,const char **list)
 				 * Just search UNIX list.
 				 */
 
-				if(user_in_group_list(user, *list +1))
+				if(user_in_group_list(user, *list +1, groups, n_groups))
 					return True;
 			}
 
@@ -511,7 +538,7 @@ BOOL user_in_list(const char *user,const char **list)
 				 */
 				if(user_in_netgroup_list(user, *list +2))
 					return True;
-				if(user_in_group_list(user, *list +2))
+				if(user_in_group_list(user, *list +2, groups, n_groups))
 					return True;
 			} else {
 				/*
diff --git a/source3/lib/util_str.c b/source3/lib/util_str.c
index 17c7cc29fe..cd906d37c0 100644
--- a/source3/lib/util_str.c
+++ b/source3/lib/util_str.c
@@ -442,6 +442,8 @@ char *safe_strcpy(char *dest,const char *src, size_t maxlength)
 		return NULL;
 	}
 
+	dest[maxlength]='\0';
+
 	if (!src) {
 		*dest = 0;
 		return dest;
@@ -450,8 +452,8 @@ char *safe_strcpy(char *dest,const char *src, size_t maxlength)
 	len = strlen(src);
 
 	if (len > maxlength) {
-		DEBUG(0,("ERROR: string overflow by %d in safe_strcpy [%.50s]\n",
-			 (int)(len-maxlength), src));
+		DEBUG(0,("ERROR: string overflow by %u (%u - %u) in safe_strcpy [%.50s]\n",
+			 (unsigned int)(len-maxlength), len, maxlength, src));
 		len = maxlength;
 	}
       
@@ -1597,7 +1599,7 @@ char * base64_encode_data_blob(DATA_BLOB data)
 {
 	int bits = 0;
 	int char_count = 0;
-	int out_cnt = 0;
+	size_t out_cnt = 0;
 	size_t len = data.length;
 	size_t output_len = data.length * 2;
 	char *result = malloc(output_len); /* get us plenty of space */
diff --git a/source3/printing/nt_printing.c b/source3/printing/nt_printing.c
index 10d490a4c1..836324ecc9 100644
--- a/source3/printing/nt_printing.c
+++ b/source3/printing/nt_printing.c
@@ -4734,7 +4734,7 @@ BOOL print_access_check(struct current_user *user, int snum, int access_type)
 	/* Always allow root or printer admins to do anything */
 
 	if (user->uid == 0 ||
-	    user_in_list(uidtoname(user->uid), lp_printer_admin(snum))) {
+	    user_in_list(uidtoname(user->uid), lp_printer_admin(snum), user->groups, user->ngroups)) {
 		return True;
 	}
 
diff --git a/source3/rpc_server/srv_samr_nt.c b/source3/rpc_server/srv_samr_nt.c
index 2896fd79e4..d766e9c19e 100644
--- a/source3/rpc_server/srv_samr_nt.c
+++ b/source3/rpc_server/srv_samr_nt.c
@@ -3427,7 +3427,7 @@ NTSTATUS _samr_add_aliasmem(pipes_struct *p, SAMR_Q_ADD_ALIASMEM *q_u, SAMR_R_AD
 	fstrcpy(grp_name, grp->gr_name);
 
 	/* if the user is already in the group */
-	if(user_in_group_list(pwd->pw_name, grp_name)) {
+	if(user_in_unix_group_list(pwd->pw_name, grp_name)) {
 		passwd_free(&pwd);
 		return NT_STATUS_MEMBER_IN_ALIAS;
 	}
@@ -3439,7 +3439,7 @@ NTSTATUS _samr_add_aliasmem(pipes_struct *p, SAMR_Q_ADD_ALIASMEM *q_u, SAMR_R_AD
 	smb_add_user_group(grp_name, pwd->pw_name);
 
 	/* check if the user has been added then ... */
-	if(!user_in_group_list(pwd->pw_name, grp_name)) {
+	if(!user_in_unix_group_list(pwd->pw_name, grp_name)) {
 		passwd_free(&pwd);
 		return NT_STATUS_MEMBER_NOT_IN_ALIAS;	/* don't know what to reply else */
 	}
@@ -3485,7 +3485,7 @@ NTSTATUS _samr_del_aliasmem(pipes_struct *p, SAMR_Q_DEL_ALIASMEM *q_u, SAMR_R_DE
 	if ((grp=getgrgid(map.gid)) == NULL)
 		return NT_STATUS_NO_SUCH_ALIAS;
 
-	/* we need to copy the name otherwise it's overloaded in user_in_group_list */
+	/* we need to copy the name otherwise it's overloaded in user_in_unix_group_list */
 	fstrcpy(grp_name, grp->gr_name);
 
 	/* check if the user exists before trying to remove it from the group */
@@ -3497,7 +3497,7 @@ NTSTATUS _samr_del_aliasmem(pipes_struct *p, SAMR_Q_DEL_ALIASMEM *q_u, SAMR_R_DE
 	}
 
 	/* if the user is not in the group */
-	if(!user_in_group_list(pdb_get_username(sam_pass), grp_name)) {
+	if(!user_in_unix_group_list(pdb_get_username(sam_pass), grp_name)) {
 		pdb_free_sam(&sam_pass);
 		return NT_STATUS_MEMBER_IN_ALIAS;
 	}
@@ -3505,7 +3505,7 @@ NTSTATUS _samr_del_aliasmem(pipes_struct *p, SAMR_Q_DEL_ALIASMEM *q_u, SAMR_R_DE
 	smb_delete_user_group(grp_name, pdb_get_username(sam_pass));
 
 	/* check if the user has been removed then ... */
-	if(user_in_group_list(pdb_get_username(sam_pass), grp_name)) {
+	if(user_in_unix_group_list(pdb_get_username(sam_pass), grp_name)) {
 		pdb_free_sam(&sam_pass);
 		return NT_STATUS_MEMBER_NOT_IN_ALIAS;	/* don't know what to reply else */
 	}
@@ -3583,11 +3583,11 @@ NTSTATUS _samr_add_groupmem(pipes_struct *p, SAMR_Q_ADD_GROUPMEM *q_u, SAMR_R_AD
 		return NT_STATUS_NO_SUCH_GROUP;
 	}
 
-	/* we need to copy the name otherwise it's overloaded in user_in_group_list */
+	/* we need to copy the name otherwise it's overloaded in user_in_unix_group_list */
 	fstrcpy(grp_name, grp->gr_name);
 
 	/* if the user is already in the group */
-	if(user_in_group_list(pwd->pw_name, grp_name)) {
+	if(user_in_unix_group_list(pwd->pw_name, grp_name)) {
 		passwd_free(&pwd);
 		return NT_STATUS_MEMBER_IN_GROUP;
 	}
@@ -3601,7 +3601,7 @@ NTSTATUS _samr_add_groupmem(pipes_struct *p, SAMR_Q_ADD_GROUPMEM *q_u, SAMR_R_AD
 	smb_add_user_group(grp_name, pwd->pw_name);
 
 	/* check if the user has been added then ... */
-	if(!user_in_group_list(pwd->pw_name, grp_name)) {
+	if(!user_in_unix_group_list(pwd->pw_name, grp_name)) {
 		passwd_free(&pwd);
 		return NT_STATUS_MEMBER_NOT_IN_GROUP;		/* don't know what to reply else */
 	}
@@ -3662,7 +3662,7 @@ NTSTATUS _samr_del_groupmem(pipes_struct *p, SAMR_Q_DEL_GROUPMEM *q_u, SAMR_R_DE
 	}
 
 	/* if the user is not in the group */
-	if (!user_in_group_list(pdb_get_username(sam_pass), grp_name)) {
+	if (!user_in_unix_group_list(pdb_get_username(sam_pass), grp_name)) {
 		pdb_free_sam(&sam_pass);
 		return NT_STATUS_MEMBER_NOT_IN_GROUP;
 	}
@@ -3670,7 +3670,7 @@ NTSTATUS _samr_del_groupmem(pipes_struct *p, SAMR_Q_DEL_GROUPMEM *q_u, SAMR_R_DE
 	smb_delete_user_group(grp_name, pdb_get_username(sam_pass));
 
 	/* check if the user has been removed then ... */
-	if (user_in_group_list(pdb_get_username(sam_pass), grp_name)) {
+	if (user_in_unix_group_list(pdb_get_username(sam_pass), grp_name)) {
 		pdb_free_sam(&sam_pass);
 		return NT_STATUS_ACCESS_DENIED;		/* don't know what to reply else */
 	}
diff --git a/source3/rpc_server/srv_spoolss_nt.c b/source3/rpc_server/srv_spoolss_nt.c
index 0bcc3c5a30..3a7ced7725 100644
--- a/source3/rpc_server/srv_spoolss_nt.c
+++ b/source3/rpc_server/srv_spoolss_nt.c
@@ -1606,7 +1606,7 @@ Can't find printer handle we created for printer %s\n", name ));
 			/* if the user is not root and not a printer admin, then fail */
 			
 			if ( user.uid != 0
-			     && !user_in_list(uidtoname(user.uid), lp_printer_admin(snum)) )
+			     && !user_in_list(uidtoname(user.uid), lp_printer_admin(snum), user.groups, user.ngroups) )
 			{
 				close_printer_handle(p, handle);
 				return WERR_ACCESS_DENIED;
@@ -1653,7 +1653,7 @@ Can't find printer handle we created for printer %s\n", name ));
 
 		/* check smb.conf parameters and the the sec_desc */
 		
-		if (!user_ok(uidtoname(user.uid), snum) || !print_access_check(&user, snum, printer_default->access_required)) {
+		if (!user_ok(uidtoname(user.uid), snum, user.groups, user.ngroups) || !print_access_check(&user, snum, printer_default->access_required)) {
 			DEBUG(3, ("access DENIED for printer open\n"));
 			close_printer_handle(p, handle);
 			return WERR_ACCESS_DENIED;
diff --git a/source3/smbd/password.c b/source3/smbd/password.c
index 5274028db4..784c1525c8 100644
--- a/source3/smbd/password.c
+++ b/source3/smbd/password.c
@@ -267,7 +267,7 @@ void add_session_user(const char *user)
 /****************************************************************************
 check if a username is valid
 ****************************************************************************/
-BOOL user_ok(const char *user,int snum)
+BOOL user_ok(const char *user,int snum, gid_t *groups, size_t n_groups)
 {
 	char **valid, **invalid;
 	BOOL ret;
@@ -278,7 +278,7 @@ BOOL user_ok(const char *user,int snum)
 	if (lp_invalid_users(snum)) {
 		str_list_copy(&invalid, lp_invalid_users(snum));
 		if (invalid && str_list_substitute(invalid, "%S", lp_servicename(snum))) {
-			ret = !user_in_list(user, (const char **)invalid);
+			ret = !user_in_list(user, (const char **)invalid, groups, n_groups);
 		}
 	}
 	if (invalid)
@@ -287,7 +287,7 @@ BOOL user_ok(const char *user,int snum)
 	if (ret && lp_valid_users(snum)) {
 		str_list_copy(&valid, lp_valid_users(snum));
 		if (valid && str_list_substitute(valid, "%S", lp_servicename(snum))) {
-			ret = user_in_list(user, (const char **)valid);
+			ret = user_in_list(user, (const char **)valid, groups, n_groups);
 		}
 	}
 	if (valid)
@@ -296,7 +296,7 @@ BOOL user_ok(const char *user,int snum)
 	if (ret && lp_onlyuser(snum)) {
 		char **user_list = str_list_make (lp_username(snum), NULL);
 		if (user_list && str_list_substitute(user_list, "%S", lp_servicename(snum))) {
-			ret = user_in_list(user, (const char **)user_list);
+			ret = user_in_list(user, (const char **)user_list, groups, n_groups);
 		}
 		if (user_list) str_list_free (&user_list);
 	}
@@ -315,7 +315,7 @@ static char *validate_group(char *group, DATA_BLOB password,int snum)
 		setnetgrent(group);
 		while (getnetgrent(&host, &user, &domain)) {
 			if (user) {
-				if (user_ok(user, snum) && 
+				if (user_ok(user, snum, NULL, 0) && 
 				    password_ok(user,password)) {
 					endnetgrent();
 					return(user);
@@ -370,7 +370,7 @@ static char *validate_group(char *group, DATA_BLOB password,int snum)
 			while (*member) {
 				static fstring name;
 				fstrcpy(name,member);
-				if (user_ok(name,snum) &&
+				if (user_ok(name,snum, NULL, 0) &&
 				    password_ok(name,password)) {
 					endgrent();
 					return(&name[0]);
@@ -429,7 +429,7 @@ BOOL authorise_login(int snum, fstring user, DATA_BLOB password,
 		     auser = strtok(NULL,LIST_SEP)) {
 			fstring user2;
 			fstrcpy(user2,auser);
-			if (!user_ok(user2,snum))
+			if (!user_ok(user2,snum, NULL, 0))
 				continue;
 			
 			if (password_ok(user2,password)) {
@@ -464,7 +464,7 @@ and given password ok (%s)\n", user));
 			} else {
 				fstring user2;
 				fstrcpy(user2,auser);
-				if (user_ok(user2,snum) && password_ok(user2,password)) {
+				if (user_ok(user2,snum, NULL, 0) && password_ok(user2,password)) {
 					ok = True;
 					fstrcpy(user,user2);
 					DEBUG(3,("authorise_login: ACCEPTED: user list username \
@@ -489,7 +489,7 @@ and given password ok (%s)\n", user));
 		*guest = True;
 	}
 
-	if (ok && !user_ok(user,snum)) {
+	if (ok && !user_ok(user, snum, NULL, 0)) {
 		DEBUG(0,("authorise_login: rejected invalid user %s\n",user));
 		ok = False;
 	}
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 5069db8097..2739f73b0a 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -573,7 +573,7 @@ static BOOL uid_entry_in_group( canon_ace *uid_ace, canon_ace *group_ace )
 	 * not uids/gids.
 	 */
 
-	return user_in_group_list(u_name, g_name );
+	return user_in_group_list(u_name, g_name, NULL, 0);
 }
 
 /****************************************************************************
diff --git a/source3/smbd/service.c b/source3/smbd/service.c
index 2a41a6db1c..f9d84872d7 100644
--- a/source3/smbd/service.c
+++ b/source3/smbd/service.c
@@ -258,7 +258,7 @@ static NTSTATUS share_sanity_checks(int snum, pstring dev)
 /****************************************************************************
  readonly share?
 ****************************************************************************/
-static void set_read_only(connection_struct *conn) 
+static void set_read_only(connection_struct *conn, gid_t *groups, size_t n_groups)
 {
 	char **list;
 	char *service = lp_servicename(conn->service);
@@ -271,7 +271,7 @@ static void set_read_only(connection_struct *conn)
 		if (!str_list_substitute(list, "%S", service)) {
 			DEBUG(0, ("ERROR: read list substitution failed\n"));
 		}
-		if (user_in_list(conn->user, (const char **)list))
+		if (user_in_list(conn->user, (const char **)list, groups, n_groups))
 			conn->read_only = True;
 		str_list_free(&list);
 	}
@@ -281,7 +281,7 @@ static void set_read_only(connection_struct *conn)
 		if (!str_list_substitute(list, "%S", service)) {
 			DEBUG(0, ("ERROR: write list substitution failed\n"));
 		}
-		if (user_in_list(conn->user, (const char **)list))
+		if (user_in_list(conn->user, (const char **)list, groups, n_groups))
 			conn->read_only = False;
 		str_list_free(&list);
 	}
@@ -291,7 +291,7 @@ static void set_read_only(connection_struct *conn)
 /****************************************************************************
   admin user check
 ****************************************************************************/
-static void set_admin_user(connection_struct *conn) 
+static void set_admin_user(connection_struct *conn, gid_t *groups, size_t n_groups)
 {
 	/* admin user check */
 	
@@ -299,7 +299,7 @@ static void set_admin_user(connection_struct *conn)
 	   marked read_only. Changed as I don't think this is needed,
 	   but old code left in case there is a problem here.
 	*/
-	if (user_in_list(conn->user,lp_admin_users(conn->service)) 
+	if (user_in_list(conn->user,lp_admin_users(conn->service), groups, n_groups) 
 #if 0
 	    && !conn->read_only
 #endif
@@ -370,7 +370,7 @@ static connection_struct *make_connection_snum(int snum, user_struct *vuser,
 				      return NULL;
 			}
 		} else {
-			if (!user_ok(vuser->user.unix_name, snum)) {
+			if (!user_ok(vuser->user.unix_name, snum, vuser->groups, vuser->n_groups)) {
 				DEBUG(2, ("user '%s' (from session setup) not permitted to access this share (%s)", vuser->user.unix_name, lp_servicename(snum)));
 				conn_free(conn);
 				*status = NT_STATUS_ACCESS_DENIED;
@@ -427,9 +427,9 @@ static connection_struct *make_connection_snum(int snum, user_struct *vuser,
 	string_set(&conn->user,user);
 	conn->nt_user_token = NULL;
 	
-	set_read_only(conn);
+	set_read_only(conn, vuser ? vuser->groups : NULL, vuser ? vuser->n_groups : 0);
 	
-	set_admin_user(conn);
+	set_admin_user(conn, vuser ? vuser->groups : NULL, vuser ? vuser->n_groups : 0);
 
 	/*
 	 * If force user is true, then store the
@@ -499,7 +499,7 @@ static connection_struct *make_connection_snum(int snum, user_struct *vuser,
 			 * Otherwise, the meaning of the '+' would be ignored.
 			 */
 			if (conn->force_user && user_must_be_member) {
-				if (user_in_group_list( user, gname )) {
+				if (user_in_group_list( user, gname, NULL, 0)) {
 						conn->gid = gid;
 						DEBUG(3,("Forced group %s for member %s\n",gname,user));
 				}
diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index e7c00ba456..4ebee75a15 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -60,7 +60,7 @@ BOOL change_to_guest(void)
 
 static BOOL check_user_ok(connection_struct *conn, user_struct *vuser,int snum)
 {
-	int i;
+	unsigned i;
 	for (i=0;i<conn->vuid_cache.entries && i< VUID_CACHE_SIZE;i++)
 		if (conn->vuid_cache.list[i] == vuser->vuid)
 			return(True);
@@ -70,7 +70,7 @@ static BOOL check_user_ok(connection_struct *conn, user_struct *vuser,int snum)
 		return False;
 	}
 	
-	if (!user_ok(vuser->user.unix_name,snum))
+	if (!user_ok(vuser->user.unix_name,snum, vuser->groups, vuser->n_groups))
 		return(False);
 
 	if (!share_access_check(conn, snum, vuser, conn->read_only ? FILE_READ_DATA : FILE_WRITE_DATA)) {