[SSSD] [PATCH] Two enhancements for PAM client

Sumit Bose sbose at redhat.com
Mon Apr 26 11:41:57 UTC 2010


On Thu, Apr 22, 2010 at 09:03:25AM -0400, Stephen Gallagher wrote:
> On 04/22/2010 06:08 AM, Sumit Bose wrote:
> > Hi,
> >
> > the two patches attached should fix #446 and #417 respectively.
> >
> > For #417 a different solution, where the message is generated by SSSD
> > and send to the client, would be possilbe. But I decided against it,
> > because with the attached patch it is possilbe to support localized
> > messages.
> 
> 
> Patch 0001: Ack.
> 
> Patch 0002: Nack. You would be introducing a security hole here in the 
> form of a stat-open race condition. Please open the file first and use 
> fstat() to determine its size. It probably wouldn't be a bad idea to 
> verify that the file is owned by root and not writable by anyone but 
> root. I don't think it's important to verify whether the file is a 
> symlink or a regular file.

done

> 
> Even making the above check, it wouldn't be a bad idea to verify that 
> total_len == stat_buf.st_size after the read loop.

done

> 
> The DEBUG message in the read loop is wrong. It should be "read failed", 
> not "open failed".

done

> 
> Also, if you're going to allow custom messages here, we probably need to 
> make this message file be per-domain. Since it may be translatable, I 
> suggest we add a new directory structure under /etc/sssd that looks like 
> this (assuming the domain name is LDAP):
> 
> /etc/sssd/customize
> /etc/sssd/customize/LDAP/
> /etc/sssd/customize/LDAP/root_pw_reset.txt
> /etc/sssd/customize/LDAP/root_pw_reset.zh_TW
> ...
> 

done

new version attached.

bye,
Sumit

> 
> -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From 1776ba3f543c8c7ce1adfc77555d5ff8c48a37e6 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 21 Apr 2010 10:46:01 +0200
Subject: [PATCH 1/2] Unset authentication tokens if password change fails

---
 src/sss_client/pam_sss.c |   79 ++++++++++++++++++++++++++++++----------------
 1 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index 4208faa..77dec19 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -1201,37 +1201,62 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
 
     ret = send_and_receive(pamh, &pi, task);
 
-/* We allow sssd to send the return code PAM_NEW_AUTHTOK_REQD during
- * authentication, see sss_cli.h for details */
-    if (ret == PAM_NEW_AUTHTOK_REQD && task == SSS_PAM_AUTHENTICATE) {
-        D(("Authtoken expired, trying to change it"));
-
-        exp_data = malloc(sizeof(int));
-        if (exp_data == NULL) {
-            D(("malloc failed."));
-            return PAM_BUF_ERR;
-        }
-        *exp_data = 1;
-        ret = pam_set_data(pamh, PWEXP_FLAG, exp_data, free_exp_data);
-        if (ret != PAM_SUCCESS) {
-            D(("pam_set_data failed."));
-            return ret;
-        }
-
-        return PAM_SUCCESS;
-    }
+    switch (task) {
+        case SSS_PAM_AUTHENTICATE:
+            /* We allow sssd to send the return code PAM_NEW_AUTHTOK_REQD during
+             * authentication, see sss_cli.h for details */
+            if (ret == PAM_NEW_AUTHTOK_REQD) {
+                D(("Authtoken expired, trying to change it"));
+
+                exp_data = malloc(sizeof(int));
+                if (exp_data == NULL) {
+                    D(("malloc failed."));
+                    return PAM_BUF_ERR;
+                }
+                *exp_data = 1;
+                ret = pam_set_data(pamh, PWEXP_FLAG, exp_data, free_exp_data);
+                if (ret != PAM_SUCCESS) {
+                    D(("pam_set_data failed."));
+                    return ret;
+                }
 
-    if (ret == PAM_SUCCESS && task == SSS_PAM_ACCT_MGMT &&
-        pam_get_data(pamh, PWEXP_FLAG, (const void **) &exp_data) ==
+                return PAM_SUCCESS;
+            }
+            break;
+        case SSS_PAM_ACCT_MGMT:
+            if (ret == PAM_SUCCESS &&
+                pam_get_data(pamh, PWEXP_FLAG, (const void **) &exp_data) ==
                                                                   PAM_SUCCESS) {
-        ret = do_pam_conversation(pamh, PAM_TEXT_INFO,
-                _("Password expired. Change your password now."), NULL, NULL);
-        if (ret != PAM_SUCCESS) {
-            D(("do_pam_conversation failed."));
-        }
-        return PAM_NEW_AUTHTOK_REQD;
+                ret = do_pam_conversation(pamh, PAM_TEXT_INFO,
+                               _("Password expired. Change your password now."),
+                               NULL, NULL);
+                if (ret != PAM_SUCCESS) {
+                    D(("do_pam_conversation failed."));
+                }
+                return PAM_NEW_AUTHTOK_REQD;
+            }
+            break;
+        case SSS_PAM_CHAUTHTOK:
+            if (ret != PAM_SUCCESS && ret != PAM_USER_UNKNOWN) {
+                ret = pam_set_item(pamh, PAM_AUTHTOK, NULL);
+                if (ret != PAM_SUCCESS) {
+                    D(("Failed to unset PAM_AUTHTOK [%s]",
+                       pam_strerror(pamh,ret)));
+                }
+                ret = pam_set_item(pamh, PAM_OLDAUTHTOK, NULL);
+                if (ret != PAM_SUCCESS) {
+                    D(("Failed to unset PAM_OLDAUTHTOK [%s]",
+                       pam_strerror(pamh,ret)));
+                }
+            }
+            break;
+        default:
+            /* nothing to do */
+            break;
     }
 
+
+
     overwrite_and_free_authtoks(&pi);
 
     return ret;
-- 
1.6.6.1

-------------- next part --------------
From 10a9f90cd3f1ee1d1babcd7595dfe563e9f319b4 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 21 Apr 2010 14:42:34 +0200
Subject: [PATCH 2/2] Display a message if a password reset by root fails

---
 src/man/pam_sss.8.xml          |   24 +++++
 src/providers/krb5/krb5_auth.c |    7 ++
 src/providers/ldap/ldap_auth.c |    8 ++
 src/sss_client/pam_sss.c       |  212 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 243 insertions(+), 8 deletions(-)

diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
index f6ac9f4..8684e42 100644
--- a/src/man/pam_sss.8.xml
+++ b/src/man/pam_sss.8.xml
@@ -85,6 +85,30 @@
         </para>
     </refsect1>
 
+    <refsect1 id='files'>
+        <title>FILES</title>
+        <para>If a password reset by root fails, because the corresponding SSSD
+        provider does not support password resets, an individual message can be
+        displayed. This message can e.g. contain instructions about how to reset
+        a password.</para>
+
+        <para>The message is read from the file
+        <filename>pam_sss_pw_reset_message.LOC</filename> where LOC stands for a
+        locale string returned by <citerefentry>
+        <refentrytitle>setlocale</refentrytitle><manvolnum>3</manvolnum>
+        </citerefentry>. If there is no matching file the content of
+        <filename>pam_sss_pw_reset_message.txt</filename> is displayed. Root
+        must be the owner of the files and only root may have read and write
+        permissions while all other users must have only read
+        permisssions.</para>
+
+        <para>These files are searched in the directory
+        <filename>/etc/sssd/customize/DOMAIN_NAME/</filename> and as a fallback
+        in <filename>/etc/sssd/customize/</filename>.</para>
+
+        <para>If no matching file is present a generic message is displayed.</para>
+    </refsect1>
+
     <refsect1 id='see_also'>
         <title>SEE ALSO</title>
         <para>
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 6a57fe5..e1aaebf 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -635,7 +635,14 @@ void krb5_pam_handler(struct be_req *be_req)
     switch (pd->cmd) {
         case SSS_PAM_AUTHENTICATE:
         case SSS_PAM_CHAUTHTOK:
+            break;
         case SSS_PAM_CHAUTHTOK_PRELIM:
+            if (pd->priv == 1 && pd->authtok_size == 0) {
+                DEBUG(4, ("Password reset by root is not supported.\n"));
+                pam_status = PAM_PERM_DENIED;
+                dp_err = DP_ERR_OK;
+                goto done;
+            }
             break;
         case SSS_PAM_ACCT_MGMT:
         case SSS_PAM_SETCRED:
diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
index e0935da..95931ac 100644
--- a/src/providers/ldap/ldap_auth.c
+++ b/src/providers/ldap/ldap_auth.c
@@ -636,6 +636,14 @@ void sdap_pam_chpass_handler(struct be_req *breq)
         goto done;
     }
 
+    if (pd->priv == 1 && pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM &&
+        pd->authtok_size == 0) {
+        DEBUG(4, ("Password reset by root is not supported.\n"));
+        pd->pam_status = PAM_PERM_DENIED;
+        dp_err = DP_ERR_OK;
+        goto done;
+    }
+
     DEBUG(2, ("starting password change request for user [%s].\n", pd->user));
 
     pd->pam_status = PAM_SYSTEM_ERR;
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index 77dec19..eeb6c6d 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -35,6 +35,10 @@
 #include <stdio.h>
 #include <syslog.h>
 #include <time.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <locale.h>
 
 #include <security/pam_modules.h>
 #include <security/pam_ext.h>
@@ -53,6 +57,9 @@
 
 #define PWEXP_FLAG "pam_sss:password_expired_flag"
 
+#define PW_RESET_MSG_FILENAME_TEMPLATE SSSD_CONF_DIR"/customize/%s%spam_sss_pw_reset_message.%s"
+#define PW_RESET_MSG_MAX_SIZE 4096
+
 struct pam_items {
     const char* pam_service;
     const char* pam_user;
@@ -74,6 +81,7 @@ struct pam_items {
     size_t pam_newauthtok_size;
     pid_t cli_pid;
     const char *login_name;
+    char *domain_name;
 };
 
 #define DEBUG_MGS_LEN 1024
@@ -183,7 +191,7 @@ static size_t add_string_item(enum pam_item_type type, const char *str,
     return rp;
 }
 
-static void overwrite_and_free_authtoks(struct pam_items *pi)
+static void overwrite_and_free_pam_items(struct pam_items *pi)
 {
     if (pi->pam_authtok != NULL) {
         _pam_overwrite_n((void *)pi->pam_authtok, pi->pam_authtok_size);
@@ -196,6 +204,8 @@ static void overwrite_and_free_authtoks(struct pam_items *pi)
         free((void *)pi->pam_newauthtok);
         pi->pam_newauthtok = NULL;
     }
+
+    free(pi->domain_name);
 }
 
 static int pack_message_v3(struct pam_items *pi, size_t *size,
@@ -389,6 +399,168 @@ static int do_pam_conversation(pam_handle_t *pamh, const int msg_style,
     return PAM_SUCCESS;
 }
 
+static errno_t display_pw_reset_message(pam_handle_t *pamh,
+                                        const char *domain_name,
+                                        const char *suffix)
+{
+    int ret;
+    struct stat stat_buf;
+    char *msg_buf = NULL;
+    int fd = -1;
+    size_t size;
+    size_t total_len;
+    char *filename = NULL;
+
+    if (strchr(suffix, '/') != NULL) {
+        D(("Suffix contains illegal character."));
+        return EINVAL;
+    }
+
+    size = sizeof(PW_RESET_MSG_FILENAME_TEMPLATE) + strlen(domain_name) +
+           strlen(suffix);
+    filename = malloc(size);
+    if (filename == NULL) {
+        D(("malloc failed."));
+        ret = ENOMEM;
+        goto done;
+    }
+    ret = snprintf(filename, size, PW_RESET_MSG_FILENAME_TEMPLATE, domain_name,
+                   (*domain_name == '\0' ? "" : "/"), suffix);
+    if (ret < 0 || ret >= size) {
+        D(("snprintf failed."));
+        ret = EFAULT;
+        goto done;
+    }
+
+    fd = open(filename, O_RDONLY);
+    if (fd == -1) {
+        ret = errno;
+        D(("open failed [%d][%s].\n", ret, strerror(ret)));
+        goto done;
+    }
+
+    ret = fstat(fd, &stat_buf);
+    if (ret == -1) {
+        ret = errno;
+        D(("fstat failed [%d][%s].", ret, strerror(ret)));
+        goto done;
+    }
+
+    if (!S_ISREG(stat_buf.st_mode)) {
+        logger(pamh, LOG_ERR,
+               "Password reset message file is not a regular file.");
+        ret = EINVAL;
+        goto done;
+    }
+
+    if (stat_buf.st_uid != 0 || stat_buf.st_gid != 0 ||
+        (stat_buf.st_mode & ~S_IFMT) != 0644) {
+        logger(pamh, LOG_ERR,"Permission error, "
+               "file [%s] must be owned by root with permissions 0644.",
+               filename);
+        ret = EPERM;
+        goto done;
+    }
+
+    if (stat_buf.st_size > PW_RESET_MSG_MAX_SIZE) {
+        logger(pamh, LOG_ERR, "Password reset message file is too large.");
+        ret = EFBIG;
+        goto done;
+    }
+
+    msg_buf = malloc(stat_buf.st_size + 1);
+    if (msg_buf == NULL) {
+        D(("malloc failed."));
+        ret = ENOMEM;
+        goto done;
+    }
+
+
+    total_len = 0;
+    while (total_len < stat_buf.st_size) {
+        ret = read(fd, msg_buf + total_len, stat_buf.st_size - total_len);
+        if (ret == -1) {
+            if (errno == EINTR) continue;
+            ret = errno;
+            D(("read failed [%d][%s].", ret, strerror(ret)));
+            goto done;
+        }
+        total_len += ret;
+    }
+
+    ret = close(fd);
+    fd = -1;
+    if (ret == -1) {
+        ret = errno;
+        D(("close failed [%d][%s].", ret, strerror(ret)));
+    }
+
+    if (total_len != stat_buf.st_size) {
+        D(("read fewer bytes [%d] then expected [%d]."));
+        ret = EIO;
+        goto done;
+    }
+
+    msg_buf[stat_buf.st_size] = '\0';
+
+    ret = do_pam_conversation(pamh, PAM_TEXT_INFO, msg_buf, NULL, NULL);
+    if (ret != PAM_SUCCESS) {
+        D(("do_pam_conversation failed."));
+    }
+
+done:
+    if (fd != -1) {
+        close(fd);
+    }
+    free(msg_buf);
+    free(filename);
+
+    return ret;
+}
+
+static errno_t select_pw_reset_message(pam_handle_t *pamh, struct pam_items *pi)
+{
+    int ret;
+    char *locale;
+    const char *domain_name;
+
+    domain_name = pi->domain_name;
+    if (domain_name == NULL) {
+        domain_name = "";
+    }
+
+    locale = setlocale(LC_MESSAGES, NULL);
+
+    /* If available try with domain_name, then fall back to an empty
+    * domain_name and use a the standard message as a last resort. */
+    ret = 0;
+    do {
+        if (ret != 0) {
+            domain_name = "";
+        }
+        ret = -1;
+        if (locale != NULL) {
+            ret = display_pw_reset_message(pamh, domain_name, locale);
+        }
+
+        if (ret != 0) {
+            ret = display_pw_reset_message(pamh, domain_name, "txt");
+        }
+
+    } while (ret != 0 && *domain_name != '\0');
+
+    if (ret != 0) {
+        ret = do_pam_conversation(pamh, PAM_TEXT_INFO,
+                      _("Password reset by root is not supported."),
+                      NULL, NULL);
+        if (ret != PAM_SUCCESS) {
+            D(("do_pam_conversation failed."));
+        }
+    }
+
+    return ret;
+}
+
 static int user_info_offline_auth(pam_handle_t *pamh, size_t buflen,
                                   uint8_t *buf)
 {
@@ -679,7 +851,8 @@ static int eval_user_info_response(pam_handle_t *pamh, size_t buflen,
     return ret;
 }
 
-static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf)
+static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf,
+                         struct pam_items *pi)
 {
     int ret;
     size_t p=0;
@@ -727,7 +900,15 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf)
                 logger(pamh, LOG_INFO, "system info: [%s]", &buf[p]);
                 break;
             case SSS_PAM_DOMAIN_NAME:
+                if (buf[p + (len -1)] != '\0') {
+                    D(("domain name does not end with \\0."));
+                    break;
+                }
                 D(("domain name: [%s]", &buf[p]));
+                pi->domain_name = strdup((char *) &buf[p]);
+                if (pi->domain_name == NULL) {
+                    D(("strdup failed"));
+                }
                 break;
             case SSS_ENV_ITEM:
             case SSS_PAM_ENV_ITEM:
@@ -835,6 +1016,8 @@ static int get_pam_items(pam_handle_t *pamh, struct pam_items *pi)
     pi->login_name = pam_modutil_getlogin(pamh);
     if (pi->login_name == NULL) pi->login_name="";
 
+    pi->domain_name = NULL;
+
     return PAM_SUCCESS;
 }
 
@@ -896,7 +1079,7 @@ static int send_and_receive(pam_handle_t *pamh, struct pam_items *pi,
     }
 
     pam_status = ((int32_t *)repbuf)[0];
-    ret = eval_response(pamh, replen, repbuf);
+    ret = eval_response(pamh, replen, repbuf, pi);
     if (ret != PAM_SUCCESS) {
         D(("eval_response failed."));
         pam_status = ret;
@@ -1211,16 +1394,18 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
                 exp_data = malloc(sizeof(int));
                 if (exp_data == NULL) {
                     D(("malloc failed."));
-                    return PAM_BUF_ERR;
+                    ret = PAM_BUF_ERR;
+                    break;
                 }
                 *exp_data = 1;
                 ret = pam_set_data(pamh, PWEXP_FLAG, exp_data, free_exp_data);
                 if (ret != PAM_SUCCESS) {
                     D(("pam_set_data failed."));
-                    return ret;
+                    ret = ret;
+                    break;
                 }
 
-                return PAM_SUCCESS;
+                ret = PAM_SUCCESS;
             }
             break;
         case SSS_PAM_ACCT_MGMT:
@@ -1233,7 +1418,7 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
                 if (ret != PAM_SUCCESS) {
                     D(("do_pam_conversation failed."));
                 }
-                return PAM_NEW_AUTHTOK_REQD;
+                ret = PAM_NEW_AUTHTOK_REQD;
             }
             break;
         case SSS_PAM_CHAUTHTOK:
@@ -1250,6 +1435,17 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
                 }
             }
             break;
+        case SSS_PAM_CHAUTHTOK_PRELIM:
+            if (ret == PAM_PERM_DENIED && pi.pam_authtok_size == 0 &&
+                getuid() == 0 &&
+                pam_get_data(pamh, PWEXP_FLAG, (const void **) &exp_data) !=
+                                                                  PAM_SUCCESS) {
+
+                ret = select_pw_reset_message(pamh, &pi);
+                if (ret != 0) {
+                }
+                ret = PAM_PERM_DENIED;
+            }
         default:
             /* nothing to do */
             break;
@@ -1257,7 +1453,7 @@ static int pam_sss(enum sss_cli_command task, pam_handle_t *pamh,
 
 
 
-    overwrite_and_free_authtoks(&pi);
+    overwrite_and_free_pam_items(&pi);
 
     return ret;
 }
-- 
1.6.6.1



More information about the sssd-devel mailing list