[SSSD] [PATCH] PAM: Check for trusted domain before sending the request to BE

Jakub Hrozek jhrozek at redhat.com
Mon Nov 24 15:38:20 UTC 2014


Hi,

When I was working on https://fedorahosted.org/sssd/ticket/2501 I
noticed several things I didn't like. Most importnantly, the checks were
done on several places -- I think security decisions should be done at
one place only ideally so any changes don't miss other calls.

I'll be sending other patches soon, but this one is important to get in
soon.

In particular, the issue would hit when you try to authenticate as a
user from a domain that is second on the list. In pam_dom_forwarder, we
might change the pd->dom pointer, bypassing the check done previously.

Additionally, the restricted domains are only checked if the process is
trusted. Shall I split that to a new patch?

Please also let me know if the performance implication outlined in the
ticket seems like an important one.
-------------- next part --------------
>From e4a3f4c303e5200a2dccb3181fa5317f01a96ea8 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 23 Nov 2014 19:43:04 +0100
Subject: [PATCH] PAM: Check for trusted domain before sending the request to
 BE

https://fedorahosted.org/sssd/ticket/2501

Moving the checks to one place has the advantage of not duplicating
security decisions. Previously, the checks were scattered all over the
responder code, making testing hard.

The disadvantage is that we actually check for the presence of the user,
which might trigger some back end lookups. But I think the benefits
overweight the disadvantage.

Also only check the requested domains from a trusted client. An untrusted
client should simply have no say in what domains he wants to talk to, it
should ignore the 'domains' option.
---
 src/responder/pam/pamsrv_cmd.c | 67 ++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 41 deletions(-)

diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 6eed4f981b0bd53a2fbd49884e3a1472993093a7..ea084d56af03b3dcb509eaccc13ee88abb163d55 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -898,17 +898,6 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
             goto done;
         }
 
-        /* Untrusted users can access only public domains. */
-        if (!pctx->is_uid_trusted &&
-            !is_domain_public(pd->domain, pctx->public_domains,
-                              pctx->public_domains_count)) {
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "Untrusted user %"PRIu32" cannot access unpublic domain %s.\n",
-                  cctx->client_euid, pd->domain);
-            ret = EPERM;
-            goto done;
-        }
-
         ncret = sss_ncache_check_user(pctx->ncache, pctx->neg_timeout,
                                       preq->domain, pd->user);
         if (ncret == EEXIST) {
@@ -916,34 +905,12 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
             ret = ENOENT;
             goto done;
         }
-
-        /* skip this domain if not requested */
-        if (!is_domain_requested(pd, pd->domain)) {
-            ret = ENOENT;
-            goto done;
-        }
     } else {
         for (dom = preq->cctx->rctx->domains;
              dom;
              dom = get_next_domain(dom, false)) {
             if (dom->fqnames) continue;
 
-            /* Untrusted users can access only public domains. */
-            if (!pctx->is_uid_trusted &&
-                !is_domain_public(dom->name, pctx->public_domains,
-                                  pctx->public_domains_count)) {
-                DEBUG(SSSDBG_MINOR_FAILURE,
-                      "Untrusted user %"PRIu32" cannot access unpublic domain %s."
-                      " Trying next domain.\n",
-                      cctx->client_euid, dom->name);
-                continue;
-            }
-
-            /* skip this domain if not requested */
-            if (!is_domain_requested(pd, dom->name)) {
-                continue;
-            }
-
             ncret = sss_ncache_check_user(pctx->ncache, pctx->neg_timeout,
                                           dom, pd->user);
             if (ncret == ENOENT) {
@@ -959,7 +926,7 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
                    "Trying next domain.\n", pd->user, dom->name);
         }
 
-        if (!dom || !is_domain_requested(pd, dom->name)) {
+        if (!dom) {
             ret = ENOENT;
             goto done;
         }
@@ -1055,14 +1022,9 @@ static int pam_check_user_search(struct pam_auth_req *preq)
 
     while (dom) {
        /* if it is a domainless search, skip domains that require fully
-        * qualified names instead, also untrusted users can access only
-        * public domains */
+        * qualified names instead */
         while (dom && !preq->pd->domain && !preq->pd->name_is_upn
-               && (dom->fqnames ||
-                   (!pctx->is_uid_trusted &&
-                    !is_domain_public(dom->name,
-                                      pctx->public_domains,
-                                      pctx->public_domains_count)))) {
+               && dom->fqnames) {
             dom = get_next_domain(dom, false);
         }
 
@@ -1334,11 +1296,34 @@ done:
 static void pam_dom_forwarder(struct pam_auth_req *preq)
 {
     int ret;
+    struct pam_ctx *pctx =
+            talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx);
 
     if (!preq->pd->domain) {
         preq->pd->domain = preq->domain->name;
     }
 
+    /* Untrusted users can access only public domains. */
+    if (!pctx->is_uid_trusted &&
+            !is_domain_public(preq->pd->domain, pctx->public_domains,
+                            pctx->public_domains_count)) {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+                "Untrusted user %"PRIu32" cannot access non-public domain %s.\n",
+                preq->cctx->client_euid, preq->pd->domain);
+        preq->pd->pam_status = PAM_PERM_DENIED;
+        pam_reply(preq);
+        return;
+    }
+
+    /* skip this domain if not requested and the user is trusted
+     * as untrusted users can't request a domain */
+    if (pctx->is_uid_trusted &&
+            !is_domain_requested(preq->pd, preq->pd->domain)) {
+        preq->pd->pam_status = PAM_USER_UNKNOWN;
+        pam_reply(preq);
+        return;
+    }
+
     if (!NEED_CHECK_PROVIDER(preq->domain->provider)) {
         preq->callback = pam_reply;
         ret = LOCAL_pam_handler(preq);
-- 
1.9.3



More information about the sssd-devel mailing list