URL: https://github.com/SSSD/sssd/pull/616
Author: asheplyakov
Title: #616: become_user: add supplementary groups so ad provider can access
keytab
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/616/head:pr616
git checkout pr616
From 25ed58a1c020accf8955ba35b35026a95c1da28d Mon Sep 17 00:00:00 2001
From: Alexey Sheplyakov <asheplya...@altlinux.org>
Date: Tue, 10 Jul 2018 15:42:31 +0000
Subject: [PATCH] become_user_ex: add supplementary groups so ad provider can
access keytab
For security reasons one might want to run providers as a non-privileged
user (say, _sssd). However some providers (in particular ad) might need
an access to restricted (non world-readable) files (for instance,
/etc/krb5.keytab). One of the possible ways to solve the problem is to
- add a special group (for instance, _keytab)
- set the owner:group of the file in question to root:_keytab
- set the permissions of the file in question to 640
- make the _sssd user a member of the _keytab group
For this to work it's necessary to assign supplementary groups when switching
to a non-privileged user. However in some contexts finding out supplementary
groups might be too expensive/inappropriate (case in point: `krb5_child`
switching to the user being authenticated). To solve the problem add a new
function `become_user_ex`, and call it where appropriate, i.e. `server_setup`
and `ldap_child`.
Note: `ldap_child` can use the same approach to avoid being SUID (when sssd
runs as a non-privileged user). However that's a subject of a separate patch.
---
src/providers/ldap/ldap_child.c | 2 +-
src/tests/cwrap/group | 1 +
src/tests/cwrap/passwd | 1 +
src/tests/cwrap/test_become_user.c | 88 ++++++++++++++++++++++++++++++++++++++
src/util/become_user.c | 42 ++++++++++++++----
src/util/server.c | 2 +-
src/util/util.h | 1 +
7 files changed, 127 insertions(+), 10 deletions(-)
diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index 8c11d7896f..c65b8ed6a2 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -701,7 +701,7 @@ int main(int argc, const char *argv[])
}
DEBUG(SSSDBG_TRACE_INTERNAL, "Kerberos context initialized\n");
- kerr = become_user(ibuf->uid, ibuf->gid);
+ kerr = become_user_ex(ibuf->uid, ibuf->gid, true);
if (kerr != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
goto fail;
diff --git a/src/tests/cwrap/group b/src/tests/cwrap/group
index d0cea659ea..e84e23ed49 100644
--- a/src/tests/cwrap/group
+++ b/src/tests/cwrap/group
@@ -1,2 +1,3 @@
sssd:x:123:
+_keytab:x:321:_sssd
foogroup:x:10001:
diff --git a/src/tests/cwrap/passwd b/src/tests/cwrap/passwd
index 862ccfe03e..4fe01c619b 100644
--- a/src/tests/cwrap/passwd
+++ b/src/tests/cwrap/passwd
@@ -1,2 +1,3 @@
sssd:x:123:456:sssd unprivileged user:/:/sbin/nologin
+_sssd:x:987:654:sssd unprivileged user:/:/sbin/nologin
foobar:x:10001:10001:User for SSSD testing:/home/foobar:/bin/bash
diff --git a/src/tests/cwrap/test_become_user.c b/src/tests/cwrap/test_become_user.c
index e63cde9d72..52a9123e5c 100644
--- a/src/tests/cwrap/test_become_user.c
+++ b/src/tests/cwrap/test_become_user.c
@@ -70,6 +70,93 @@ void test_become_user(void **state)
assert_int_equal(WEXITSTATUS(status), 0);
}
+void test_become_user_ex(void **state)
+{
+ struct passwd *sssd;
+ TALLOC_CTX *tmp_ctx;
+ gid_t *groups, *actual_groups;
+ errno_t ret;
+ pid_t pid, wpid;
+ int status;
+ int group_count, actual_group_count;
+
+ assert_true(leak_check_setup());
+
+ tmp_ctx = talloc_new(global_talloc_context);
+ assert_non_null(tmp_ctx);
+ check_leaks_push(tmp_ctx);
+
+ /* Must root as root, real or fake */
+ assert_int_equal(geteuid(), 0);
+
+ sssd = getpwnam("_sssd");
+ assert_non_null(sssd);
+
+ getgrouplist(sssd->pw_name, sssd->pw_gid, NULL, &group_count);
+ assert_int_not_equal(group_count, 0);
+ groups = talloc_array(tmp_ctx, gid_t, group_count);
+ assert_non_null(groups);
+ actual_groups = talloc_array(tmp_ctx, gid_t, group_count);
+ assert_non_null(actual_groups);
+ status = getgrouplist(sssd->pw_name, sssd->pw_gid, groups, &group_count);
+ assert_int_equal(status, group_count);
+
+ pid = fork();
+ if (pid == 0) {
+ /* Change the UID in a child */
+ ret = become_user_ex(sssd->pw_uid, sssd->pw_gid, true);
+ assert_int_equal(ret, EOK);
+
+ /* Make sure we have the requested UID and GID now and there
+ * are no supplementary groups
+ */
+ assert_int_equal(geteuid(), sssd->pw_uid);
+ assert_int_equal(getegid(), sssd->pw_gid);
+ assert_int_equal(getuid(), sssd->pw_uid);
+ assert_int_equal(getgid(), sssd->pw_gid);
+
+ /* Another become_user is a no-op */
+ ret = become_user(sssd->pw_uid, sssd->pw_gid);
+ assert_int_equal(ret, EOK);
+
+ actual_group_count = getgroups(0, NULL);
+ assert_int_equal(actual_group_count, group_count);
+ status = getgroups(actual_group_count, actual_groups);
+ assert_int_not_equal(status, -1);
+
+ for (int i = 0; i < group_count; i++) {
+ bool found_group = false;
+ if (actual_groups[i] == sssd->pw_gid) {
+ /* in general getgroups is not guaranteed to return effective gid */
+ continue;
+ }
+ for (int j = 0; j < group_count; j++) {
+ if (groups[j] != actual_groups[i]) {
+ continue;
+ } else {
+ found_group = true;
+ break;
+ }
+ }
+ assert_true(found_group);
+ }
+ exit(0);
+ }
+
+ assert_int_not_equal(pid, -1);
+
+ wpid = waitpid(pid, &status, 0);
+ assert_int_equal(wpid, pid);
+ assert_true(WIFEXITED(status));
+ assert_int_equal(WEXITSTATUS(status), 0);
+
+ talloc_free(groups);
+ talloc_free(actual_groups);
+ assert_true(check_leaks_pop(tmp_ctx));
+ talloc_free(tmp_ctx);
+ assert_true(leak_check_teardown());
+}
+
void test_switch_user(void **state)
{
errno_t ret;
@@ -138,6 +225,7 @@ int main(int argc, const char *argv[])
const struct CMUnitTest tests[] = {
cmocka_unit_test(test_become_user),
cmocka_unit_test(test_switch_user),
+ cmocka_unit_test(test_become_user_ex),
};
/* Set debug level to invalid value so we can decide if -d 0 was used. */
diff --git a/src/util/become_user.c b/src/util/become_user.c
index c3f726d189..e2cad7e20f 100644
--- a/src/util/become_user.c
+++ b/src/util/become_user.c
@@ -24,11 +24,13 @@
#include "util/util.h"
#include <grp.h>
+#include <pwd.h>
-errno_t become_user(uid_t uid, gid_t gid)
+errno_t become_user_ex(uid_t uid, gid_t gid, bool suppl_groups)
{
uid_t cuid;
int ret;
+ struct passwd *pwd;
DEBUG(SSSDBG_FUNC_DATA,
"Trying to become user [%"SPRIuid"][%"SPRIgid"].\n", uid, gid);
@@ -40,13 +42,32 @@ errno_t become_user(uid_t uid, gid_t gid)
return EOK;
}
- /* drop supplementary groups first */
- ret = setgroups(0, NULL);
- if (ret == -1) {
- ret = errno;
- DEBUG(SSSDBG_CRIT_FAILURE,
- "setgroups failed [%d][%s].\n", ret, strerror(ret));
- return ret;
+ if (suppl_groups) {
+ /* init supplmentary groups */
+ errno = 0;
+ pwd = getpwuid(uid);
+ if (pwd == NULL || pwd->pw_name == NULL) {
+ ret = errno ?: ENOENT;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "getpwuid failed [%d][%s].\n", ret, strerror(ret));
+ return ret;
+ }
+ ret = initgroups(pwd->pw_name, gid);
+ if (ret == -1) {
+ ret = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "initgroups failed [%d][%s].\n", ret, strerror(ret));
+ return ret;
+ }
+ } else {
+ /* drop supplementary groups */
+ ret = setgroups(0, NULL);
+ if (ret == -1) {
+ ret = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "setgroups failed [%d][%s].\n", ret, strerror(ret));
+ return ret;
+ }
}
/* change GID so that root cannot be regained (changes saved GID too) */
@@ -71,6 +92,11 @@ errno_t become_user(uid_t uid, gid_t gid)
return EOK;
}
+errno_t become_user(uid_t uid, gid_t gid)
+{
+ return become_user_ex(uid, gid, false);
+}
+
struct sss_creds {
uid_t uid;
gid_t gid;
diff --git a/src/util/server.c b/src/util/server.c
index f34bf49f6b..7b544971e8 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -484,7 +484,7 @@ int server_setup(const char *name, int flags,
"Cannot chown the debug files, debugging might not work!\n");
}
- ret = become_user(uid, gid);
+ ret = become_user_ex(uid, gid, true);
if (ret != EOK) {
DEBUG(SSSDBG_FUNC_DATA,
"Cannot become user [%"SPRIuid"][%"SPRIgid"].\n", uid, gid);
diff --git a/src/util/util.h b/src/util/util.h
index bc89ecbc25..61b42a1a85 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -641,6 +641,7 @@ char **concatenate_string_array(TALLOC_CTX *mem_ctx,
char **arr2, size_t len2);
/* from become_user.c */
+errno_t become_user_ex(uid_t uid, gid_t gid, bool suppl_groups);
errno_t become_user(uid_t uid, gid_t gid);
struct sss_creds;
errno_t switch_creds(TALLOC_CTX *mem_ctx,
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/KH4S5ZFMUBGB4KG25WNMUSIV7IA5AND2/
