Bug 7777 - When requesting lookups for BUILTIN sids, winbindd allocates new uids/gids in error.
Summary: When requesting lookups for BUILTIN sids, winbindd allocates new uids/gids in...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 3.5.6
Hardware: All All
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
: 7650 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-11-05 13:45 UTC by Jeremy Allison
Modified: 2020-12-04 09:13 UTC (History)
2 users (show)

See Also:


Attachments
First part of fix. (1.03 KB, patch)
2010-11-05 14:16 UTC, Jeremy Allison
obnox: review+
Details
Second part of fix. (2.38 KB, patch)
2010-11-05 14:16 UTC, Jeremy Allison
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2010-11-05 13:45:04 UTC
There are actually 2 problems here I am fixing. Problem 1 is that the passdb module is not correctly initialized. Problem 2 is that for SIDs in domains which the machine is authoritative for, BUILTIN and our own SAM SID, winbindd is falling back to allocation when it should be returning no mappings.

To reproduce:

Join a freshly installed machine to a domain as a member server. Clean out all the winbindd mappings. As root:

cd /usr/local/samba/var
rm -rf log.*
rm -rf locks/netsamlogon_cache.tdb 
rm -rf locks/gencache*
rm -rf locks/winbindd_*
rm -rf locks/group_mapping.ldb 

Then start winbindd:

../sbin/winbindd -d10

Now try and look up a SID from the BUILTIN domain (BUILTIN\Administrators):

wbinfo -Y S-1-5-32-544

winbindd will return a new mapped gid. It should return :
Could not convert sid S-1-5-32-544 to gid

Patch for 3.5.7 follows. The code in master and 3.6.0 is different, but the fix is similar.

Jeremy.
Comment 1 Jeremy Allison 2010-11-05 14:16:03 UTC
Created attachment 6048 [details]
First part of fix.
Comment 2 Jeremy Allison 2010-11-05 14:16:25 UTC
Created attachment 6049 [details]
Second part of fix.
Comment 3 Jeremy Allison 2010-11-05 14:17:24 UTC
Volker, Michael, please review and assign to Karolin for inclusion in 3.5.7 if you are happy.
Thanks !
Jeremy.
Comment 4 Michael Adam 2010-11-05 17:45:23 UTC
Comment on attachment 6048 [details]
First part of fix.

This patch is good.

Somehow the patch makes me feel that we should move the initialization of the standard domains (default, passdb) into a more central place. this seems so scattered. But for now this is definitely good.
Comment 5 Michael Adam 2010-11-05 17:47:23 UTC
(In reply to comment #4)
> (From update of attachment 6048 [details])
> This patch is good.
> 
> Somehow the patch makes me feel that we should move the initialization of the
> standard domains (default, passdb) into a more central place. this seems so
> scattered. But for now this is definitely good.

This part of the fix should go to master/3.6, too, I guess.
Comment 6 Michael Adam 2010-11-05 18:13:39 UTC
Comment on attachment 6049 [details]
Second part of fix.

The patch looks good, but this is code that deceives easily. I would like to perform some tests. Coming back to this later...
Comment 7 Jeremy Allison 2010-11-05 19:35:46 UTC
I've figured out the issue with master/v3-6-test and the similar patch. It's actually a provisioning issue now winbindd doesn't have a passdb backend. In the server we return a users primary group as "DOMAIN\Domain Users", so when the dc winbindd tests try and look up a user it fails unless the S-1-5-32-<Domain SID>-513 group is mapped to a UNIX group.

So the logic in this code is correct, it's the test that isn't correct for master and v3-6-test. I'll look into fixing that.

Jeremy.
Comment 8 Jeremy Allison 2010-11-05 21:59:28 UTC
Been thinking more about this and I think I may have a more elegant fix for master/v3-6-test.
Jeremy.
Comment 9 Jeremy Allison 2010-11-05 22:54:13 UTC
Ok, I have one more fix for 3.5.x - in the DC case we need to ensure we have mappings for :

Domain Admins = S-1-5-32-<domain>-512
Domain Users = S-1-5-32-<domain>-513
Domain Guests = S-1-5-32-<domain>-514

We should pre-create these on startup, like we pre-create S-1-5-32-544.

Third patch to follow

Jeremy.
Comment 10 Jeremy Allison 2010-11-06 00:22:42 UTC
Arg. Elegant fix doesn't work :-(. Due to the horrible hack handling of group 513 ("Domain Users") pre-creating the groups programatically still causes the tests to fail in the DC case. Still thinking about the correct fix - maybe back to fixing the test provisioning for "Domain Users" :-(.
Jeremy.
Comment 11 Jeremy Allison 2010-11-08 16:44:26 UTC
Ok, I fixed the test provisioning a DC - ensured that there is always a "Domain Users" mapped group and all tests now pass. Code now checked into master and v3-6-test.

I think this patch is correct for 3.5.7 - so long as for the DC case there's a UNIX group mapped for Domain Users.

Vl and Obnox, please review and re-assign to Karolin if you're happy with this.

Jeremy.
Comment 12 Michael Adam 2010-11-08 16:50:08 UTC
(In reply to comment #11)
> Ok, I fixed the test provisioning a DC - ensured that there is always a "Domain
> Users" mapped group and all tests now pass. Code now checked into master and
> v3-6-test.

Cool!

> I think this patch is correct for 3.5.7 - so long as for the DC case there's a
> UNIX group mapped for Domain Users.
> 
> Vl and Obnox, please review and re-assign to Karolin if you're happy with this.

Ok, will do shortly. Just wanted to be sure about the modified check (specific domain request) in that 3.5 code... :-)

Cheers - Michael
Comment 13 Michael Adam 2010-11-15 05:14:27 UTC
Comment on attachment 6049 [details]
Second part of fix.

Yep. I finally managed to test this. Looks good!
Comment 14 Michael Adam 2010-11-15 05:15:24 UTC
Karo,
Please apply the two patches for 3.5.7.
Thanks - Michael
Comment 15 Karolin Seeger 2010-11-16 08:49:28 UTC
Pushed both patches to v3-5-test.
Closing out bug report.

Thanks!
Comment 16 Kevin Shanahan (dead mail address) 2011-02-10 17:52:50 UTC
*** Bug 7650 has been marked as a duplicate of this bug. ***