[SSSD] [PATCH] IPA: handle searches by SID in apply_subdomain_homedir

Jakub Hrozek jhrozek at redhat.com
Mon Aug 18 08:46:50 UTC 2014


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 at win.example.com'}}
> 
> >From 95ef335f17739c441bb6c9793a6b4d7c6185ca2b Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek at 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 at 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 at win.example.com'}}
-------------- next part --------------
>From 5f9250baafe0a87bda552e359997797b4ead1a3f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at 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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
index 113bc6c06f82bc631b3efa92b87a1cadc7f22605..659bc7c2cba4fd9e4d8b07d79efaa6feede35cb9 100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -498,6 +498,9 @@ apply_subdomain_homedir(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
 
     if (filter_type == BE_FILTER_NAME) {
         ret = sysdb_getpwnam(mem_ctx, dom, filter_value, &res);
+        if (res && res->count == 0) {
+            ret = ENOENT;
+        }
     } else if (filter_type == BE_FILTER_IDNUM) {
         errno = 0;
         uid = strtouint32(filter_value, NULL, 10);
@@ -506,6 +509,9 @@ apply_subdomain_homedir(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
             goto done;
         }
         ret = sysdb_getpwuid(mem_ctx, dom, uid, &res);
+        if (res && res->count == 0) {
+            ret = ENOENT;
+        }
     } else if (filter_type == BE_FILTER_SECID) {
         ret = sysdb_search_user_by_sid_str(mem_ctx, dom, filter_value,
                                            attrs, &msg);
@@ -521,10 +527,9 @@ apply_subdomain_homedir(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
               "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;
+    } else if (ret == ENOENT) {
+        DEBUG(SSSDBG_TRACE_FUNC, "Cannot find [%s] with search type [%d]\n",
+              filter_value, filter_type);
         goto done;
     }
 
-- 
1.9.3



More information about the sssd-devel mailing list