URL: https://github.com/SSSD/sssd/pull/498 Author: fidencio Title: #498: DESKPROFILE: Do not require CAP_DAC_OVERRIDE Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/498/head:pr498 git checkout pr498
From 0a2b0848a4491394f9acabdcc18d34b209f57765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 13 Feb 2018 22:02:45 +0100 Subject: [PATCH 1/6] DESKPROFILE: Harden the permission of deskprofilepath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After discussing the permissions with Simo, we have agreed on having the deskprofile dir with the minimal set of permissions needed Related: https://pagure.io/SSSD/sssd/issue/3621 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- contrib/sssd.spec.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index f4430b424..37efcbff5 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -968,7 +968,7 @@ done %if (0%{?with_secrets} == 1) %attr(700,root,root) %dir %{secdbpath} %endif -%attr(755,sssd,sssd) %dir %{deskprofilepath} +%attr(751,sssd,sssd) %dir %{deskprofilepath} %ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/passwd %ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/group %ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/initgroups From 050cb8fc5785bee9ad392d320d0eaa994b6bb708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Sat, 20 Jan 2018 15:06:37 +0100 Subject: [PATCH 2/6] DESKPROFILE: Soften umask for the domain's dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default umask (0177) is way too strict, not allowing us to create the domain's dir, which has to have its mode set as 751. In order to solve this, let's soften the umask to 0026. This issue was exposed due to CAP_DAC_OVERRIDE being removed from Fedora package. Resolves: https://pagure.io/SSSD/sssd/issue/3621 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ipa/ipa_deskprofile_rules_util.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 01b7d0527..989f3aadd 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -229,6 +229,7 @@ ipa_deskprofile_rules_create_user_dir( char *domain; char *domain_dir; errno_t ret; + mode_t old_umask; tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { @@ -243,8 +244,10 @@ ipa_deskprofile_rules_create_user_dir( goto done; } - ret = sss_create_dir(IPA_DESKPROFILE_RULES_USER_DIR, domain, 0755, + old_umask = umask(0026); + ret = sss_create_dir(IPA_DESKPROFILE_RULES_USER_DIR, domain, 0751, getuid(), getgid()); + umask(old_umask); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to create the directory \"%s/%s\" that would be used to " From 2f40a0b24cf25e9e76f3fecee8358d1c0ccfc9c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Sat, 20 Jan 2018 23:58:14 +0100 Subject: [PATCH 3/6] DESKPROFILE: Fix the permissions and soften the umask for user's dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user dir has been created as 0600 and owned by the user. It doesn't work anymore as CAP_DAC_OVERRIDE has been dropped from our systemd service upstream. In order to have it working again, let's change it to 0700 (as the executable bit is needed for creating a file inside a folder) and soften the default umask from (0177) to (0077) to be able to create this dir. This issue was exposed due to CAP_DAC_OVERRIDE being removed from Fedora package. Resolves: https://pagure.io/SSSD/sssd/issue/3621 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ipa/ipa_deskprofile_rules_util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 989f3aadd..0846b16f6 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -264,7 +264,11 @@ ipa_deskprofile_rules_create_user_dir( goto done; } - ret = sss_create_dir(domain_dir, shortname, 0600, uid, gid); + /* In order to read, create and traverse the directory, we need to have its + * permissions set as 'rwx------' (700). */ + old_umask = umask(0077); + ret = sss_create_dir(domain_dir, shortname, 0700, uid, gid); + umask(old_umask); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to create the directory \"%s/%s/%s\" that would be used " From b38748019e3606483459b602e25c171d9fafff46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Mon, 22 Jan 2018 11:49:23 +0100 Subject: [PATCH 4/6] DESKPROFILE: Use seteuid()/setegid() to create the profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to create the file, having its owner properly, let's use seteuid()/setegid() to create when creating the profile, as due to the drop of the CAP_DAC_OVERRIDE "root" doesn't have access to the folder where the profile will be created anymore. By adopting the seteuid()/setegid() solution, calling fchown() in the profile doesn't make sense, thus it was also removed. This issue was exposed due to CAP_DAC_OVERRIDE being removed from Fedora package. Resolves: https://pagure.io/SSSD/sssd/issue/3621 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ipa/ipa_deskprofile_rules_util.c | 70 ++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 4 deletions(-) diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 0846b16f6..eb04a69f8 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -706,6 +706,8 @@ ipa_deskprofile_rules_save_rule_to_disk( const char *extension = "json"; uint32_t prio; int fd = -1; + gid_t orig_gid; + uid_t orig_uid; errno_t ret; tmp_ctx = talloc_new(mem_ctx); @@ -713,6 +715,9 @@ ipa_deskprofile_rules_save_rule_to_disk( return ENOMEM; } + orig_gid = getegid(); + orig_uid = geteuid(); + ret = sysdb_attrs_get_string(rule, IPA_CN, &rule_name); if (ret != EOK) { DEBUG(SSSDBG_TRACE_FUNC, @@ -875,6 +880,26 @@ ipa_deskprofile_rules_save_rule_to_disk( goto done; } + ret = setegid(gid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective group id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + gid, ret, sss_strerror(ret)); + goto done; + } + + ret = seteuid(uid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective user id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + uid, ret, sss_strerror(ret)); + goto done; + } + fd = open(filename_path, O_WRONLY | O_CREAT | O_TRUNC, 0600); if (fd == -1) { ret = errno; @@ -895,12 +920,23 @@ ipa_deskprofile_rules_save_rule_to_disk( goto done; } - ret = fchown(fd, uid, gid); - if (ret != EOK) { + ret = seteuid(orig_uid); + if (ret == -1) { ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, - "Failed to own the Desktop Profile Rule file \"%s\" [%d]: %s\n", - filename_path, ret, sss_strerror(ret)); + "Failed to set the effect user id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + orig_uid, ret, sss_strerror(ret)); + goto done; + } + + ret = setegid(orig_gid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to set the effect group id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + orig_gid, ret, sss_strerror(ret)); goto done; } @@ -910,6 +946,32 @@ ipa_deskprofile_rules_save_rule_to_disk( if (fd != -1) { close(fd); } + if (geteuid() != orig_uid) { + ret = seteuid(orig_uid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective user id (%"PRIu32") of the " + "domain's process [%d]: %s\n", + orig_uid, ret, sss_strerror(ret)); + DEBUG(SSSDBG_CRIT_FAILURE, + "Sending SIGUSR2 to the process: %d\n", getpid()); + kill(getpid(), SIGUSR2); + } + } + if (getegid() != orig_gid) { + ret = setegid(orig_gid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective group id (%"PRIu32") of the " + "domain's process. Let's have the process restartd!\n", + orig_gid); + DEBUG(SSSDBG_CRIT_FAILURE, + "Sending SIGUSR2 to the process: %d\n", getpid()); + kill(getpid(), SIGUSR2); + } + } talloc_free(tmp_ctx); return ret; } From 17d8ffee44ec1aad0af4036137640ba73322041c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Wed, 31 Jan 2018 16:50:38 +0100 Subject: [PATCH 5/6] DESKPROFILE: Use seteuid()/setegid() to delete the profile/user's dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's use seteuid()/setegid() in order to properly delete the desktop profiles related files. Some malabarism has been introduced in order to proper delete those dirs/files as: /var/lib/sss/deskprofile/ipa.example/admin/profile ------------------------ ----------- ----- ------- | | | | v | | | Created by sssd package, | | | not touching at all | | | v | | This one is owned by | | root:root and has 751 | | as permissions | | v | This one is owned by | admin:admins and has | 0700 as permissions | v This one is owned by admin:admins and has 0600 as permissions So, when deleting we do: - as admin: - sss_remove_subtree("/var/lib/sss/deskprofile/ipa.example/admin/"); We can't remove the "admin" dir itself as it would require different permissions in the domain's folder and that's something we don't want to change - as root: - sss_remove_tree("/var/lib/sss/deskprofile/ipa.example/admin/"); Now we just removed the "admin" dir. The main reason behind not being able to just delete it as root is because the permissions of the file and dirs do not allow root to access then when not relying in the CAP_DAC_OVERRIDE This issue was exposed due to the CAP_DAC_OVERRIDE being removed from Fedora package. Resolves: https://pagure.io/SSSD/sssd/issue/3621 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ipa/ipa_deskprofile_rules_util.c | 91 ++++++++++++++++++++++++-- src/providers/ipa/ipa_deskprofile_rules_util.h | 4 +- src/providers/ipa/ipa_session.c | 4 +- 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index eb04a69f8..2102713d6 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -977,21 +977,104 @@ ipa_deskprofile_rules_save_rule_to_disk( } errno_t -ipa_deskprofile_rules_remove_user_dir(const char *user_dir) +ipa_deskprofile_rules_remove_user_dir(const char *user_dir, + uid_t uid, + gid_t gid) { + gid_t orig_gid; + uid_t orig_uid; errno_t ret; + orig_gid = getegid(); + orig_uid = geteuid(); + + ret = setegid(gid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective group id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + gid, ret, sss_strerror(ret)); + goto done; + } + + ret = seteuid(uid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective user id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + uid, ret, sss_strerror(ret)); + goto done; + } + + ret = sss_remove_subtree(user_dir); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Cannot remove \"%s\" directory [%d]: %s\n", + user_dir, ret, sss_strerror(ret)); + goto done; + } + + ret = seteuid(orig_uid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to set the effect user id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + orig_uid, ret, sss_strerror(ret)); + goto done; + } + + ret = setegid(orig_gid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to set the effect group id (%"PRIu32") of the domain's " + "process [%d]: %s\n", + orig_gid, ret, sss_strerror(ret)); + goto done; + } + ret = sss_remove_tree(user_dir); if (ret == ENOENT) { - return EOK; + ret = EOK; } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Cannot remove \"%s\" directory [%d]: %s\n", user_dir, ret, sss_strerror(ret)); - return ret; + goto done; } - return EOK; + ret = EOK; + +done: + if (geteuid() != orig_uid) { + ret = seteuid(orig_uid); + if (ret == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + "unable to set effective user id (%"PRIu32") of the " + "domain's process [%d]: %s\n", + orig_uid, ret, sss_strerror(ret)); + DEBUG(SSSDBG_CRIT_FAILURE, + "Sending SIGUSR2 to the process: %d\n", getpid()); + kill(getpid(), SIGUSR2); + } + } + if (getegid() != orig_gid) { + ret = setegid(orig_gid); + if (ret == -1) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to set effective user id (%"PRIu32") of the " + "domain's process [%d]: %s\n", + orig_uid, ret, sss_strerror(ret)); + DEBUG(SSSDBG_CRIT_FAILURE, + "Sending SIGUSR2 to the process: %d\n", getpid()); + kill(getpid(), SIGUSR2); + } + } + return ret; } errno_t diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.h b/src/providers/ipa/ipa_deskprofile_rules_util.h index 61f404df8..4016dbccf 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.h +++ b/src/providers/ipa/ipa_deskprofile_rules_util.h @@ -45,7 +45,9 @@ ipa_deskprofile_rules_save_rule_to_disk( uid_t uid, gid_t gid); errno_t -ipa_deskprofile_rules_remove_user_dir(const char *user_dir); +ipa_deskprofile_rules_remove_user_dir(const char *user_dir, + uid_t uid, + gid_t gid); errno_t deskprofile_get_cached_priority(struct sss_domain_info *domain, diff --git a/src/providers/ipa/ipa_session.c b/src/providers/ipa/ipa_session.c index 3c7dd33c3..25ad5ce51 100644 --- a/src/providers/ipa/ipa_session.c +++ b/src/providers/ipa/ipa_session.c @@ -524,7 +524,9 @@ ipa_pam_session_handler_send(TALLOC_CTX *mem_ctx, /* As no proper merging mechanism has been implemented yet ... * let's just remove the user directory stored in the disk as it's * going to be created again in case there's any rule fetched. */ - ret = ipa_deskprofile_rules_remove_user_dir(state->user_dir); + ret = ipa_deskprofile_rules_remove_user_dir(state->user_dir, + state->uid, + state->gid); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "ipa_deskprofile_rules_remove_user_dir() failed.\n"); From 3339299395d0735a0a83a116a901ae0f76ee064a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Mon, 5 Feb 2018 07:56:53 +0100 Subject: [PATCH 6/6] DESKPROFILE: Set the profile permissions to read-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sumit suggested to have the profile permissions with the least possible permissions and it does make sense. So, let's change it from read-write to read-only. Related: https://pagure.io/SSSD/sssd/issue/362 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/ipa/ipa_deskprofile_rules_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 2102713d6..e52587378 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -900,7 +900,7 @@ ipa_deskprofile_rules_save_rule_to_disk( goto done; } - fd = open(filename_path, O_WRONLY | O_CREAT | O_TRUNC, 0600); + fd = open(filename_path, O_WRONLY | O_CREAT | O_TRUNC, 0400); if (fd == -1) { ret = errno; DEBUG(SSSDBG_CRIT_FAILURE,
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
