New approach to "valid users" fix

Gerald (Jerry) Carter jerry at samba.org
Fri Aug 11 15:21:44 GMT 2006


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeremy and Volker,

I think we've been dealing with the "valid users" problem
in smb.conf the wrong way.  The latest bug report made
it pretty obvious.  The name2sid() and gid2sid() methods
are not symmetric.

What happens is that when using smbpasswd and no group
mappings, the user's token gets created using the
pdb_rid_algorithm() via the gid_to_sid() calls.

NT user token of user S-1-5-21-391507597-2097566357-2340928898-3008
contains 6 SIDs
SID[  0]: S-1-5-21-391507597-2097566357-2340928898-3008
SID[  1]: S-1-5-21-391507597-2097566357-2340928898-1201
SID[  2]: S-1-1-0
SID[  3]: S-1-5-2
SID[  4]: S-1-5-11
SID[  5]: S-1-5-21-391507597-2097566357-2340928898-3091

lookup_name_smbconf() tries to resolve the name directly
to a SID and so the group "users" appears as S-1-5-21-....-1201
in the token but is S-1-22-2-100 from lookup_name.

What I have done is to split lookup_name_smbconf() into
two functions, one for users and one for groups, since
we always know in the caller what type of SID we need.

The new lookup_{user,group}_smbconf converts the name
from a uid/gid to a SID.  And then calls lookup_sid() to
get the type.  I had to fix our lookup_global_sam_rid()
function to deal with backends having pdb_rid_algorithm()
support.

This also means that we can remove the LOOKUP_NAME_GROUP
hack from lookup_name() as we convert valid users = +users
to a gid and then to a SID instead of guessing which domain
it should be from.

This works for some basic standalone/smbpasswd scenarios.
I'm still testing more complex setups such as member servers
and DCs.

I think will also fix the ACL editor problem with smbpasswd
backends where it was displaying a raw SID.



cheers, jerry
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFE3KCIIR7qMdg1EfYRAjDiAJ9eQXP2yYsP9hsbgcYAN2ClfXvl4QCgize4
HzQTELQg2TCA1/huhhUzdG0=
=6VPx
-----END PGP SIGNATURE-----
-------------- next part --------------
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/auth/auth_util.c samba-3.0.23b-patched/source/auth/auth_util.c
--- samba-3.0.23b/source/auth/auth_util.c	2006-08-07 11:46:33.000000000 -0500
+++ samba-3.0.23b-patched/source/auth/auth_util.c	2006-08-11 10:03:44.000000000 -0500
@@ -1052,9 +1052,8 @@
 		return NT_STATUS_NO_MEMORY;
 	}
 
-	if (!lookup_name_smbconf(tmp_ctx, username, LOOKUP_NAME_ALL,
-			 NULL, NULL, &user_sid, &type)) {
-		DEBUG(1, ("lookup_name_smbconf for %s failed\n", username));
+	if (!lookup_user_smbconf(tmp_ctx, username, &user_sid, &type)) {
+		DEBUG(1, ("lookup_user_smbconf(%s) failed\n", username));
 		goto done;
 	}
 
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/include/smb.h samba-3.0.23b-patched/source/include/smb.h
--- samba-3.0.23b/source/include/smb.h	2006-07-10 11:27:52.000000000 -0500
+++ samba-3.0.23b-patched/source/include/smb.h	2006-08-11 10:03:44.000000000 -0500
@@ -272,7 +272,7 @@
 #define LOOKUP_NAME_REMOTE   2  /* Ask others */
 #define LOOKUP_NAME_ALL (LOOKUP_NAME_ISOLATED|LOOKUP_NAME_REMOTE)
 
-#define LOOKUP_NAME_GROUP    4  /* This is a NASTY hack for valid users = @foo
+#define LOOKUP_NAME_GROUP    4  /* (unused) This is a NASTY hack for valid users = @foo
 				 * where foo also exists in as user. */
 
 /**
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/passdb/lookup_sid.c samba-3.0.23b-patched/source/passdb/lookup_sid.c
--- samba-3.0.23b/source/passdb/lookup_sid.c	2006-08-07 11:46:33.000000000 -0500
+++ samba-3.0.23b-patched/source/passdb/lookup_sid.c	2006-08-11 10:03:44.000000000 -0500
@@ -120,63 +120,6 @@
 		goto failed;
 	}
 
-	/*
-	 * Nasty hack necessary for too common scenarios:
-	 *
-	 * For 'valid users = +users' we know "users" is most probably not
-	 * BUILTIN\users but the unix group users. This hack requires the
-	 * admin to explicitly qualify BUILTIN if BUILTIN\users is meant.
-	 *
-	 * Please note that LOOKUP_NAME_GROUP can not be requested via for
-	 * example lsa_lookupnames, it only comes into this routine via
-	 * the expansion of group names coming in from smb.conf
-	 */
-
-	if ((flags & LOOKUP_NAME_GROUP) && ((grp = getgrnam(name)) != NULL)) {
-
-		GROUP_MAP map;
-
-		if (pdb_getgrgid(&map, grp->gr_gid)) {
-			/* The hack gets worse. Handle the case where we have
-			 * 'force group = +unixgroup' but "unixgroup" has a
-			 * group mapping */
-
-			if (sid_check_is_in_builtin(&map.sid)) {
-				domain = talloc_strdup(
-					tmp_ctx, builtin_domain_name());
-			} else {
-				domain = talloc_strdup(
-					tmp_ctx, get_global_sam_name());
-			}
-
-			sid_copy(&sid, &map.sid);
-			type = map.sid_name_use;
-			goto ok;
-		}
-
-		/* If we are using the smbpasswd backend, we need to use the
-		 * algorithmic mapping for the unix group we find. This is
-		 * necessary because when creating the NT token from the unix
-		 * gid list we got from initgroups() we use gid_to_sid() that
-		 * uses algorithmic mapping if pdb_rid_algorithm() is true. */
-
-		if (pdb_rid_algorithm() &&
-		    (grp->gr_gid < max_algorithmic_gid())) {
-			domain = talloc_strdup(tmp_ctx, get_global_sam_name());
-			sid_compose(&sid, get_global_sam_sid(),
-				    pdb_gid_to_group_rid(grp->gr_gid));
-			type = SID_NAME_DOM_GRP;
-			goto ok;
-		}
-		
-		if (lookup_unix_group_name(name, &sid)) {
-			domain = talloc_strdup(tmp_ctx,
-					       unix_groups_domain_name());
-			type = SID_NAME_DOM_GRP;
-			goto ok;
-		}
-	}
-
 	/* Now the guesswork begins, we haven't been given an explicit
 	 * domain. Try the sequence as documented on
 	 * http://msdn.microsoft.com/library/en-us/secmgmt/security/lsalookupnames.asp
@@ -357,71 +300,56 @@
 }
 
 /************************************************************************
- Names from smb.conf can be unqualified. eg. valid users = foo
- These names should never map to a remote name. Try global_sam_name()\foo,
- and then "Unix Users"\foo (or "Unix Groups"\foo).
 ************************************************************************/
 
-BOOL lookup_name_smbconf(TALLOC_CTX *mem_ctx,
-		 const char *full_name, int flags,
-		 const char **ret_domain, const char **ret_name,
-		 DOM_SID *ret_sid, enum SID_NAME_USE *ret_type)
+BOOL lookup_user_smbconf( TALLOC_CTX *mem_ctx, const char *full_name,
+                           DOM_SID *ret_sid, enum SID_NAME_USE *ret_type)
 {
-	char *qualified_name;
-	const char *p;
-
-	/* NB. No winbindd_separator here as lookup_name needs \\' */
-	if ((p = strchr_m(full_name, *lp_winbind_separator())) != NULL) {
-
-		/* The name is already qualified with a domain. */
-
-		if (*lp_winbind_separator() != '\\') {
-			char *tmp;
-
-			/* lookup_name() needs '\\' as a separator */
+	struct passwd *pw;
 
-			tmp = talloc_strdup(mem_ctx, full_name);
-			if (!tmp) {
-				return False;
-			}
-			tmp[p - full_name] = '\\';
-			full_name = tmp;
-		}
-
-		return lookup_name(mem_ctx, full_name, flags,
-				ret_domain, ret_name,
-				ret_sid, ret_type);
+	if ( (pw = sys_getpwnam( full_name ) ) == NULL ) {
+		DEBUG(5,("lookup_user_smbconf: Failed to convert %s to a uid.\n", 
+			full_name));
+		return False;
 	}
-
-	/* Try with our own SAM name. */
-	qualified_name = talloc_asprintf(mem_ctx, "%s\\%s",
-				get_global_sam_name(),
-				full_name );
-	if (!qualified_name) {
+	
+	uid_to_sid( ret_sid, pw->pw_uid );
+	
+	if ( !lookup_sid( mem_ctx, ret_sid, NULL, NULL, ret_type ) ) {
+		DEBUG(0,("lookup_user_smbconf: lookup_sid() failed!  This should not happen!\n"));
 		return False;
 	}
+	
+	return True;	
+}
 
-	if (lookup_name(mem_ctx, qualified_name, flags,
-				ret_domain, ret_name,
-				ret_sid, ret_type)) {
-		return True;
-	}
+/************************************************************************
+************************************************************************/
 
-	/* Finally try with "Unix Users" or "Unix Group" */
-	qualified_name = talloc_asprintf(mem_ctx, "%s\\%s",
-				flags & LOOKUP_NAME_GROUP ?
-					unix_groups_domain_name() :
-					unix_users_domain_name(),
-				full_name );
-	if (!qualified_name) {
+BOOL lookup_group_smbconf( TALLOC_CTX *mem_ctx, const char *full_name,
+                           DOM_SID *ret_sid, enum SID_NAME_USE *ret_type)
+{
+	struct group *grp;
+	
+	if ( (grp = sys_getgrnam( full_name ) ) == NULL ) {
+		DEBUG(5,("lookup_group_smbconf: Failed to convert %s to a gid.\n", 
+			full_name));
 		return False;
 	}
-
-	return lookup_name(mem_ctx, qualified_name, flags,
-				ret_domain, ret_name,
-				ret_sid, ret_type);
+	
+	gid_to_sid( ret_sid, grp->gr_gid );
+	
+	if ( !lookup_sid( mem_ctx, ret_sid, NULL, NULL, ret_type ) ) {
+		DEBUG(0,("lookup_group_smbconf: lookup_sid() failed!  This should not happen!\n"));
+		return False;
+	}
+	
+	return True;	
 }
 
+/************************************************************************
+************************************************************************/
+
 static BOOL winbind_lookup_rids(TALLOC_CTX *mem_ctx,
 				const DOM_SID *domain_sid,
 				int num_rids, uint32 *rids,
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/passdb/pdb_interface.c samba-3.0.23b-patched/source/passdb/pdb_interface.c
--- samba-3.0.23b/source/passdb/pdb_interface.c	2006-07-21 11:22:57.000000000 -0500
+++ samba-3.0.23b-patched/source/passdb/pdb_interface.c	2006-08-11 10:03:44.000000000 -0500
@@ -1532,20 +1532,53 @@
 
 		return True;
 	}
-	
-	/* Windows will always map RID 513 to something.  On a non-domain 
-	   controller, this gets mapped to SERVER\None. */
 
-	if ( unix_id ) {
-		DEBUG(5, ("Can't find a unix id for an unmapped group\n"));
-		return False;
+	/* if we don't need a bvalid uid or gid, just see if 
+	   we can resolve the name. Windows will always map 
+	   RID 513 to something.  On a non-domain controller, 
+	   this gets mapped to SERVER\None. */
+	   
+	if ( !unix_id && rid == DOMAIN_GROUP_RID_USERS ) {
+		*name = talloc_strdup(mem_ctx, "None" );
+		*psid_name_use = SID_NAME_DOM_GRP;
+		
+		return True;
 	}
 	
-	if ( rid == DOMAIN_GROUP_RID_USERS ) {
-		*name = talloc_strdup(mem_ctx, "None" );
+	/* we haven't resolved the RID at this point.  But 
+	   if the passdb is using a RID algorithm, we should
+	   just apply the invese function */
+	
+	if ( pdb_rid_algorithm() ) {
+		uid_t uid;
+		gid_t gid;
+		struct passwd *pw;
+		struct group *grp;
+	
+		/* Is it is a user? */
+		
+		if ( algorithmic_pdb_rid_is_user(rid) ) {
+			uid = algorithmic_pdb_user_rid_to_uid( rid );
+			if ( (pw = sys_getpwuid(uid) ) == NULL ) {
+				return False;
+			}
+			*name = talloc_strdup(mem_ctx, pw->pw_name );
+			*psid_name_use = SID_NAME_USER;
+			
+			return True;
+		}
+		
+		/* else it's a group */
+		
+		gid = pdb_group_rid_to_gid( rid );
+		if ( (grp = getgrgid( gid )) == NULL ) {
+			return False;
+		}
+		*name = talloc_strdup(mem_ctx, grp->gr_name );
 		*psid_name_use = SID_NAME_DOM_GRP;
 		
 		return True;
+		
 	}
 
 	return False;
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/smbd/service.c samba-3.0.23b-patched/source/smbd/service.c
--- samba-3.0.23b/source/smbd/service.c	2006-08-07 11:46:33.000000000 -0500
+++ samba-3.0.23b-patched/source/smbd/service.c	2006-08-11 10:03:44.000000000 -0500
@@ -443,11 +443,8 @@
 	groupname = talloc_string_sub(mem_ctx, groupname,
 				      "%S", lp_servicename(snum));
 
-	if (!lookup_name_smbconf(mem_ctx, groupname,
-			 LOOKUP_NAME_ALL|LOOKUP_NAME_GROUP,
-			 NULL, NULL, &group_sid, &type)) {
-		DEBUG(10, ("lookup_name_smbconf(%s) failed\n",
-			   groupname));
+	if (!lookup_group_smbconf(mem_ctx, groupname, &group_sid, &type)) {
+		DEBUG(10, ("lookup_group_smbconf(%s) failed\n", groupname));
 		goto done;
 	}
 
diff -urN --exclude-from=/home/drizzt/jerry/tmp/diff.excludes samba-3.0.23b/source/smbd/share_access.c samba-3.0.23b-patched/source/smbd/share_access.c
--- samba-3.0.23b/source/smbd/share_access.c	2006-08-07 11:46:33.000000000 -0500
+++ samba-3.0.23b-patched/source/smbd/share_access.c	2006-08-11 10:03:44.000000000 -0500
@@ -94,8 +94,7 @@
 	}
 
 	if (!do_group_checks(&name, &prefix)) {
-		if (!lookup_name_smbconf(mem_ctx, name, LOOKUP_NAME_ALL,
-				 NULL, NULL, &sid, &type)) {
+		if (!lookup_user_smbconf(mem_ctx, name, &sid, &type)) {
 			DEBUG(5, ("lookup_name %s failed\n", name));
 			return False;
 		}
@@ -109,9 +108,7 @@
 
 	for (/* initialized above */ ; *prefix != '\0'; prefix++) {
 		if (*prefix == '+') {
-			if (!lookup_name_smbconf(mem_ctx, name,
-					 LOOKUP_NAME_ALL|LOOKUP_NAME_GROUP,
-					 NULL, NULL, &sid, &type)) {
+			if (!lookup_group_smbconf(mem_ctx, name, &sid, &type)) {
 				DEBUG(5, ("lookup_name %s failed\n", name));
 				return False;
 			}


More information about the samba-technical mailing list