Hi, this patch added the checks requested in ticket #1382 to the PAC responder. The check itself can be found in the commom responder code. It can be used by all responder, but currently only the PAC responder uses it.
I took a quite strict default here, i.e. only root is allowed to access the PAC responder by default. Is this too restrictive? bye, Sumit
From f3918008bbcd9bca54cd10cdd57750a39f32c708 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 5 Jul 2012 10:50:08 +0200 Subject: [PATCH] pac responder: limit access by checking UIDs A check for allowed UIDs is added in the common responder code directly after accept(). If the platform does not support reading the UID of the peer but allowed UIDs are configured, access is denied. Currently only the PAC responder sets the allowed UIDs for a socket. The default is that only root is allowed to access the socket of the PAC responder. Fixes: https://fedorahosted.org/sssd/ticket/1382 --- Makefile.am | 17 +++- src/confdb/confdb.h | 1 + src/config/SSSDConfig/__init__.py.in | 3 + src/config/SSSDConfigTest.py | 6 +- src/config/etc/sssd.api.conf | 4 + src/man/sssd.conf.5.xml | 29 ++++- src/responder/common/responder.h | 9 ++ src/responder/common/responder_common.c | 113 ++++++++++++++++++- src/responder/pac/pacsrv.c | 19 +++ src/tests/responder_socket_access-tests.c | 177 +++++++++++++++++++++++++++++ 10 files changed, 367 insertions(+), 11 deletions(-) create mode 100644 src/tests/responder_socket_access-tests.c diff --git a/Makefile.am b/Makefile.am index 1848d73..1e30b32 100644 --- a/Makefile.am +++ b/Makefile.am @@ -122,7 +122,8 @@ if HAVE_CHECK util-tests \ debug-tests \ ipa_hbac-tests \ - sss_idmap-tests + sss_idmap-tests \ + responder_socket_access-tests if BUILD_PAC_RESPONDER non_interactive_check_based_tests += pac_responder-tests @@ -1015,6 +1016,20 @@ pac_responder_tests_LDADD = \ libsss_debug.la \ libsss_util.la \ libsss_test_common.la + +responder_socket_access_tests_SOURCES = \ + src/tests/responder_socket_access-tests.c \ + src/responder/common/responder_common.c \ + src/responder/common/responder_packet.c \ + src/responder/common/responder_cmd.c +responder_socket_access_tests_CFLAGS = \ + $(AM_CFLAGS) \ + $(CHECK_CFLAGS) +responder_socket_access_tests_LDADD = \ + $(CHECK_LIBS) \ + $(TALLOC_LIBS) \ + libsss_test_common.la \ + libsss_util.la endif stress_tests_SOURCES = \ diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index cdbd1ff..4996d22 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -57,6 +57,7 @@ #define CONFDB_SERVICE_FORCE_TIMEOUT "force_timeout" #define CONFDB_SERVICE_RECON_RETRIES "reconnection_retries" #define CONFDB_SERVICE_FD_LIMIT "fd_limit" +#define CONFDB_SERVICE_ALLOWED_UIDS "allowed_uids" /* Monitor */ #define CONFDB_MONITOR_CONF_ENTRY "config/sssd" diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index 99ccc5a..4d0fc38 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -88,6 +88,9 @@ option_strings = { # [ssh] 'ssh_hash_known_hosts': _('Whether to hash host names and addresses in the known_hosts file'), + # [pac] + 'allowed_uids': _('List of UIDs allowed to access the PAC responder'), + # [provider] 'id_provider' : _('Identity provider'), 'auth_provider' : _('Authentication provider'), diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index f4d4d54..bcfbb78 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -1177,7 +1177,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'pam', 'sudo', 'autofs', - 'ssh'] + 'ssh', + 'pac'] for section in control_list: self.assertTrue(sssdconfig.has_section(section), "Section [%s] missing" % @@ -1269,7 +1270,8 @@ class SSSDConfigTestSSSDConfig(unittest.TestCase): 'nss', 'sudo', 'autofs', - 'ssh'] + 'ssh', + 'pac'] service_list = sssdconfig.list_services() for service in control_list: self.assertTrue(service in service_list, diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index f1cd067..35ebb2e 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -63,6 +63,10 @@ autofs_negative_timeout = int, None, false # ssh service ssh_hash_known_hosts = bool, None, false +[pac] +# PAC responder +allowed_uids = str, None, false + [provider] #Available provider types id_provider = str, None, true diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index bb9b912..f67c9d8 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -792,10 +792,6 @@ <refsect2 id='PAC_RESPONDER' condition="with_pac_responder"> <title>PAC responder configuration options</title> <para> - Currently there are no PAC responder specific configuration - options. - </para> - <para> <xi:include xmlns:xi="http://www.w3.org/2001/XInclude"; href="include/experimental.xml" /> </para> @@ -822,6 +818,31 @@ groups.</para></listitem> </itemizedlist> </para> + <para> + These options can be used to configure the PAC responder. + </para> + <variablelist> + <varlistentry> + <term>allowed_uids (string)</term> + <listitem> + <para> + Specifies the comma-separated list of UID values + that are allowed to access the PAC responder. + </para> + <para> + Default: 0 (only the root user is allowed to access + the PAC responder) + </para> + <para> + Please note that although the UID 0 is used as the + default it will be overwritten with this option. If + you still want to allow the root user to access the + PAC responder, which would be the typical case, you + have to add 0 to the list of allowed UIDs as well. + </para> + </listitem> + </varlistentry> + </variablelist> </refsect2> </refsect1> diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index 43a4fa0..d19902b 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -98,6 +98,9 @@ struct resp_ctx { struct timeval get_domains_last_call; + size_t allowed_uids_count; + uid_t *allowed_uids; + void *pvt_ctx; }; @@ -289,4 +292,10 @@ struct tevent_req *sss_dp_get_domains_send(TALLOC_CTX *mem_ctx, const char *hint); errno_t sss_dp_get_domains_recv(struct tevent_req *req); + +errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *cvs_string, + size_t *_uid_count, uid_t **_uids); + +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count, + uid_t *allowed_uids); #endif /* __SSS_RESPONDER_H__ */ diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index e557740..fab378c 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -33,6 +33,7 @@ #include <errno.h> #include <popt.h> #include "util/util.h" +#include "util/strtonum.h" #include "db/sysdb.h" #include "confdb/confdb.h" #include "dbus/dbus.h" @@ -104,15 +105,15 @@ static int client_destructor(struct cli_ctx *ctx) static errno_t get_client_cred(struct cli_ctx *cctx) { + cctx->client_euid = -1; + cctx->client_egid = -1; + cctx->client_pid = -1; + #ifdef HAVE_UCRED int ret; struct ucred client_cred; socklen_t client_cred_len = sizeof(client_cred); - cctx->client_euid = -1; - cctx->client_egid = -1; - cctx->client_pid = -1; - ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &client_cred, &client_cred_len); if (ret != EOK) { @@ -136,6 +137,84 @@ static errno_t get_client_cred(struct cli_ctx *cctx) return EOK; } +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count, + uid_t *allowed_uids) +{ + size_t c; + + if (allowed_uids == NULL) { + return EINVAL; + } + + for (c = 0; c < allowed_uids_count; c++) { + if (uid == allowed_uids[c]) { + return EOK; + } + } + + return EACCES; +} + +errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *cvs_string, + size_t *_uid_count, uid_t **_uids) +{ + int ret; + size_t c; + char **list = NULL; + int list_size; + uid_t *uids = NULL; + char *endptr; + + ret = split_on_separator(mem_ctx, cvs_string, ',', true, &list, &list_size); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, ("split_on_separator failed [%d][%s].\n", + ret, strerror(ret))); + goto done; + } + + uids = talloc_array(mem_ctx, uint32_t, list_size); + if (uids == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n")); + ret = ENOMEM; + goto done; + } + + for (c = 0; c < list_size; c++) { + errno = 0; + if (*list[c] == '\0') { + DEBUG(SSSDBG_OP_FAILURE, ("Empty list item.\n")); + ret = EINVAL; + goto done; + } + + uids[c] = strtouint32(list[c], &endptr, 10); + if (errno != 0 || *endptr != '\0') { + ret = errno; + DEBUG(SSSDBG_OP_FAILURE, ("List item [%s] %s.\n", list[c], + ret == 0 ? "contains invalid " \ + "characters, only digits " \ + "expected" : + "is out of range")); + ret = EINVAL; + goto done; + } + } + + *_uid_count = list_size; + *_uids = uids; + + ret = EOK; + +done: + talloc_free(list); + if (ret != EOK) { + talloc_free(uids); + } + + return ret; +} + + static void client_send(struct cli_ctx *cctx) { int ret; @@ -320,6 +399,32 @@ static void accept_fd_handler(struct tevent_context *ev, "client cred may not be available.\n")); } + if (rctx->allowed_uids_count != 0) { + if (cctx->client_euid == -1) { + DEBUG(SSSDBG_CRIT_FAILURE, ("allowed_uids configured, " \ + "but platform does not support " \ + "reading peer credential from the " \ + "socket. Access denied.\n")); + close(cctx->cfd); + talloc_free(cctx); + return; + } + + ret = check_allowed_uids(cctx->client_euid, rctx->allowed_uids_count, + rctx->allowed_uids); + if (ret != EOK) { + if (ret == EACCES) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Access denied for uid [%d].\n", + cctx->client_euid)); + } else { + DEBUG(SSSDBG_OP_FAILURE, ("check_allowed_uids failed.\n")); + } + close(cctx->cfd); + talloc_free(cctx); + return; + } + } + cctx->cfde = tevent_add_fd(ev, cctx, cctx->cfd, TEVENT_FD_READ, client_fd_handler, cctx); if (!cctx->cfde) { diff --git a/src/responder/pac/pacsrv.c b/src/responder/pac/pacsrv.c index d4a1471..f6f8492 100644 --- a/src/responder/pac/pacsrv.c +++ b/src/responder/pac/pacsrv.c @@ -45,6 +45,7 @@ #define SSS_PAC_PIPE_NAME "pac" #define DEFAULT_PAC_FD_LIMIT 8192 +#define DEFAULT_ALLOWED_UIDS "0" struct sbus_method monitor_pac_methods[] = { { MON_CLI_METHOD_PING, monitor_common_pong }, @@ -124,6 +125,7 @@ int pac_process_init(TALLOC_CTX *mem_ctx, int ret, max_retries; enum idmap_error_code err; int fd_limit; + char *uid_str; pac_ctx = talloc_zero(mem_ctx, struct pac_ctx); if (!pac_ctx) { @@ -147,6 +149,23 @@ int pac_process_init(TALLOC_CTX *mem_ctx, } pac_ctx->rctx->pvt_ctx = pac_ctx; + + ret = confdb_get_string(pac_ctx->rctx->cdb, pac_ctx->rctx, + CONFDB_PAC_CONF_ENTRY, CONFDB_SERVICE_ALLOWED_UIDS, + DEFAULT_ALLOWED_UIDS, &uid_str); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get allowed UIDs.\n")); + return ret; + } + + ret = csv_string_to_uid_array(pac_ctx->rctx, uid_str, + &pac_ctx->rctx->allowed_uids_count, + &pac_ctx->rctx->allowed_uids); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to set allowed UIDs.\n")); + return ret; + } + /* Enable automatic reconnection to the Data Provider */ ret = confdb_get_int(pac_ctx->rctx->cdb, CONFDB_PAC_CONF_ENTRY, diff --git a/src/tests/responder_socket_access-tests.c b/src/tests/responder_socket_access-tests.c new file mode 100644 index 0000000..0fb18e8 --- /dev/null +++ b/src/tests/responder_socket_access-tests.c @@ -0,0 +1,177 @@ +/* + SSSD - Test for routine to check to access to responder sockets + + Authors: + Sumit Bose <sb...@redhat.com> + + Copyright (C) 2012 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <popt.h> +#include <check.h> +#include <string.h> + +#include "tests/common.h" +#include "responder/common/responder.h" + +struct cli_protocol_version *register_cli_protocol_version(void) +{ + static struct cli_protocol_version responder_test_cli_protocol_version[] = { + {0, NULL, NULL} + }; + + return responder_test_cli_protocol_version; +} + +struct s2a_data { + const char *inp; + int exp_ret; + size_t exp_count; + uid_t *exp_uids; +}; + +struct s2a_data s2a_data[] = { + {"1,2,3", 0, 3, (uid_t []){1, 2, 3}}, + {"1,2,3, 4,5 , 6 , 7 ", 0, 7, (uid_t []){1, 2, 3, 4, 5, 6, 7}}, + {"1", 0, 1, (uid_t []){1}}, + {"1, +2,3", 0, 3, (uid_t []){1, 2, 3}}, + {"1, -2,3", EINVAL, 0, NULL}, + {"1, 2ab, 3, 4", EINVAL, 0, NULL}, + {"1,", EINVAL, 0, NULL}, + {"", EINVAL, 0, NULL}, + {"1, 2, 4294967295", 0, 3, (uid_t []){1, 2, 4294967295U}}, + {"1, 2, 4294967296", EINVAL, 0, NULL}, + {NULL, EINVAL, 0, NULL}, + {NULL, -1, 0, NULL} +}; + +START_TEST(resp_str_to_array_test) +{ + int ret; + size_t uid_count; + uid_t *uids = NULL; + size_t c; + size_t d; + + for (c = 0; s2a_data[c].exp_ret != -1; c++) { + ret = csv_string_to_uid_array(global_talloc_context, s2a_data[c].inp, + &uid_count, &uids); + fail_unless(ret == s2a_data[c].exp_ret, + "csv_string_to_uid_array failed [%d][%s].", ret, + strerror(ret)); + if (ret == 0) { + fail_unless(uid_count == s2a_data[c].exp_count, + "Wrong number of values, expected [%d], got [%d].", + s2a_data[c].exp_count, uid_count); + for (d = 0; d < s2a_data[c].exp_count; d++) { + fail_unless(uids[d] == s2a_data[c].exp_uids[d], + "Wrong value, expected [%d], got [%d].\n", + s2a_data[c].exp_uids[d], uids[d]); + } + } + + talloc_free(uids); + uids = NULL; + } + +} +END_TEST + +struct uid_check_data { + uid_t uid; + size_t allowed_uids_count; + uid_t *allowed_uids; + int exp_ret; +}; + +struct uid_check_data uid_check_data[] = { + {1, 3, (uid_t []){1, 2, 3}, 0}, + {2, 3, (uid_t []){1, 2, 3}, 0}, + {3, 3, (uid_t []){1, 2, 3}, 0}, + {4, 3, (uid_t []){1, 2, 3}, EACCES}, + {4, 0, NULL, EINVAL}, + {0, 0, NULL, -1} +}; + +START_TEST(check_allowed_uids_test) +{ + int ret; + size_t c; + + for (c = 0; uid_check_data[c].exp_ret == -1; c++) { + ret = check_allowed_uids(uid_check_data[c].uid, + uid_check_data[c].allowed_uids_count, + uid_check_data[c].allowed_uids); + fail_unless(ret == uid_check_data[c].exp_ret, + "check_allowed_uids failed [%d][%s].", ret, strerror(ret)); + } +} +END_TEST + +Suite *responder_test_suite(void) +{ + Suite *s = suite_create ("Responder socket access"); + + TCase *tc_utils = tcase_create("Utility test"); + + tcase_add_test(tc_utils, resp_str_to_array_test); + tcase_add_test(tc_utils, check_allowed_uids_test); + + suite_add_tcase(s, tc_utils); + + return s; +} + +int main(int argc, const char *argv[]) +{ + int opt; + int number_failed; + poptContext pc; + + struct poptOption long_options[] = { + POPT_AUTOHELP + SSSD_MAIN_OPTS + POPT_TABLEEND + }; + + /* Set debug level to invalid value so we can deside if -d 0 was used. */ + debug_level = SSSDBG_INVALID; + + pc = poptGetContext(argv[0], argc, argv, long_options, 0); + while((opt = poptGetNextOpt(pc)) != -1) { + switch(opt) { + default: + fprintf(stderr, "\nInvalid option %s: %s\n\n", + poptBadOption(pc, 0), poptStrerror(opt)); + poptPrintUsage(pc, stderr, 0); + return 1; + } + } + poptFreeContext(pc); + + CONVERT_AND_SET_DEBUG_LEVEL(debug_level); + tests_set_cwd(); + + Suite *s = responder_test_suite(); + SRunner *sr = srunner_create(s); + + /* If CK_VERBOSITY is set, use that, otherwise it defaults to CK_NORMAL */ + srunner_run_all(sr, CK_ENV); + number_failed = srunner_ntests_failed (sr); + srunner_free (sr); + + return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +} -- 1.7.7.6
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
