On 08/19/2014 01:14 PM, Jakub Hrozek wrote:
> On Mon, Aug 18, 2014 at 08:52:52PM +0200, Michal Židek wrote:
>> On 08/18/2014 10:46 AM, Jakub Hrozek wrote:
>>> On Tue, Aug 12, 2014 at 01:14:03PM +0200, Lukas Slebodnik wrote:
>>>> On (12/08/14 10:54), Jakub Hrozek wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch fixes
https://fedorahosted.org/sssd/ticket/2391
>>>>>
>>>>> To reproduce, request a group by SID, like this:
>>>>>
>>>>> $ python
>>>>> import pysss_nss_idmap
>>>>>
>>>>>
pysss_nss_idmap.getnamebysid('S-1-5-21-1104576142-166023250-1132398744-513')
>>>>>
>>>>> {'S-1-5-21-1104576142-166023250-1132398744-513':
{'type': 2,
>>>>> 'name': u'domain users(a)win.example.com'}}
>>>>
>>>> >From 95ef335f17739c441bb6c9793a6b4d7c6185ca2b Mon Sep 17 00:00:00
>>>> 2001
>>>>> From: Jakub Hrozek <jhrozek(a)redhat.com>
>>>>> Date: Tue, 12 Aug 2014 10:32:33 +0200
>>>>> Subject: [PATCH] IPA: handle searches by SID in
>>>>> apply_subdomain_homedir
>>>>>
>>>>>
https://fedorahosted.org/sssd/ticket/2391
>>>>>
>>>>> apply_subdomain_homedir() didn't handle the situation where an
entity
>>>>> that doesn't match was requested from the cache. For user and
group
>>>>> lookups this wasn't a problem because the negative match was
caught
>>>>> sooner.
>>>>>
>>>>> But SID lookups can match either user or group. When a group SID was
>>>>> requested, the preceding LDAP request matched the SID and stored the
>>>>> group in the cache. Then apply_subdomain_homedir() only tried to
>>>>> search
>>>>> user by SID, didn't find the entry and accessed a NULL pointer.
>>>>>
>>>>> A simple reproducer is:
>>>>> $ python
>>>>>>>> import pysss_nss_idmap
>>>>>>>> pysss_nss_idmap.getnamebysid(group_sid)
>>>>>
>>>>> The group_sid can be anything, including Domain Users (XXX-513)
>>>>> ---
>>>>> src/providers/ipa/ipa_subdomains_id.c | 18 +++++++++++-------
>>>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/src/providers/ipa/ipa_subdomains_id.c
>>>>> b/src/providers/ipa/ipa_subdomains_id.c
>>>>> index
>>>>>
113bc6c06f82bc631b3efa92b87a1cadc7f22605..7857cd566f7585663d2e0fc94b1265132f4af739
>>>>> 100644
>>>>> --- a/src/providers/ipa/ipa_subdomains_id.c
>>>>> +++ b/src/providers/ipa/ipa_subdomains_id.c
>>>>> @@ -516,18 +516,22 @@ apply_subdomain_homedir(TALLOC_CTX *mem_ctx,
>>>>> struct sss_domain_info *dom,
>>>>> goto done;
>>>>> }
>>>>>
>>>>> - if (ret != EOK && ret != ENOENT) {
>>>>> - DEBUG(SSSDBG_OP_FAILURE,
>>>>> - "Failed to make request to our cache: [%d]:
[%s]\n",
>>>>> - ret, sss_strerror(ret));
>>>>> - goto done;
>>>>> - }
>>>>> -
>>>>> if ((res && res->count == 0) || (msg &&
msg->num_elements ==
>>>>> 0)) {
>>>>> ret = ENOENT;
>>>>> goto done;
>>>>> }
>>>>>
>>>>> + if (ret != EOK && ret != ENOENT) {
>>>>> + DEBUG(SSSDBG_OP_FAILURE,
>>>>> + "Failed to make request to our cache: [%d]:
[%s]\n",
>>>>> + ret, sss_strerror(ret));
>>>>> + goto done;
>>>>> + } else if (ret == ENOENT) {
>>>>> + DEBUG(SSSDBG_TRACE_FUNC, "Cannot find [%s] with search
>>>>> type [%d]\n",
>>>>> + filter_value, filter_type);
>>>>> + goto done;
>>>>> + }
>>>>> +
>>>>> if (res != NULL) {
>>>>> msg = res->msgs[0];
>>>>> }
>>>>
>>>> I admit it can fix the crash, but it is opposite of change PavelR
>>>> did in
>>>> different code.
>>>>
>>>> commit 09be002e58babda513b4b75d2b9eb9b2c351fa26
>>>> Author: Pavel Reichl <preichl(a)redhat.com>
>>>> Date: Wed Feb 26 16:58:24 2014 +0000
>>>> NSS: sysdb_getnetgr check return value first
>>>
>>> OK, what about the attached version? I unrolled the changes so the code
>>> only sets ENOENT and does not shortcut until the ENOENT check.
>>>
>>>>
>>>> It could be also chance to improve problems decribed in ticket #1991.
>>>
>>> Yes, but is it in the scope of fixing this crash and minimal changes
>>> towards downstream? Could we fix the crash with as small patch as
>>> possible and add additional one of top? I'm fine with adding the
>>> additional patch to master right away..
>>>
>>>>
>>>> BTW I was not able to reproduce crash. Could you describe hierarchy
>>>> of groups
>>>> in your testing environment.
>>>
>>> You need to test in a scenario with AD-IPA trusts on the IPA server
>>> itself. The hierarchy is not nested, the user is a member of two flat
>>> groups that have POSIX IDs and Domain Users which don't have POSIX IDs.
>>>
>>> There it was enough for me to request a group by its SID:
>>> $ python
>>> import pysss_nss_idmap
>>>
>>>
pysss_nss_idmap.getnamebysid('S-1-5-21-1104576142-166023250-1132398744-513')
>>>
>>> {'S-1-5-21-1104576142-166023250-1132398744-513':
{'type': 2,
>>> 'name': u'domain users(a)win.example.com'}}
>>>
>>>
>>>
>>
>> I can not reproduce this with current master. What SSSD version
>> did you use?
>
> git master.
>
> I can give you access to my test setup, I can reproduce the problem
> there easily.
Sorry, I tested it wrongly (used the user SID, not the group SID).
I can reproduce it now, will send review soon.
Michal
ACK for master. Please send also patch that applies on sssd-1-11.
Michal