From 785f915b4a82383e3062d92e26e73b937dee83df Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Thu, 2 Mar 2017 17:54:46 +0100
Subject: [PATCH 1/2] SYSDB: When searching for UPNs, search either the whole
 DB or only the given domain

The search-by-UPN functions always searched for the whole domain. In
some cases, the caller depends on the result coming from the domain
specified by the 'domain' parameter. This is the case in the cache_req
code at least. Even though it should be safe to just switch to always
searching the whole domain, in order to allow us to examine the code
carefully and test each codepath, let's introduce a boolean option to
the search functions. Currently it defaults to false in all codepaths
and as we test the individual ones, we can flip the option to true until
we finally remove the option altogether.
---
 src/db/sysdb.h                                             |  3 +++
 src/db/sysdb_ops.c                                         | 10 ++++++++--
 src/db/sysdb_search.c                                      |  7 ++++---
 src/providers/ad/ad_pac.c                                  |  2 +-
 src/providers/ipa/ipa_subdomains_id.c                      |  2 +-
 src/providers/ldap/ldap_id.c                               |  2 +-
 .../common/cache_req/plugins/cache_req_user_by_upn.c       |  7 ++++---
 src/responder/pam/pamsrv_cmd.c                             |  2 +-
 src/tests/cmocka/test_sysdb_ts_cache.c                     |  6 +++---
 src/tests/sysdb-tests.c                                    | 14 +++++++-------
 10 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 809ca35..54bac7e 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -658,6 +658,7 @@ int sysdb_getpwuid(TALLOC_CTX *mem_ctx,
 
 int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
                    struct sss_domain_info *domain,
+                   bool domain_scope,
                    const char *upn,
                    struct ldb_result **res);
 
@@ -832,12 +833,14 @@ int sysdb_search_user_by_sid_str(TALLOC_CTX *mem_ctx,
 
 int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
                                  struct sss_domain_info *domain,
+                                 bool domain_scope,
                                  const char *upn,
                                  const char **attrs,
                                  struct ldb_result **out_res);
 
 int sysdb_search_user_by_upn(TALLOC_CTX *mem_ctx,
                              struct sss_domain_info *domain,
+                             bool domain_scope,
                              const char *sid_str,
                              const char **attrs,
                              struct ldb_message **msg);
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 7f6c127..47f878f 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -538,6 +538,7 @@ int sysdb_search_user_by_sid_str(TALLOC_CTX *mem_ctx,
 
 int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
                                  struct sss_domain_info *domain,
+                                 bool domain_scope,
                                  const char *upn,
                                  const char **attrs,
                                  struct ldb_result **out_res)
@@ -555,7 +556,11 @@ int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
         goto done;
     }
 
-    base_dn = sysdb_base_dn(domain->sysdb, tmp_ctx);
+    if (domain_scope == true) {
+        base_dn = sysdb_user_base_dn(tmp_ctx, domain);
+    } else {
+        base_dn = sysdb_base_dn(domain->sysdb, tmp_ctx);
+    }
     if (base_dn == NULL) {
         ret = ENOMEM;
         goto done;
@@ -599,6 +604,7 @@ int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
 
 int sysdb_search_user_by_upn(TALLOC_CTX *mem_ctx,
                              struct sss_domain_info *domain,
+                             bool domain_scope,
                              const char *upn,
                              const char **attrs,
                              struct ldb_message **msg)
@@ -613,7 +619,7 @@ int sysdb_search_user_by_upn(TALLOC_CTX *mem_ctx,
         goto done;
     }
 
-    ret = sysdb_search_user_by_upn_res(tmp_ctx, domain, upn, attrs, &res);
+    ret = sysdb_search_user_by_upn_res(tmp_ctx, domain, domain_scope, upn, attrs, &res);
     if (ret == ENOENT) {
         DEBUG(SSSDBG_TRACE_FUNC, "No entry with upn [%s] found.\n", upn);
         goto done;
diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
index 4d63c38..474bc08 100644
--- a/src/db/sysdb_search.c
+++ b/src/db/sysdb_search.c
@@ -454,6 +454,7 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
 
 int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
                    struct sss_domain_info *domain,
+                   bool domain_scope,
                    const char *upn,
                    struct ldb_result **_res)
 {
@@ -468,7 +469,7 @@ int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
         return ENOMEM;
     }
 
-    ret = sysdb_search_user_by_upn_res(tmp_ctx, domain, upn, attrs, &res);
+    ret = sysdb_search_user_by_upn_res(tmp_ctx, domain, domain_scope, upn, attrs, &res);
     if (ret != EOK && ret != ENOENT) {
         DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn_res() failed.\n");
         goto done;
@@ -1322,7 +1323,7 @@ int sysdb_initgroups_by_upn(TALLOC_CTX *mem_ctx,
         return ENOMEM;
     }
 
-    ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, &msg);
+    ret = sysdb_search_user_by_upn(tmp_ctx, domain, false, upn, attrs, &msg);
     if (ret != EOK && ret != ENOENT) {
         DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn() failed.\n");
         goto done;
@@ -2113,7 +2114,7 @@ errno_t sysdb_get_real_name(TALLOC_CTX *mem_ctx,
     }
 
     if (res->count == 0) {
-        ret = sysdb_search_user_by_upn(tmp_ctx, domain, name_or_upn_or_sid,
+        ret = sysdb_search_user_by_upn(tmp_ctx, domain, false, name_or_upn_or_sid,
                                        NULL, &msg);
         if (ret == ENOENT) {
             ret = sysdb_search_user_by_sid_str(tmp_ctx, domain,
diff --git a/src/providers/ad/ad_pac.c b/src/providers/ad/ad_pac.c
index 1fd7fd2..deade5e 100644
--- a/src/providers/ad/ad_pac.c
+++ b/src/providers/ad/ad_pac.c
@@ -51,7 +51,7 @@ static errno_t find_user_entry(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
     }
 
     if (ar->extra_value && strcmp(ar->extra_value, EXTRA_NAME_IS_UPN) == 0) {
-        ret = sysdb_search_user_by_upn(tmp_ctx, dom, ar->filter_value,
+        ret = sysdb_search_user_by_upn(tmp_ctx, dom, false, ar->filter_value,
                                        user_attrs, &msg);
     } else {
         switch (ar->filter_type) {
diff --git a/src/providers/ipa/ipa_subdomains_id.c b/src/providers/ipa/ipa_subdomains_id.c
index 0d74e20..4b639f4 100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -1051,7 +1051,7 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
         case BE_REQ_USER_AND_GROUP:
             if (ar->extra_value
                     && strcmp(ar->extra_value, EXTRA_NAME_IS_UPN) == 0) {
-                ret = sysdb_search_user_by_upn(mem_ctx, dom, ar->filter_value,
+                ret = sysdb_search_user_by_upn(mem_ctx, dom, false, ar->filter_value,
                                                attrs, &msg);
             } else {
                 ret = sysdb_search_user_by_name(mem_ctx, dom, ar->filter_value,
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 70300e6..e9455b5 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -526,7 +526,7 @@ static void users_get_done(struct tevent_req *subreq)
             return;
         case BE_FILTER_NAME:
             if (state->name_is_upn == true) {
-                ret = sysdb_search_user_by_upn(state, state->domain,
+                ret = sysdb_search_user_by_upn(state, state->domain, false,
                                                state->filter_value,
                                                NULL, &msg);
                 if (ret != EOK) {
diff --git a/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c b/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
index 9c69573..2453430 100644
--- a/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
+++ b/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
@@ -85,11 +85,12 @@ cache_req_user_by_upn_lookup(TALLOC_CTX *mem_ctx,
                              struct ldb_result **_result)
 {
     if (data->attrs == NULL) {
-        return sysdb_getpwupn(mem_ctx, domain, data->name.lookup, _result);
+        return sysdb_getpwupn(mem_ctx, domain, false, data->name.lookup, _result);
     }
 
-    return sysdb_search_user_by_upn_res(mem_ctx, domain, data->name.lookup,
-                                        data->attrs, _result);
+    return sysdb_search_user_by_upn_res(mem_ctx, domain, false,
+                                        data->name.lookup, data->attrs,
+                                        _result);
 }
 
 static struct tevent_req *
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index e788a75..d4b2d32 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1658,7 +1658,7 @@ static int pam_check_user_search(struct pam_auth_req *preq)
         }
 
         if (preq->pd->name_is_upn) {
-            ret = sysdb_search_user_by_upn(preq, dom, name, user_attrs,
+            ret = sysdb_search_user_by_upn(preq, dom, false, name, user_attrs,
                                            &preq->user_obj);
             if (ret == EOK) {
                 /* Since sysdb_search_user_by_upn() searches the whole cache we
diff --git a/src/tests/cmocka/test_sysdb_ts_cache.c b/src/tests/cmocka/test_sysdb_ts_cache.c
index f5aab73..74badf8 100644
--- a/src/tests/cmocka/test_sysdb_ts_cache.c
+++ b/src/tests/cmocka/test_sysdb_ts_cache.c
@@ -1303,14 +1303,14 @@ static void test_user_byupn(void **state)
                            TEST_NOW_2);
     assert_int_equal(ret, EOK);
 
-    ret = sysdb_getpwupn(test_ctx, test_ctx->tctx->dom, TEST_USER_UPN, &res);
+    ret = sysdb_getpwupn(test_ctx, test_ctx->tctx->dom, false, TEST_USER_UPN, &res);
     assert_int_equal(ret, EOK);
     assert_int_equal(res->count, 1);
     assert_ts_attrs_res(res, TEST_NOW_2 + TEST_CACHE_TIMEOUT, TEST_NOW_2);
     talloc_free(res);
 
     ret = sysdb_search_user_by_upn_res(test_ctx, test_ctx->tctx->dom,
-                                       TEST_USER_UPN, pw_fetch_attrs,
+                                       false, TEST_USER_UPN, pw_fetch_attrs,
                                        &res);
     assert_int_equal(ret, EOK);
     assert_int_equal(res->count, 1);
@@ -1318,7 +1318,7 @@ static void test_user_byupn(void **state)
     talloc_free(res);
 
     ret = sysdb_search_user_by_upn(test_ctx, test_ctx->tctx->dom,
-                                   TEST_USER_UPN, pw_fetch_attrs,
+                                   false, TEST_USER_UPN, pw_fetch_attrs,
                                    &msg);
     assert_int_equal(ret, EOK);
     assert_ts_attrs_msg(msg, TEST_NOW_2 + TEST_CACHE_TIMEOUT, TEST_NOW_2);
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index fe043f6..65692ba 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -6253,12 +6253,12 @@ START_TEST(test_upn_basic)
                            attrs, NULL, -1, 0);
     fail_unless(ret == EOK, "Could not store user.");
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    "abc@def.ghi", NULL, &msg);
     fail_unless(ret == ENOENT,
                 "sysdb_search_user_by_upn failed with non-existing UPN.");
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_PRINC, NULL, &msg);
     fail_unless(ret == EOK, "sysdb_search_user_by_upn failed.");
 
@@ -6290,7 +6290,7 @@ START_TEST(test_upn_basic_case)
         return;
     }
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_PRINC_WRONG_CASE, NULL, &msg);
     fail_unless(ret == EOK, "sysdb_search_user_by_upn failed.");
 
@@ -6322,7 +6322,7 @@ START_TEST(test_upn_canon)
         return;
     }
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_CANON_PRINC, NULL, &msg);
     fail_unless(ret == EOK, "sysdb_search_user_by_upn failed.");
 
@@ -6359,7 +6359,7 @@ START_TEST(test_upn_canon_case)
         return;
     }
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_CANON_PRINC_WRONG_CASE, NULL, &msg);
     fail_unless(ret == EOK, "sysdb_search_user_by_upn failed.");
 
@@ -6410,12 +6410,12 @@ START_TEST(test_upn_dup)
                            attrs, NULL, -1, 0);
     fail_unless(ret == EOK, "Could not store user.");
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_CANON_PRINC, NULL, &msg);
     fail_unless(ret == EINVAL,
                 "sysdb_search_user_by_upn failed for duplicated UPN.");
 
-    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain,
+    ret = sysdb_search_user_by_upn(test_ctx, test_ctx->domain, false,
                                    UPN_PRINC, NULL, &msg);
     fail_unless(ret == EOK, "sysdb_search_user_by_upn failed.");
 

From 72fa2d5918fe29b71300b8c559a2b9d2880407eb Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Fri, 3 Mar 2017 13:47:52 +0100
Subject: [PATCH 2/2] CACHE_REQ: Only search the given domain when looking up
 entries by UPN

We were searching UPNs in the whole sysdb, which made cache_req think the
result came in from the domain it was searching.

The bug manifested when a user from a trusted domain was looked by UPN,
then cache_req searched the main domain, the result from subdomain was
considered as coming from the main domain and as a result, the getpwnam()
output was not qualified. That is a problem, because PAM applications
often sanitize the user with getpwnam, so effectively a login with UPN
was shortened to just a shortname and failed.
---
 src/responder/common/cache_req/plugins/cache_req_user_by_upn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c b/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
index 2453430..40a097b 100644
--- a/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
+++ b/src/responder/common/cache_req/plugins/cache_req_user_by_upn.c
@@ -85,10 +85,10 @@ cache_req_user_by_upn_lookup(TALLOC_CTX *mem_ctx,
                              struct ldb_result **_result)
 {
     if (data->attrs == NULL) {
-        return sysdb_getpwupn(mem_ctx, domain, false, data->name.lookup, _result);
+        return sysdb_getpwupn(mem_ctx, domain, true, data->name.lookup, _result);
     }
 
-    return sysdb_search_user_by_upn_res(mem_ctx, domain, false,
+    return sysdb_search_user_by_upn_res(mem_ctx, domain, true,
                                         data->name.lookup, data->attrs,
                                         _result);
 }
