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

Jakub Hrozek jhrozek at redhat.com
Tue Jan 27 09:45:20 UTC 2015


On Tue, Jan 27, 2015 at 10:20:17AM +0100, Sumit Bose wrote:
> On Mon, Jan 26, 2015 at 06:18:26PM +0100, Jakub Hrozek wrote:
> > Hi,
> > 
> > to reproduce, set a SELinux label for a user. Log in as the user and
> > observer the permissions of /etc/selinux/targeted/modules/active
> > changing to 0600. As a result, even local semanage tools didn't work..
> 
> > From dbe42c9ca20df500d09949512b86c86f7b4ce7ce Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek at redhat.com>
> > Date: Mon, 26 Jan 2015 16:55:42 +0100
> > Subject: [PATCH] SELINUX: Set permissive umask for selinux operations
> > 
> > https://fedorahosted.org/sssd/ticket/2563
> > 
> > libsemanage calls mkdir() and then requires that the directory is created
> > with permissions 0700. That doesn't work well for programs like sssd that
> > set umask to a very restrictive value (like 177).
> > ---
> >  src/util/sss_semanage.c | 29 ++++++++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
> > index 3c566553f2085a696f79c5ee35ec6015824d56a6..4de0ce2bb4d52852cc7fa4df8a3e92b667c27c81 100644
> > --- a/src/util/sss_semanage.c
> > +++ b/src/util/sss_semanage.c
> > @@ -216,8 +216,8 @@ done:
> >      return ret;
> >  }
> >  
> > -int set_seuser(const char *login_name, const char *seuser_name,
> > -               const char *mls)
> > +int _set_seuser(const char *login_name, const char *seuser_name,
> > +                const char *mls)
> 
> would it be possilbe to make _set_seuser static as _del_seuser?

Sure, will do.

> 
> >  {
> >      semanage_handle_t *handle = NULL;
> >      semanage_seuser_key_t *key = NULL;
> > @@ -282,7 +282,19 @@ done:
> >      return ret;
> >  }
> >  
> > -int del_seuser(const char *login_name)
> > +int set_seuser(const char *login_name, const char *seuser_name,
> > +               const char *mls)
> > +{
> > +    mode_t old_mask;
> > +    int ret;
> > +
> > +    old_mask = umask(0);
> > +    ret = _set_seuser(login_name, seuser_name, mls);
> > +    umask(old_mask);
> 
> just a comment, please note that umask() will change the umask of the
> whole process. With asynchronous processing we should always make sure
> to change the umask only where it is needed.

Yes, that's why the synchronous set function is enclosed in setting and
then resetting umask. Do you think it's better to only enclose specific
libsemanage functions according to which of them might create the
directory? I was considering this, but on the other hand, I think we
should treat libsemanage as a black box and don't depend too much on the
implementation.



More information about the sssd-devel mailing list