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 2fa70e54298c8de35566d81a5da62f6f9d69e517 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 1/5] DESKPROFILE: Soften the 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 755.

In order to solve this, let's soften the umask to 0022.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c
index 53c433145..f9a867daf 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;
     }
 
+    old_umask = umask(0022);
     ret = sss_create_dir(IPA_DESKPROFILE_RULES_USER_DIR, domain, 0755,
                          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 00616f8ec6f3ff763cefe8ba68a277b8d5a88752 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 2/5] 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c
index f9a867daf..081171299 100644
--- a/src/providers/ipa/ipa_deskprofile_rules_util.c
+++ b/src/providers/ipa/ipa_deskprofile_rules_util.c
@@ -264,7 +264,9 @@ ipa_deskprofile_rules_create_user_dir(
         goto done;
     }
 
-    ret = sss_create_dir(domain_dir, shortname, 0600, uid, gid);
+    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 c1cb56dcec2ad1b2231dabd7eafcaf5f31f2efb7 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 3/5] 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 081171299..65ed8c01a 100644
--- a/src/providers/ipa/ipa_deskprofile_rules_util.c
+++ b/src/providers/ipa/ipa_deskprofile_rules_util.c
@@ -702,6 +702,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);
@@ -709,6 +711,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,
@@ -809,6 +814,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;
@@ -829,12 +854,23 @@ ipa_deskprofile_rules_save_rule_to_disk(
         goto done;
     }
 
-    ret = fchown(fd, uid, gid);
-    if (ret != EOK) {
+    ret = setegid(orig_gid);
+    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 group id (%"PRIu32") of the domain's "
+              "process [%d]: %s\n",
+              orig_gid, 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;
     }
 
@@ -844,6 +880,32 @@ ipa_deskprofile_rules_save_rule_to_disk(
     if (fd != -1) {
         close(fd);
     }
+    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);
+        }
+    }
+    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);
+        }
+    }
     talloc_free(tmp_ctx);
     return ret;
 }

From 3b0c55b4ac7834bbb0eb4d49d2ce78060c8aff50 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 4/5] 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 755 |       |
                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 65ed8c01a..de8d56cc3 100644
--- a/src/providers/ipa/ipa_deskprofile_rules_util.c
+++ b/src/providers/ipa/ipa_deskprofile_rules_util.c
@@ -911,21 +911,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 = 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 = 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 = 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 (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);
+        }
+    }
+    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);
+        }
+    }
+    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 b21b7e78330b6210b2e93f93779cecf4d8f890de 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 5/5] 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 de8d56cc3..7d88eed44 100644
--- a/src/providers/ipa/ipa_deskprofile_rules_util.c
+++ b/src/providers/ipa/ipa_deskprofile_rules_util.c
@@ -834,7 +834,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

Reply via email to