[SSSD] Review of umask() in SSSD

Jakub Hrozek jhrozek at redhat.com
Wed Oct 14 11:26:23 UTC 2015


On Tue, Oct 13, 2015 at 04:40:55PM +0200, Petr Cech wrote:
> >> From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001
> >>From: Petr Cech <pcech at redhat.com>
> >>Date: Wed, 7 Oct 2015 08:57:15 -0400
> >>Subject: [PATCH 10/11] KRB5_CHILD: More restrictive umask
> >>
> >>We could use more restrictive umask in krb5_child. I found out that
> >>there is directory creation, but it is done by create_ccache_dir()
> >>which has its own umask setup.
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2424
> >>---
> >>  src/providers/krb5/krb5_child.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
> >>index 69b7687188c04498f6ef7c10a1b5ca602daca8ef..be8db23df4660adcb59fcd2677b28ee415cd18d8 100644
> >>--- a/src/providers/krb5/krb5_child.c
> >>+++ b/src/providers/krb5/krb5_child.c
> >>@@ -720,7 +720,7 @@ static krb5_error_code create_ccache(char *ccname, krb5_creds *creds)
> >>  #endif
> >>
> >>      /* Set a restrictive umask, just in case we end up creating any file */
> >>-    umask(SSS_DFL_X_UMASK);
> >>+    umask(SSS_DFL_UMASK);
> >
> >I think this change is OK, as you say, the directories might need the
> >executable flag, but then the directory-creating code should make sure
> >the permissions are more relaxed..
> >
> There is no ACK, so I will work on it later.

I think your patch was fine (and would be applied to master only anyway)
but it's also OK if you want to think about it more..

> 
> >btw I tested both FILE ccache:
> >     krb5_ccname_template = FILE:/tmp/ccache_%p.XXXXXX
> >the result looked OK to me:
> >     # ll /tmp/ccache_admin at IPA.TEST.KDaxgn
> >     -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin at IPA.TEST.KDaxgn
> >and DIR ccache:
> >     krb5_ccname_template = DIR:/tmp/ccaches/ccache_%p
> >also looked good:
> >     # ll -d /tmp/ccaches/
> >     drwx------. 3 admin admins 4096 Oct 12 09:31 /tmp/ccaches/
> >     # ll -d /tmp/ccaches/ccache_admin at IPA.TEST/
> >     drwx------. 2 admin admins 4096 Oct 12 09:31 /tmp/ccaches/ccache_admin at IPA.TEST/
> >     # ll /tmp/ccaches/ccache_admin at IPA.TEST
> >     -rw-------. 1 admin admins   10 Oct 12 09:31 primary
> >     -rw-------. 1 admin admins 1041 Oct 12 09:31 tktrg2WYD
> >
> >>
> >>      /* we create a new context here as the main process one may have been
> >>       * opened as root and contain possibly references (even open handles ?)
> >>--
> >>2.4.3
> >>
> >
> >> From 6085c5ce86e6ba79f29d2c18f6fceca9bab5cecb Mon Sep 17 00:00:00 2001
> >>From: Petr Cech <pcech at redhat.com>
> >>Date: Wed, 7 Oct 2015 09:32:12 -0400
> >>Subject: [PATCH 11/11] UTILS: Removing SSS_DFL_X UMASK constant
> >
> >077 is still used in sss_unique_file(). So we can either use SSS_DFL_X
> >umask there or convert to non-executable umask. Either way, I think it's
> >OK to keep SSS_DFL_X even though it's unused right now for later use.
> >It's just a constant.
> >
> >sss_unique_file is used to generate kdcinfo files, where non-x would be
> >OK because later we fchmod to 644 anyway:
> >      ret = fchmod(fd, S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH);
> >
> >..and also used in gpo_cache_store_file() which uses the same pattern..
> >
> >...then also in sss_unique_filename() which is used to create dummy
> >keytabs in ipa_server_trusted_dom_setup_1way(), handle_randomized() and
> >ldap_child_get_tgt_sync(). Now:
> >     - ipa_server_trusted_dom_setup_1way() - safe to change, we only use it
> >     to get a unique filename, the contents are filled with ipa-getkeytab
> >     - handle_randomized() - safe to change, libkrb5 unlinks the unique
> >       file later, so we just really need the filename
> >     - ldap_child_get_tgt_sync() - ditto, only used as input for
> >       krb5_cc_resolve()
> The same as above. I will work on it later.

OK, fine.

ACK to the attached patches.


More information about the sssd-devel mailing list