[SSSD] [PATCH] Two enhancements for PAM client
Sumit Bose
sbose at redhat.com
Mon Apr 26 12:50:14 UTC 2010
On Mon, Apr 26, 2010 at 08:13:40AM -0400, Stephen Gallagher wrote:
> On 04/26/2010 07:41 AM, Sumit Bose wrote:
> > 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
> >
>
> Nack.
>
> Minor nitpick: s/then/than/ in "read fewer bytes [%d] then expected
> [%d]." And also, you do not have any variables there to handle the two
> %d substitutions.
>
done
> Please explain why you would not have a domain_name in
> select_pw_reset_message(). By the time you have received a message from
> PAM that the password needs to be reset, you should absolutely know
> which domain you're in. If domain is NULL here, that's almost certainly
> an error. I don't think it should be possible to specify
> /etc/sssd/customize/root_pw_reset.* This is unneeded complexity, since
> we will always have a true default message, and we should never need
> this if, as mentioned above, we're properly detecting the domain.
>
I removed the fallback path and made a missing domain name an error.
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 1c8248e1ea3575771841a4546db4322d44e61427 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 | 22 ++++
src/providers/krb5/krb5_auth.c | 7 ++
src/providers/ldap/ldap_auth.c | 8 ++
src/sss_client/pam_sss.c | 206 ++++++++++++++++++++++++++++++++++++++--
4 files changed, 235 insertions(+), 8 deletions(-)
diff --git a/src/man/pam_sss.8.xml b/src/man/pam_sss.8.xml
index f6ac9f4..9a68196 100644
--- a/src/man/pam_sss.8.xml
+++ b/src/man/pam_sss.8.xml
@@ -85,6 +85,28 @@
</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>. 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..3f88d68 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/pam_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,162 @@ 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 || strchr(domain_name, '/') != NULL) {
+ D(("Suffix [%s] or domain name [%s] contain illegal character.", suffix,
+ domain_name));
+ 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,
+ 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] than expected [%d].", total_len,
+ stat_buf.st_size));
+ 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 == '\0') {
+ D(("Domain name is unknown."));
+ return EINVAL;
+ }
+
+ locale = setlocale(LC_MESSAGES, NULL);
+
+ 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");
+ }
+
+ 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 +845,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 +894,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 +1010,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 +1073,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 +1388,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 +1412,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 +1429,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 +1447,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