WINBINDD_LOOKUPRIDS is a lookup call, so in winbindd_lookuprids.c the find_domain_from_sid is wrong.
Created attachment 6109 [details] Patch for 3.5
Bug and Patch also apply to master/3.6.
Comment on attachment 6109 [details] Patch for 3.5 LGTM!
Re-assigning to Karolin for inclusion in 3.5.next. Jeremy.
Pushed to v3-5-test. Closing out bug report. Thanks!
Hi guys, It seems for me that this fix has broken LOOKUP-RIDS functionality. Winbind always uses PRIMARY domain instead of domain-resolved-from-SID. Since only RID is used in the request (am I mistaken?), winbind is trying to resolve RID via PRIMARY domain (using PRIMARY domain's SID prefix); as the result, when resolving RID from some TRUSTED domain - we have 'not found' error (or wrong name in the worst case). Tested on samba 3.5.8: Samba server is joined to 'QA.DELL-IDC.COM' AD domain. 'DELL-IDC.COM' is a trusted domain. [root@LG-D21A-LNX samba]# ./bin/wbinfo --name-to-sid 'dell-idc\volodymyr' S-1-5-21-124719809-1973235383-4081168169-1172 SID_USER (1) [root@LG-D21A-LNX samba]# ./bin/wbinfo --domain=dell-idc --lookup-rids 1172 winbind_lookup_rids failed: WBC_ERR_DOMAIN_NOT_FOUND Could not lookup RIDs 1172 Here PRIMARY domain 'QA' have been searched for RID=1172. QA domain has no such RID, 'not found' error. The worst case: [root@LG-D21A-LNX samba]# ./bin/wbinfo --sid-to-name S-1-5-21-4231570626-2605438007-4103547854-1101 QA\DELL-IDC$ 1 [root@LG-D21A-LNX samba]# ./bin/wbinfo --sid-to-name S-1-5-21-124719809-1973235383-4081168169-1101 DELL-IDC\DnsAdmins 4 [root@LG-D21A-LNX samba]# ./bin/wbinfo --domain=DELL-IDC --lookup-rids 1101 Domain: QA 1101: DELL-IDC$ (SID_USER) Here 'QA' and 'DELL-IDC' domains both has RID=1101. When asked for RID from DELL-IDC domain, winbind is replying by the result from PRIMARY 'QA' domain (wrong reply). Due this bug Samba cannot resolve trusted domains' SIDs from attached ACLs (when viewing ACL in 'Security' tab of file properties).
As this can return an incorrect name for access checks, and affects 3.6.0, I'm raising to "blocker" for 3.6.0 until we confirm. Re-opening and assigning to Volker. Jeremy.
As a first glance, the logic in find_lookup_domain_from_sid() and find_lookup_domain_from_name() doesn't look quite right to me. Investigating. This is my fault as the reviewer of the original patch. Please accept my apologies. Re-assigning to Volker. Volker, do you remember the exact case the original patch fixed ? Jeremy.
No, checked very carefully and the logic in find_lookup_domain_from_sid() and find_lookup_domain_from_name() is completely correct. Looks like the bug is that when doing a lookup on rids as a member server, we can't ask the local Domain Controller to do this as it can only look up rids on the domain handles it is authoritative for (it's own domain sid and builtin). I think winbindd must ask the trusted DC's directly when doing rid lookups, not SID lookups - which means the original change moving to find_ookup_domain_from_sid() from find_domain_from_sid_noinit() was incorrect. I should have seen that in the review :-(. Volker, did you have a specific test case you were fixing with the original change ? Jeremy.
Created attachment 6604 [details] git-am fix for 3.6.0
Created attachment 6605 [details] git-am fix for 3.5.next Same fix for 3.5.next. Volodymyr, can you test this fix in your 3.5.x setup and see if it does the right thing ? Jeremy.
(In reply to comment #9) > No, checked very carefully and the logic in find_lookup_domain_from_sid() and > find_lookup_domain_from_name() is completely correct. > > Looks like the bug is that when doing a lookup on rids as a member server, we > can't ask the local Domain Controller to do this as it can only look up rids on > the domain handles it is authoritative for (it's own domain sid and builtin). > > I think winbindd must ask the trusted DC's directly when doing rid lookups, not > SID lookups - which means the original change moving to > find_ookup_domain_from_sid() from find_domain_from_sid_noinit() was incorrect. > I should have seen that in the review :-(. I haven't looked at the code carefully but a client should never need to contact a trusted DC, it may not even have access to it. What is not clear to me is why are we doing RID lookups at all against another server, why not always turn the request into a sid2name lookup ?
> Volodymyr, can you test this fix in your 3.5.x setup and see > if it does the right thing ? Tested, now it is fine - lookup-rids returns the same domain as requested: [root@node0-vmca238 ~]# wbinfo --domain=DELL-IDC --lookup-rids 1101 Domain: DELL-IDC 1101: DnsAdmins (SID_ALIAS) > Client should never need to > contact a trusted DC, it may not even have access to it. In such case original domain SID prefix (or domain name) must be passed to PRIMARY DC in request. In that case it will be able to resolve RID against specified domain (and not against own domain). > What is not clear to me is why are we doing RID lookups at all > against another server, why not always turn the request into a sid2name lookup ? LOOKUP-RIDS is able to process multiple RIDs per a request (within single domain). Is sid2name really able to do so? No performance penalty?
Volker wrote : > "I haven't looked at the code carefully but a client should never need to > contact a trusted DC, it may not even have access to it. > > What is not clear to me is why are we doing RID lookups at all against another > server, why not always turn the request into a sid2name lookup ?" This is a very good point. We could change this in wbinfo instead, and turn wbinfo --lookup-rids into a LOOKUP_NAMES call which uses our primary domain when no --domain= parameter is used, and into a lookup on the domain sid followed by a LOOKUP_NAMES ? However, the thing that give me pause on doing this is the use of winbind_lookup_rids() which calls wbcLookupRids() with an arbitrary domain sid. From looking at how the code is called from the generic lookup_sids() function with an info level of 1, it appears that smbd can end up calling winbind_lookup_rids() with an arbitrary domain sid. So my gut feeling is that this patch is necessary, at least in order to get 3.6.0/3.5.next out of the door - but that the complete fix is to re-write winbindd/winbindd_lookuprids.c to convert the rid lookup into a bulk sid lookup when the given domain isn't builtin, unix, or our primary domain sid. Jeremy.
I'd prefer to have a real fix for this, it shouldn't be too hard to change to lookup sids instead of lookup rids. Contacting dc's of indirect trusts is as broken as the current behavior is.
A real fix would be nice, but can you do this before 3.6.0 needs to ship ? If you think so, then please write it and we can test. If not, then I'd advise shipping with this patch and then fixing it properly for 3.6.1. Jeremy.
On Thu, Jun 23, 2011 at 07:02:38PM +0200, samba-bugs@samba.org wrote: > https://bugzilla.samba.org/show_bug.cgi?id=7841 > > --- Comment #16 from Jeremy Allison <jra@samba.org> 2011-06-23 17:02:37 UTC --- > A real fix would be nice, but can you do this before 3.6.0 needs to ship ? > > If you think so, then please write it and we can test. I'll have a look at it tomorrow. > If not, then I'd advise shipping with this patch and then fixing it properly for 3.6.1. I'd advice to ship without the patch, as 3.6.0 would have the same behavior (bug) as current 3.5.x releases. metze
I don't think we can do this (ship without the patch) as the change broke significantly the lookup_rid functionality in 3.5.x as well - it breaks the viewing of file ACLs from trusted domains in the Windows ACL editor. See this comment: https://bugzilla.samba.org/show_bug.cgi?id=7841#c6 for details on that. This is why I raised the priority to "blocker". We can't ship with the current broken functionality. Prior 3.5.x releases were broken in that winbindd on member servers directly contacted trusted DC's, this patch restores that level of brokenness - but it's better than what is in the tree for 3.6.0 and 3.5.x right now. Jeremy.
Jeremy wrote: > I don't think we can do this (ship without the patch) as the change broke > significantly the lookup_rid functionality in 3.5.x as well - it breaks the > viewing of file ACLs from trusted domains in the Windows ACL editor. Also file's owner is displayed as the SID (not resolved to name) if the owner is from TRUSTED domain (file's property -> Security tab -> Advanced -> Owner). Although this doesn't have direct influence on access decisions (SID->name is used only for convenience here), users are unable to manage ACLs/ownership of the files via SIDs. > Prior 3.5.x releases were broken in > that winbindd on member servers directly contacted trusted DC's, this patch > restores that level of brokenness - but it's better than what is in the tree > for 3.6.0 and 3.5.x right now. This change was added only recently (Dec 2010). That means it is inside samba only since version 3.5.7. We've used version 3.5.4 before and had no visible issues with SIDs translation. After upgrade to 3.5.8 the issue has risen. I agree that previous - even not so correct - behavior [asking trusted DCs] is better that totally broken [always asking PRIMARY DC]. With the first approach the worst error is failed request if trusted DC is unavailable. But in the second approach result is always wrong for trusted domains (either 'not found' or even WRONG name). Thanks.
Maybe this patch works (completely untested yet): http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=12b03ad89ecc9860 metze
(In reply to comment #20) > Maybe this patch works (completely untested yet): > > http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=12b03ad89ecc9860 > > metze hmm, that doesn't pass make test...:-(
As I said, this is trickier to get right than we have time to properly test for 3.6.0. I recommend adding the "not quite correct" patch that we have, then finishing your patch and getting that into 3.6.1. But we do need to get this fixed for 3.6.0 Jeremy.
Created attachment 6631 [details] Next try for 3.6.0. 3.5 to follow soon.
Created attachment 6632 [details] Next try for 3.5.
Created attachment 6633 [details] Next try for 3.6.0. Metze, now with fixed commit msg
Comment on attachment 6633 [details] Next try for 3.6.0. Looks good to me
Comment on attachment 6632 [details] Next try for 3.5. Looks also good, Karolin can you fix commit message to match 3.6 and master?
Karolin, please pick for 3.6 and 3.5 (with fixed commit message) thanks!
(In reply to comment #25) > Created attachment 6633 [details] > Next try for 3.6.0. > > Metze, now with fixed commit msg I am sorry, but this patch does not apply on my current v3-6-test branch (355ffd89e): user@host:/data/git/samba/v3-6-test> git am 7841.patch Applying: s3: explicitly pass domain_sid to wbint_LookupRids() (bug #7841) error: patch failed: source3/winbindd/winbindd_dual_srv.c:494 error: source3/winbindd/winbindd_dual_srv.c: patch does not apply Cherry-picking patch from master (0a74caa47) does work, but I did not push it yet. Re-assigning to Volker for verification.
(In reply to comment #28) > Karolin, please pick for 3.6 and 3.5 (with fixed commit message) thanks! Pushed to v3-5-test.
0a74caa47 is the right one. Do we need another review? Volker
(In reply to comment #31) > 0a74caa47 is the right one. Do we need another review? > > Volker No, I don't thik so. Pushed to v3-6-test. Closing out bug report. Thanks!
Created attachment 6664 [details] Patch additional patch for v3-5-test For v3-5-test, we need to commit the regenerated pidl output, otherwise it doesn't build without a 'make samba3-idl'.
Comment on attachment 6664 [details] Patch additional patch for v3-5-test Thanks, forgot that
(In reply to comment #33) > Created attachment 6664 [details] > (...) > otherwise it doesn't build without a 'make samba3-idl'. This is very annoying. Karolin, can we push this, please? Björn
(In reply to comment #35) > (In reply to comment #33) > > Created attachment 6664 [details] [details] > > (...) > > otherwise it doesn't build without a 'make samba3-idl'. > > This is very annoying. Karolin, can we push this, please? > > Björn Pushed to v3-5-test. Closing out bug report. Please assign bug reports to me if they contain patches that should be pushed, otherwise they might be ignored. Thanks!