Bug 7841 - WINBINDD_LOOKUPRIDS asks the wrong domain
WINBINDD_LOOKUPRIDS asks the wrong domain
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: Winbind
3.5.9
Other All
: P3 regression
: ---
Assigned To: Michael Adam
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-05 09:42 UTC by Volker Lendecke
Modified: 2011-08-27 08:50 UTC (History)
1 user (show)

See Also:


Attachments
Patch for 3.5 (871 bytes, patch)
2010-12-05 09:44 UTC, Volker Lendecke
jra: review+
Details
git-am fix for 3.6.0 (3.55 KB, patch)
2011-06-21 23:01 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.next (3.53 KB, patch)
2011-06-21 23:02 UTC, Jeremy Allison
no flags Details
Next try for 3.6.0. (4.16 KB, patch)
2011-06-27 13:56 UTC, Volker Lendecke
no flags Details
Next try for 3.5. (3.54 KB, patch)
2011-06-27 14:04 UTC, Volker Lendecke
metze: review+
Details
Next try for 3.6.0. (4.18 KB, patch)
2011-06-27 15:03 UTC, Volker Lendecke
metze: review+
Details
Patch additional patch for v3-5-test (5.56 KB, patch)
2011-07-04 09:52 UTC, Stefan Metzmacher
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Volker Lendecke 2010-12-05 09:42:01 UTC
WINBINDD_LOOKUPRIDS is a lookup call, so in winbindd_lookuprids.c the find_domain_from_sid is wrong.
Comment 1 Volker Lendecke 2010-12-05 09:44:33 UTC
Created attachment 6109 [details]
Patch for 3.5
Comment 2 Volker Lendecke 2010-12-05 09:50:35 UTC
Bug and Patch also apply to master/3.6.
Comment 3 Jeremy Allison 2010-12-06 13:06:38 UTC
Comment on attachment 6109 [details]
Patch for 3.5

LGTM!
Comment 4 Jeremy Allison 2010-12-06 13:07:04 UTC
Re-assigning to Karolin for inclusion in 3.5.next.
Jeremy.
Comment 5 Karolin Seeger 2010-12-07 04:46:00 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 6 Volodymyr Khomenko 2011-06-21 12:20:22 UTC
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).
Comment 7 Jeremy Allison 2011-06-21 21:29:00 UTC
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.
Comment 8 Jeremy Allison 2011-06-21 21:32:15 UTC
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.
Comment 9 Jeremy Allison 2011-06-21 21:52:33 UTC
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.
Comment 10 Jeremy Allison 2011-06-21 23:01:08 UTC
Created attachment 6604 [details]
git-am fix for 3.6.0
Comment 11 Jeremy Allison 2011-06-21 23:02:37 UTC
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.
Comment 12 Simo Sorce 2011-06-22 02:44:13 UTC
(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 ?
Comment 13 Volodymyr Khomenko 2011-06-22 09:21:50 UTC
> 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?
Comment 14 Jeremy Allison 2011-06-22 17:06:11 UTC
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.
Comment 15 Stefan Metzmacher 2011-06-23 17:01:16 UTC
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.
Comment 16 Jeremy Allison 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. If not, then I'd advise shipping with this patch and then fixing it properly for 3.6.1.

Jeremy.
Comment 17 Stefan Metzmacher 2011-06-23 17:13:34 UTC
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
Comment 18 Jeremy Allison 2011-06-23 17:18:40 UTC
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.
Comment 19 Volodymyr Khomenko 2011-06-24 07:08:11 UTC
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.
Comment 20 Stefan Metzmacher 2011-06-24 19:02:38 UTC
Maybe this patch works (completely untested yet):

http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=12b03ad89ecc9860

metze
Comment 21 Stefan Metzmacher 2011-06-24 19:54:50 UTC
(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...:-(
Comment 22 Jeremy Allison 2011-06-25 01:30:40 UTC
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.
Comment 23 Volker Lendecke 2011-06-27 13:56:36 UTC
Created attachment 6631 [details]
Next try for 3.6.0.

3.5 to follow soon.
Comment 24 Volker Lendecke 2011-06-27 14:04:06 UTC
Created attachment 6632 [details]
Next try for 3.5.
Comment 25 Volker Lendecke 2011-06-27 15:03:44 UTC
Created attachment 6633 [details]
Next try for 3.6.0.

Metze, now with fixed commit msg
Comment 26 Stefan Metzmacher 2011-06-28 15:15:02 UTC
Comment on attachment 6633 [details]
Next try for 3.6.0.

Looks good to me
Comment 27 Stefan Metzmacher 2011-06-28 15:19:25 UTC
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?
Comment 28 Stefan Metzmacher 2011-06-28 15:20:27 UTC
Karolin, please pick for 3.6 and 3.5 (with fixed commit message) thanks!
Comment 29 Karolin Seeger 2011-06-28 19:06:47 UTC
(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.
Comment 30 Karolin Seeger 2011-06-28 19:08:51 UTC
(In reply to comment #28)
> Karolin, please pick for 3.6 and 3.5 (with fixed commit message) thanks!

Pushed to v3-5-test.
Comment 31 Volker Lendecke 2011-06-28 20:50:45 UTC
0a74caa47 is the right one. Do we need another review?

Volker
Comment 32 Karolin Seeger 2011-06-29 18:18:48 UTC
(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!
Comment 33 Stefan Metzmacher 2011-07-04 09:52:00 UTC
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 34 Volker Lendecke 2011-07-04 10:12:02 UTC
Comment on attachment 6664 [details]
Patch additional patch for v3-5-test

Thanks, forgot that
Comment 35 Björn Baumbach 2011-07-05 13:12:31 UTC
(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
Comment 36 Karolin Seeger 2011-07-05 17:47:01 UTC
(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!