[PATCH 1/2] s3-auth: fix force user for AD user

[Andreas Schneider]

On Tuesday 15 November 2011 09:18:26 Andrew Bartlett wrote:
> On Mon, 2011-11-14 at 17:31 +0100, David Disseldorp wrote:
> > Do not add a unix_users_domain_name() username prefix prior to the
> > lookup. This ensures winbind is consulted before a unix user SID is
> > manually composed.
> > 
> > Use get_primary_group_sid() only if gid_to_sid() fails lookup.
> 
> https://bugzilla.samba.org/show_bug.cgi?id=8598
> 
> > ---
> > 
> >  source3/auth/auth_util.c    |   18 ++++++++----------
> >  source3/passdb/lookup_sid.c |    3 +--
> >  2 files changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/source3/auth/auth_util.c b/source3/auth/auth_util.c
> > index fcfed83..f21cbe1 100644
> > --- a/source3/auth/auth_util.c
> > +++ b/source3/auth/auth_util.c
> > @@ -677,9 +677,9 @@ NTSTATUS make_server_info_pw(struct
> 
> auth_serversupplied_info **server_info,
> 
> >  {
> >  
> >  	NTSTATUS status;
> >  	struct samu *sampass = NULL;
> > 
> > -	char *qualified_name = NULL;
> > 
> >  	TALLOC_CTX *mem_ctx = NULL;
> >  	struct dom_sid u_sid;
> > 
> > +	struct dom_sid g_sid;
> > 
> >  	enum lsa_SidType type;
> >  	struct auth_serversupplied_info *result;
> > 
> > @@ -701,15 +701,7 @@ NTSTATUS make_server_info_pw(struct
> 
> auth_serversupplied_info **server_info,
> 
> >  		return NT_STATUS_NO_MEMORY;
> >  	
> >  	}
> > 
> > -	qualified_name = talloc_asprintf(mem_ctx, "%s\\%s",
> > -					unix_users_domain_name(),
> > -					unix_username );
> > -	if (!qualified_name) {
> > -		TALLOC_FREE(mem_ctx);
> > -		return NT_STATUS_NO_MEMORY;
> > -	}
> > -
> > -	if (!lookup_name(mem_ctx, qualified_name, LOOKUP_NAME_ALL,
> > +	if (!lookup_name(mem_ctx, unix_username, LOOKUP_NAME_ALL,
> > 
> >  						NULL, NULL,
> >  						&u_sid, &type)) {
> >  		
> >  		TALLOC_FREE(mem_ctx);
> > 
> > @@ -739,6 +731,12 @@ NTSTATUS make_server_info_pw(struct
> 
> auth_serversupplied_info **server_info,
> 
> >  	/* set the user sid to be the calculated u_sid */
> >  	pdb_set_user_sid(sampass, &u_sid, PDB_SET);
> 
> As regards the first part of the patch, I will simply note 'there be
> dragons', and to take care not to break forcing a unix user when
> allowing an AD user to be forced, and the valid/invalid user stuff.  In
> particular, check when winbindd is not in use for nsswitch.
> 
> Because of historical smb.conf requirements, we have not kept a very
> good separation between 'windows style names' and simple unix usernames
> here.
> 
> This code needs a continuing refactor - with the introduction of the
> 'unix domain', there needs to be a clearer separation or layering,
> between the codepath as used the keep smbpasswd files working (in
> make_samu_pw()), the codepath for krb5 logins when not part of AD and
> security=server, and the codepath used for things like force user.
> 
> Are you able to work out where this part of the regression occurred?

I've tried to find the regression but no change, it must be in Samba 3.2 or 
something like that. However Günther and I looked very long at the code and we 
are confident that the part above is fixed with the attached patch.

See also create_token_from_username() introduced by Andrew with
1c3c5e2156d9096f60bd53a96b88c2f1001d898a

> > +	/* samu_to_SamInfo3() calls get_primary_group_sid() if mapping fails */
> > +	gid_to_sid(&g_sid, pwd->pw_gid);
> > +	if (!is_null_sid(&g_sid)) {
> > +		pdb_set_group_sid(sampass, &g_sid, PDB_SET);
> > +	}
> > +
> > 
> >  	result = make_server_info(NULL);
> >  	if (result == NULL) {
> >  	
> >  		TALLOC_FREE(sampass);
> 
> This second part seems reasonable - gid_to_sid logic was once inside the
> passdb pdb_get_primary_group_sid code, and would explain the regression
> (this area was changed for 3.6), but again it would be good if you could
> work out when this regressed, so we can better understand this delicate
> part of the system.

This part is the 'here be dragons' part. It is needed cause of the following 
codepath:

samu_to_SamInfo3() -> pdb_get_group_sid() -> get_primary_group_sid()


        gid_to_sid(group_sid, pwd->pw_gid);
        if (!is_null_sid(group_sid)) {
                struct dom_sid domain_sid;
                uint32_t rid;

                /* We need a sid within our domain */
                sid_copy(&domain_sid, group_sid);
                sid_split_rid(&domain_sid, &rid);
                if (dom_sid_equal(&domain_sid, get_global_sam_sid())) {
                        /*
                         * As shortcut for the expensive lookup_sid call
                         * compare the domain sid part
                         */
                        switch (rid) {
                        case DOMAIN_RID_ADMINS:
                        case DOMAIN_RID_USERS:
                                goto done;
                        default:
                                need_lookup_sid = true;
                                break;
                        }
                } else {
                        /* Try group mapping */
                        ZERO_STRUCTP(group_sid);
                        if (pdb_gid_to_sid(pwd->pw_gid, group_sid)) {
                                need_lookup_sid = true;
                        }
                }
        }

if (dom_sid_equal(&domain_sid, get_global_sam_sid()))

The domain sid of a AD user is not the global sam sid, so we hit the else 
branch here and pdb_gid_to_sid() is the wrong call here for an AD user. So 
either the hunk above is correct or we need to fix get_primary_group_sid().


Please help!


	-- andreas

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-auth-Fix-force-user-with-ADS-member.patch
Type: text/x-patch
Size: 2234 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20131128/99c0107a/attachment.bin>