[SSSD] [PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user
Jakub Hrozek
jhrozek at redhat.com
Wed Nov 5 17:15:30 UTC 2014
On Fri, Oct 31, 2014 at 08:15:13PM +0100, Michal Židek wrote:
> These patches look good to me. If the nitpicks that Pavel found
> are fixed, I'll ack.
>
> Michal
OK, see the attached patches. Thank you both for the review.
-------------- next part --------------
>From 9f2a8116ab203e9c5b1b4c2ebd8741b844cc3801 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sat, 11 Oct 2014 20:22:42 +0200
Subject: [PATCH 1/4] BUILD: Install ldap_child and as setuid if running under
non-privileged user
The ldap_child permissions should be 4750, owned by root.sssd,
to make sure only root and sssd can execute the child and if executed by
sssd, the child will run as root.
---
Makefile.am | 5 +++++
contrib/sssd.spec.in | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am
index 38d689bcef9f99b29da72fedda35056b83408ea9..405c5b281849d9a626c47da8e4f387093ffe68f7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2845,6 +2845,11 @@ else
$(MKDIR_P) $(DESTDIR)$(initdir)
endif
+if SSSD_USER
+ chgrp $(SSSD_USER) $(sssdlibexecdir)/ldap_child
+ chmod 4750 $(sssdlibexecdir)/ldap_child
+endif
+
install-data-hook:
rm $(DESTDIR)/$(nsslibdir)/libnss_sss.so.2 \
$(DESTDIR)/$(nsslibdir)/libnss_sss.so
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index c1c76db25f738b5a41d63c2796d733255f8f7a94..1ffe09c9819b36bdd37cceba895e14affdc8dc3f 100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -645,7 +645,7 @@ rm -rf $RPM_BUILD_ROOT
%defattr(-,root,root,-)
%doc COPYING
%{_libdir}/%{name}/libsss_krb5_common.so
-%{_libexecdir}/%{servicename}/ldap_child
+%attr(4750,root,sssd) %{_libexecdir}/%{servicename}/ldap_child
%{_libexecdir}/%{servicename}/krb5_child
%files krb5 -f sssd_krb5.lang
--
1.9.3
-------------- next part --------------
>From bb1328ebe490d4a2795edf8eae3960bdd73c2c7d Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sat, 11 Oct 2014 17:39:21 +0200
Subject: [PATCH 2/4] LDAP: Move sss_krb5_verify_keytab_ex to ldap_child
The function was called from one place only, so it makes no sense to
keep it in a shared module. Moreover, the function should only be
called from code that runs as root.
---
src/providers/ldap/ldap_child.c | 79 ++++++++++++++++++++++++++++++++++++++++-
src/util/sss_krb5.c | 76 ---------------------------------------
src/util/sss_krb5.h | 3 --
3 files changed, 78 insertions(+), 80 deletions(-)
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index e5779b70906d90ab855677f04a154e179f2163c6..b8b4b0ad7cfffc7db52b5ca3d9b9a74f12480070 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -160,6 +160,83 @@ set_child_debugging(krb5_context ctx)
return EOK;
}
+static int lc_verify_keytab_ex(const char *principal,
+ const char *keytab_name,
+ krb5_context context,
+ krb5_keytab keytab)
+{
+ bool found;
+ char *kt_principal;
+ krb5_error_code krberr;
+ krb5_kt_cursor cursor;
+ krb5_keytab_entry entry;
+
+ krberr = krb5_kt_start_seq_get(context, keytab, &cursor);
+ if (krberr) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ "Cannot read keytab [%s].\n", KEYTAB_CLEAN_NAME);
+
+ sss_log(SSS_LOG_ERR, "Error reading keytab file [%s]: [%d][%s]. "
+ "Unable to create GSSAPI-encrypted LDAP "
+ "connection.",
+ KEYTAB_CLEAN_NAME, krberr,
+ sss_krb5_get_error_message(context, krberr));
+
+ return EIO;
+ }
+
+ found = false;
+ while ((krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0) {
+ krberr = krb5_unparse_name(context, entry.principal, &kt_principal);
+ if (krberr) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ "Could not parse keytab entry\n");
+ sss_log(SSS_LOG_ERR, "Could not parse keytab entry\n");
+ return EIO;
+ }
+
+ if (strcmp(principal, kt_principal) == 0) {
+ found = true;
+ }
+ free(kt_principal);
+ krberr = sss_krb5_free_keytab_entry_contents(context, &entry);
+ if (krberr) {
+ /* This should never happen. The API docs for this function
+ * specify only success for this function
+ */
+ DEBUG(SSSDBG_CRIT_FAILURE,"Could not free keytab entry contents\n");
+ /* This is non-fatal, so we'll continue here */
+ }
+
+ if (found) {
+ break;
+ }
+ }
+
+ krberr = krb5_kt_end_seq_get(context, keytab, &cursor);
+ if (krberr) {
+ DEBUG(SSSDBG_FATAL_FAILURE, "Could not close keytab.\n");
+ sss_log(SSS_LOG_ERR, "Could not close keytab file [%s].",
+ KEYTAB_CLEAN_NAME);
+ return EIO;
+ }
+
+ if (!found) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ "Principal [%s] not found in keytab [%s]\n",
+ principal,
+ KEYTAB_CLEAN_NAME);
+ sss_log(SSS_LOG_ERR, "Error processing keytab file [%s]: "
+ "Principal [%s] was not found. "
+ "Unable to create GSSAPI-encrypted LDAP connection.",
+ KEYTAB_CLEAN_NAME, principal);
+
+ return EFAULT;
+ }
+
+ return EOK;
+}
+
static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
const char *realm_str,
const char *princ_str,
@@ -287,7 +364,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
}
/* Verify the keytab */
- ret = sss_krb5_verify_keytab_ex(full_princ, keytab_name, context, keytab);
+ ret = lc_verify_keytab_ex(full_princ, keytab_name, context, keytab);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE,
"Unable to verify principal is present in the keytab\n");
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index d9b076ebeccbcd571f14e1b642cade86b21fd5d7..377d61e47254133a6c1b6ed2f20dc0ef1a572aa3 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -249,82 +249,6 @@ done:
return ret;
}
-int sss_krb5_verify_keytab_ex(const char *principal, const char *keytab_name,
- krb5_context context, krb5_keytab keytab)
-{
- bool found;
- char *kt_principal;
- krb5_error_code krberr;
- krb5_kt_cursor cursor;
- krb5_keytab_entry entry;
-
- krberr = krb5_kt_start_seq_get(context, keytab, &cursor);
- if (krberr) {
- DEBUG(SSSDBG_FATAL_FAILURE,
- "Cannot read keytab [%s].\n", KEYTAB_CLEAN_NAME);
-
- sss_log(SSS_LOG_ERR, "Error reading keytab file [%s]: [%d][%s]. "
- "Unable to create GSSAPI-encrypted LDAP "
- "connection.",
- KEYTAB_CLEAN_NAME, krberr,
- sss_krb5_get_error_message(context, krberr));
-
- return EIO;
- }
-
- found = false;
- while((krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0){
- krberr = krb5_unparse_name(context, entry.principal, &kt_principal);
- if (krberr) {
- DEBUG(SSSDBG_FATAL_FAILURE,
- "Could not parse keytab entry\n");
- sss_log(SSS_LOG_ERR, "Could not parse keytab entry\n");
- return EIO;
- }
-
- if (strcmp(principal, kt_principal) == 0) {
- found = true;
- }
- free(kt_principal);
- krberr = sss_krb5_free_keytab_entry_contents(context, &entry);
- if (krberr) {
- /* This should never happen. The API docs for this function
- * specify only success for this function
- */
- DEBUG(SSSDBG_CRIT_FAILURE,"Could not free keytab entry contents\n");
- /* This is non-fatal, so we'll continue here */
- }
-
- if (found) {
- break;
- }
- }
-
- krberr = krb5_kt_end_seq_get(context, keytab, &cursor);
- if (krberr) {
- DEBUG(SSSDBG_FATAL_FAILURE, "Could not close keytab.\n");
- sss_log(SSS_LOG_ERR, "Could not close keytab file [%s].",
- KEYTAB_CLEAN_NAME);
- return EIO;
- }
-
- if (!found) {
- DEBUG(SSSDBG_FATAL_FAILURE,
- "Principal [%s] not found in keytab [%s]\n",
- principal,
- KEYTAB_CLEAN_NAME);
- sss_log(SSS_LOG_ERR, "Error processing keytab file [%s]: "
- "Principal [%s] was not found. "
- "Unable to create GSSAPI-encrypted LDAP connection.",
- KEYTAB_CLEAN_NAME, principal);
-
- return EFAULT;
- }
-
- return EOK;
-}
-
-
enum matching_mode {MODE_NORMAL, MODE_PREFIX, MODE_POSTFIX};
/**
* We only have primary and instances stored separately, we need to
diff --git a/src/util/sss_krb5.h b/src/util/sss_krb5.h
index 83c72097594dc24de1f8ac93d5394b6766a449f4..afa0d1943d8a23ae1543ae3874b5abbfbb4b3372 100644
--- a/src/util/sss_krb5.h
+++ b/src/util/sss_krb5.h
@@ -70,9 +70,6 @@ void KRB5_CALLCONV sss_krb5_get_init_creds_opt_free (krb5_context context,
void KRB5_CALLCONV sss_krb5_free_unparsed_name(krb5_context context, char *name);
-int sss_krb5_verify_keytab_ex(const char *principal, const char *keytab_name,
- krb5_context context, krb5_keytab keytab);
-
krb5_error_code find_principal_in_keytab(krb5_context ctx,
krb5_keytab keytab,
const char *pattern_primary,
--
1.9.3
-------------- next part --------------
>From a2a56ad497d9ff8dd06ee987dab742dcece39f03 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 19 Oct 2014 19:20:28 +0200
Subject: [PATCH 3/4] LDAP: read the correct data type from ldap_child's input
buffer
The back end wrote uint32_t, the ldap_child process would read int32_t.
---
src/providers/ldap/ldap_child.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index b8b4b0ad7cfffc7db52b5ca3d9b9a74f12480070..e1abc9fd73f2ae95f0a0c28159589ebd36d2cf06 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -96,8 +96,8 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
}
/* ticket lifetime */
- SAFEALIGN_COPY_INT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %d\n", ibuf->lifetime);
+ SAFEALIGN_COPY_UINT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
+ DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %u\n", ibuf->lifetime);
return EOK;
}
--
1.9.3
-------------- next part --------------
>From 0931969ee8ab41b7fb6cf68834c57adb33ac17e0 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 19 Oct 2014 19:15:52 +0200
Subject: [PATCH 4/4] LDAP: Drop privileges after kinit in ldap_child
After ldap_child initializes privileges using root-owned keytab, it
drops privileges to the SSSD user, minimizing the amount of code that
runs as root.
---
Makefile.am | 4 +-
src/providers/ldap/ldap_child.c | 96 ++++++++++++++++++++-------------
src/providers/ldap/sdap_child_helpers.c | 8 ++-
3 files changed, 70 insertions(+), 38 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 405c5b281849d9a626c47da8e4f387093ffe68f7..4182a09ea078ba874dc708a57bd6bf06475bdf34 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2512,7 +2512,9 @@ ldap_child_SOURCES = \
src/util/atomic_io.c \
src/util/authtok.c \
src/util/util.c \
- src/util/signal.c
+ src/util/signal.c \
+ src/util/become_user.c \
+ $(NULL)
ldap_child_CFLAGS = \
$(AM_CFLAGS) \
$(POPT_CFLAGS) \
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index e1abc9fd73f2ae95f0a0c28159589ebd36d2cf06..a922b181715c5e89301e9f50bdb81723d1ff2a6a 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -49,6 +49,8 @@ struct input_buffer {
const char *princ_str;
const char *keytab_name;
krb5_deltat lifetime;
+ uid_t uid;
+ gid_t gid;
};
static errno_t unpack_buffer(uint8_t *buf, size_t size,
@@ -99,6 +101,12 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size,
SAFEALIGN_COPY_UINT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %u\n", ibuf->lifetime);
+ /* UID and GID to run as */
+ SAFEALIGN_COPY_UINT32_CHECK(&ibuf->uid, buf + p, size, &p);
+ SAFEALIGN_COPY_UINT32_CHECK(&ibuf->gid, buf + p, size, &p);
+ DEBUG(SSSDBG_FUNC_DATA,
+ "Will run as [%"SPRIuid"][%"SPRIgid"].\n", ibuf->uid, ibuf->gid);
+
return EOK;
}
@@ -242,6 +250,8 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
const char *princ_str,
const char *keytab_name,
const krb5_deltat lifetime,
+ uid_t uid,
+ gid_t gid,
const char **ccname_out,
time_t *expire_time_out)
{
@@ -372,42 +382,6 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
goto done;
}
- ccname_file_dummy = talloc_asprintf(tmp_ctx, "%s/ccache_%s_XXXXXX",
- DB_PATH, realm_name);
- ccname_file = talloc_asprintf(tmp_ctx, "%s/ccache_%s",
- DB_PATH, realm_name);
- if (ccname_file_dummy == NULL || ccname_file == NULL) {
- ret = ENOMEM;
- goto done;
- }
-
- old_umask = umask(077);
- fd = mkstemp(ccname_file_dummy);
- umask(old_umask);
- if (fd == -1) {
- ret = errno;
- goto done;
- }
- /* We only care about creating a unique file name here, we don't
- * need the fd
- */
- close(fd);
-
- ccname_dummy = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file_dummy);
- ccname = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file);
- if (ccname_dummy == NULL || ccname == NULL) {
- krberr = ENOMEM;
- goto done;
- }
- DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname_dummy);
-
- krberr = krb5_cc_resolve(context, ccname_dummy, &ccache);
- if (krberr) {
- DEBUG(SSSDBG_OP_FAILURE, "Failed to set cache name: %s\n",
- sss_krb5_get_error_message(context, krberr));
- goto done;
- }
-
memset(&my_creds, 0, sizeof(my_creds));
memset(&options, 0, sizeof(options));
@@ -423,8 +397,36 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
}
sss_krb5_get_init_creds_opt_set_canonicalize(&options, canonicalize);
+ ccname_file = talloc_asprintf(tmp_ctx, "%s/ccache_%s",
+ DB_PATH, realm_name);
+ if (ccname_file == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ccname_file_dummy = talloc_asprintf(tmp_ctx, "%s/ccache_%s_XXXXXX",
+ DB_PATH, realm_name);
+ if (ccname_file_dummy == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ old_umask = umask(077);
+ fd = mkstemp(ccname_file_dummy);
+ umask(old_umask);
+ if (fd == -1) {
+ ret = errno;
+ goto done;
+ }
+ /* We only care about creating a unique file name here, we don't
+ * need the fd
+ */
+ close(fd);
+
krberr = krb5_get_init_creds_keytab(context, &my_creds, kprinc,
keytab, 0, NULL, &options);
+ krb5_kt_close(context, keytab);
+ keytab = NULL;
if (krberr) {
DEBUG(SSSDBG_FATAL_FAILURE,
"Failed to init credentials: %s\n",
@@ -438,6 +440,27 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
}
DEBUG(SSSDBG_TRACE_INTERNAL, "credentials initialized\n");
+ krberr = become_user(uid, gid);
+ if (krberr != 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
+ goto done;
+ }
+
+ ccname_dummy = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file_dummy);
+ ccname = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file);
+ if (ccname_dummy == NULL || ccname == NULL) {
+ krberr = ENOMEM;
+ goto done;
+ }
+ DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname_dummy);
+
+ krberr = krb5_cc_resolve(context, ccname_dummy, &ccache);
+ if (krberr) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to set cache name: %s\n",
+ sss_krb5_get_error_message(context, krberr));
+ goto done;
+ }
+
/* Use updated principal if changed due to canonicalization. */
krberr = krb5_cc_initialize(context, ccache, my_creds.client);
if (krberr) {
@@ -643,6 +666,7 @@ int main(int argc, const char *argv[])
kerr = ldap_child_get_tgt_sync(main_ctx,
ibuf->realm_str, ibuf->princ_str,
ibuf->keytab_name, ibuf->lifetime,
+ ibuf->uid, ibuf->gid,
&ccname, &expire_time);
if (kerr != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "ldap_child_get_tgt_sync failed.\n");
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index 448c5af10e9da8949bcaa39d8a3fa05ec309e16f..e5d46b9b756cd50fadb212da72ad1cc9bdd93330 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -152,7 +152,7 @@ static errno_t create_tgt_req_send_buffer(TALLOC_CTX *mem_ctx,
return ENOMEM;
}
- buf->size = 4 * sizeof(uint32_t);
+ buf->size = 6 * sizeof(uint32_t);
if (realm_str) {
buf->size += strlen(realm_str);
}
@@ -201,6 +201,12 @@ static errno_t create_tgt_req_send_buffer(TALLOC_CTX *mem_ctx,
/* lifetime */
SAFEALIGN_SET_UINT32(&buf->data[rp], lifetime, &rp);
+ /* UID and GID to drop privileges to, if needed. The ldap_child process runs as
+ * setuid if the back end runs unprivileged as it needs to access the keytab
+ */
+ SAFEALIGN_SET_UINT32(&buf->data[rp], geteuid(), &rp);
+ SAFEALIGN_SET_UINT32(&buf->data[rp], getegid(), &rp);
+
*io_buf = buf;
return EOK;
}
--
1.9.3
More information about the sssd-devel
mailing list