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)

diff --git a/source3/lib/username.c b/source3/lib/username.c
index b1c9ca0f08a127870bd2a7b5d3cf6b253d4a2655..b8f33494ee4ad53c27a3b51be836cd284eb6767b 100644 (file)
--- 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 17c7cc29fe4be33799bc5361aae6cd0a5b072801..cd906d37c00a78dc8732b07c39fcfc2beb37cabc 100644 (file)
--- 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 10d490a4c1f69cdc756aafce08f935e1b46bd530..836324ecc934a33c653fa09b2a3b14d10c45e05d 100644 (file)
--- 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 2896fd79e40b8aa48caf16aa5d8ccb6df243a43b..d766e9c19efe34c6c5ae43511d08d02e269aa6a2 100644 (file)
--- 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 0bcc3c5a30c1226f78b56fd7cc54b5574714b032..3a7ced7725411faf20d69fff559b516ca156c803 100644 (file)
--- 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 5274028db4a4854a518b52cc0344cbfd23c7f538..784c1525c897162da24f800f046bd0024837a7d5 100644 (file)
--- 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 5069db8097b734aeef8310fe6f45204c389bcac6..2739f73b0ac30010edaced30d457e6b47a110100 100644 (file)
--- 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 2a41a6db1c509a550b7ac041b50fd3b9a52f257e..f9d84872d71f4e924794b17fbc95ff43f97a9bd2 100644 (file)
--- 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 e7c00ba45606c546d1004f52f7bd51f391fc114f..4ebee75a159e32fc851c3ec579909573fe2f9c02 100644 (file)
--- 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)) {