[PATCH] s3: Lookup unknown SIDs in get_primary_group_sid

[Andrew Bartlett]

On Fri, 2012-07-06 at 13:30 -0600, Christof Schmitt wrote:
> christof.schmitt at us.ibm.com wrote on 06/27/2012 12:44:18 PM:
> 
> > When Samba is running as AD member using winbindd for id lookups,
> > each user automatically gets the privilege of the group 'Domain
> > Users'. This happens even when the user has been removed from the
> > group 'Domain Users'.
> > 
> > A trace shows that get_primary_group_sid forces the primary group
> > to be 'Domain Users':
> > 
> > [2012/06/27 21:05:18.700197,  5] lib/username.c:171(Get_Pwnam_alloc)
> >   Finding user VIRTUAL1\testuser1
> > [2012/06/27 21:05:18.700232,  5] lib/username.c:116(Get_Pwnam_internals)
> >   Trying _Get_Pwnam(), username as lowercase is virtual1\testuser1
> > [2012/06/27 21:05:18.700268,  5] lib/username.c:149(Get_Pwnam_internals)
> >   Get_Pwnam_internals did find user [VIRTUAL1\testuser1]!
> > [2012/06/27 21:05:18.700335, 10] passdb/lookup_sid.c:1414(gid_to_sid)
> >   gid 13000514 -> sid S-1-5-21-531246827-3739281486-2559166756-514
> > [2012/06/27 21:05:18.700703, 10] groupdb/mapping_tdb.c:235(find_map)
> >   failed to unpack map
> > [2012/06/27 21:05:18.700950, 10] groupdb/mapping_tdb.c:235(find_map)
> >   failed to unpack map
> > [2012/06/27 21:05:18.701076,  3] 
> > passdb/lookup_sid.c:1759(get_primary_group_sid)
> >   Forcing Primary Group to 'Domain Users' for VIRTUAL1\testuser1
> > 
> > This is caused by get_primary_group_sid calling pdb_gid_to_sid to
> > determine if the group is a mapped group:
> > 
> >                         /* Try group mapping */
> >                         ZERO_STRUCTP(group_sid);
> >                         if (pdb_gid_to_sid(pwd->pw_gid, group_sid)) {
> >                                 need_lookup_sid = true;
> >                         }
> > 
> > Since there is no mapping for arbitrary groups that can be set as
> > primary group, this check fails and get_primary_group_sid reverts
> > to forcing 'Domain Users' as default group.
> > 
> > The attached patch removes this check to let the following code
> > verify the group with lookup_sid. With this patch, access to
> > resources only available to 'Domain Users' is denied for user ids
> > that are not members of the group.
> > 
> > Is this a valid approach to solve the problem?
> 
> Ping. Any comments on the patch?

My main thoughts are 'here be dragons'.  I've not looked over the full
implications, and I do want to encourage cleaning up this code, but my
warning is this:
 - The full complexity in this area has built up over time, with lots of
piecemeal patches to solve one-off issues
 - There is additional complexity from the fact that folks insist on
running both with and without winbind when we are a domain member
 - There are few automated tests for this area.

A patch with an addition to 'make test' to prove it would be much easier
to accept, because even if we do change behaviour, at least we would lay
down a baseline for what the behaviour should be.

In doing that, look closely at the unix.whoami smbtorture test in
source4/torture/unix/whomai.c.  This particular Samba-only protocol
extension gives you an incredibly valuable insight, previously only
available on LDAP against a windows DC:  The user's full token of
SIDs.  

As an example way forward, you could modify the test so that instead of
having the 'addc' be a boolean argument, it could be the string name of
the DC to compare with, and we could assert in this case that the domain
SIDs match.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org