[SSSD] [PATCH] SELINUX: Set permissive umask for selinux operations

Jakub Hrozek jhrozek at redhat.com
Tue Jan 27 16:38:57 UTC 2015


On Tue, Jan 27, 2015 at 05:25:36PM +0100, Jakub Hrozek wrote:
> On Tue, Jan 27, 2015 at 11:07:01AM -0500, Roland Mainz wrote:
> > 
> > 
> > ----- Original Message -----
> > > From: "Roland Mainz" <rmainz at redhat.com>
> > > To: "Development of the System Security Services Daemon" <sssd-devel at lists.fedorahosted.org>
> > > Sent: Tuesday, January 27, 2015 4:41:28 PM
> > > Subject: Re: [SSSD] [PATCH] SELINUX: Set permissive umask for selinux operations
> > > 
> > > 
> > > 
> > > ----- Original Message -----
> > > > From: "Jakub Hrozek" <jhrozek at redhat.com>
> > > > To: sssd-devel at lists.fedorahosted.org
> > > > Sent: Tuesday, January 27, 2015 11:31:58 AM
> > > > Subject: Re: [SSSD] [PATCH] SELINUX: Set permissive umask for selinux
> > > > operations
> > > > 
> > > > On Tue, Jan 27, 2015 at 10:51:24AM +0100, Jakub Hrozek wrote:
> > > > > Alternatively, we could only set and reset the umask in the caller. That
> > > > > way, we would know that a short-lived selinux_child is changing umask.
> > > > > 
> > > > > That is safer, but risks that we forget next time..so at least we need
> > > > > to add a comment to the declaration in header.
> > > > 
> > > > Attached is an alternate patch that only touches the umask in the
> > > > short-lived process.
> > > 
> > > Which syscall exactly (strace dump, please) causes the original issue ? Maybe
> > > there is a way to do this without having to rely on |umask()| (using the
> > > |*at()| APIs or a later modification of the permissions) ...
> > 
> > I discussed this with jhrozek on IRC... basically libsemanage should fix the permissions after the |mkdir()| call like the mkdir(1) code in https://searchcode.com/codesearch/view/5481405/ line 160 ... but until they we have to stick with the |umask()| workaround (and praying that sssd never uses threads).
> 
> I filed:
> https://bugzilla.redhat.com/show_bug.cgi?id=1186422
> and:
> https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> against libsemanage. As discussed with Roland on IRC, I will add a URL
> of the bugzilla to a comment in selinux_child to make it clear that it's
> just a workaround for a bug, not a clean solution.

FYI, I'll push the attached patch. The only difference from the version
Michal acked is the additional comment.
-------------- next part --------------
>From da7a6ca6bbdb32f93a0afc5f71d638a2e445eb0c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 27 Jan 2015 11:12:18 +0100
Subject: [PATCH] SELINUX: Set and reset umask when caling set_seuser from
 deamon code

---
 src/providers/ipa/selinux_child.c | 18 +++++++++++++++++-
 src/util/util.h                   |  4 ++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c
index 6390d43cbe4f3d352702493c156f93ca20b07a2f..7297f5ed3bafbd8ac9b14ee60ef07839a35587cf 100644
--- a/src/providers/ipa/selinux_child.c
+++ b/src/providers/ipa/selinux_child.c
@@ -135,6 +135,22 @@ static errno_t prepare_response(TALLOC_CTX *mem_ctx,
     return EOK;
 }
 
+static int sc_set_seuser(const char *login_name, const char *seuser_name,
+                         const char *mls)
+{
+    int ret;
+    mode_t old_mask;
+
+    /* This is a workaround for
+     * https://bugzilla.redhat.com/show_bug.cgi?id=1186422 to make sure
+     * the directories are created with the expected permissions
+     */
+    old_mask = umask(0);
+    ret = set_seuser(login_name, seuser_name, mls);
+    umask(old_mask);
+    return ret;
+}
+
 int main(int argc, const char *argv[])
 {
     int opt;
@@ -256,7 +272,7 @@ int main(int argc, const char *argv[])
 
     DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
 
-    ret = set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
+    ret = sc_set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Cannot set SELinux login context.\n");
         goto fail;
diff --git a/src/util/util.h b/src/util/util.h
index 4ee9bad113299d8fb3eabebec508c2ab4c80d4e9..22d6ef0a4e1340346d3d2997313aab50410f9dc0 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -640,6 +640,10 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
 errno_t restore_creds(struct sss_creds *saved_creds);
 
 /* from sss_semanage.c */
+/* Please note that libsemange relies on files and directories created with
+ * certain permissions. Therefore the caller should make sure the umask is
+ * not too restricted (especially when called from the daemon code).
+ */
 int set_seuser(const char *login_name, const char *seuser_name,
                const char *mlsrange);
 int del_seuser(const char *login_name);
-- 
2.1.0



More information about the sssd-devel mailing list