[SSSD] Review of umask() in SSSD
Jakub Hrozek
jhrozek at redhat.com
Mon Oct 12 09:37:51 UTC 2015
On Wed, Oct 07, 2015 at 03:55:17PM +0200, Petr Cech wrote:
> On 10/04/2015 09:39 PM, Jakub Hrozek wrote:
> >Finally, because I'm a lazy reviewer, I would prefer:
> > - a patch that converts 0177 to DFL, with a comment around the macro
> > definition that this is the default secure umask
> > - a patch that converts 0077 to DFL_X, with a comment around DFL_X
> > definition that unless executable bit is explicitly needed, DFL
> > should be used
> > - a patch per change if we need to tighten the existing umasks
> > further.
>
> Hi Jakub,
>
> I put more care and expanded review of umask in several patches.
>
> Patch 0005-P11-CHILD-NSS was discussed with Sumit (thanks).
>
> I'd like to ask about any special care at patch 0010-KRB5-CHILD.
> I investigated it, but second look will be better.
>
> Regards
>
> Petr
Thanks, this is much easier to review!
> From 97f8c14b58f29cf3ce341ead29f17204faa60f3d Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Mon, 5 Oct 2015 09:38:10 -0400
> Subject: [PATCH 01/11] REFACTOR: umask(0177) --> umask(SSS_DFL_UMASK)
>
> There are many calls of umask function with 0177 argument. This patch
> add new constant SSS_DFL_UMASK which stands for 0177. So all occurences
> of umask(0177) (except responder code) are replaced by constant
> SSS_DFL_UMASK.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
> ---
ACK
> From eab27ab030d0efe44ae25e2313bbee40db5cc9d4 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Mon, 5 Oct 2015 09:51:20 -0400
> Subject: [PATCH 02/11] REFACTOR: DFL_RSP_UMASK constant in responder code
>
> There is DFL_RSP_UMASK constant for very secure umask in responder
> code. This patch replaces occurances of value 0177 with this constant.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
ACK, but what do you think about changing the definition of
DFL_RSP_UMASK to:
#define DFL_RSP_UMASK SSS_DFL_UMASK
> From 3c9b9d9046082b6a4b586d7bdd02c9ec1eee0749 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Mon, 5 Oct 2015 10:12:36 -0400
> Subject: [PATCH 03/11] REFACTOR: umask(077) --> umask(SSS_DFL_X_UMASK)
>
> There are many calls of umask function with 077 argument. This patch
> add new constant SSS_DFL_X_UMASK which stands fot 077. So all
> occurences of umask(077) are replaced by constant SSS_DFL_X_UMASK.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
ACK
> From 1cfd7467ac939e2d12c18f8852402ea9c3305379 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 03:04:44 -0400
> Subject: [PATCH 04/11] REFACTOR: SCKT_RSP_UMASK constant in responder code
>
> This patch adds new SCKT_RSP_UMASK constant which stands for 0111. And
> it replaces all occurances in responder code.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
> ---
> src/responder/common/responder.h | 4 ++++
> src/responder/common/responder_common.c | 2 +-
> src/responder/pam/pamsrv.c | 2 +-
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
> index 4d927cfe321bf3ad240b7c175568081ea73ab652..ef072d5c72371a7033f5462001c22471ccbf5abf 100644
> --- a/src/responder/common/responder.h
> +++ b/src/responder/common/responder.h
> @@ -43,6 +43,10 @@ extern hash_table_t *dp_requests;
> * so set our umask to 0177 */
> #define DFL_RSP_UMASK 0177
>
> +/* Sockets must be readable and writable by anybody on the system.
I would add "Public sockets" here, because we also have a private PAM
socket that's only open for root:
# ll /var/lib/sss/pipes/private/pam
srw-------. 1 root root 0 Oct 10 22:28 /var/lib/sss/pipes/private/pam
> From 0a43a4febf56b8429d05dd448c5ee8800d1a8d21 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 07:05:57 -0400
> Subject: [PATCH 05/11] P11_CHILD_NSS: More restrictive permissions
>
> p11_child_nss runs as root and we must be carefull about security. This
> patch adds more restrictive permissions on it. There is no reason for
> 0077, so we use 0177 umask.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
ACKed also by Sumit.
> From 820c4edd0cc0ba2a43d363cbbb79aab2fcad6b37 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 07:57:17 -0400
> Subject: [PATCH 06/11] UTILS: More restrictive permissions in domain_info
>
> There are two occurances of creating temp. file under SSS_DFL_X_UMASK
> permissions which enable possibility to grant executable permission.
> After writting to those temp. files, they are renamed and they
> get 0644 permissions. So SSS_DFL_UMASK is good enough fot this case.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
ACK, I verified the permissions on domain mappings and krb5_localauth
files is still 644:
# ll /var/lib/sss/pubconf/krb5.include.d/
total 8
-rw-r--r--. 1 root root 387 Oct 12 09:06 domain_realm_ipa_test
-rw-r--r--. 1 root root 118 Oct 12 09:06 localauth_plugin
> From 498afc3d1a624e97fa6ab6998df3987bc5cff3b4 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 08:29:06 -0400
> Subject: [PATCH 07/11] UTIL-TESTS: More restrictive permissions
>
> This test suite tries to write into and to read from temp. files.
> There is no reason to have executable permission. So this patch
> replaces SSS_DFL_X_UMASK with SSS_DFL_UMASK.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
> ---
ACK, test still works
> From 8bbdffcb26315ccdb7156adefde8abc1fae6c789 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 09:51:21 -0400
> Subject: [PATCH 08/11] TESTS: More restrictive permissions in debug_tests
>
> Debug tests try to write into and read from crreated files. There is no
> reason to have executable permission, so this patch replaces
> SSS_DFl_X_UMASK with SSS_DFL_UMASK permissions.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2424
ACK, test still works
> From c9cebbb9d628e7514c965ddbde7bfd45ad51e378 Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech at redhat.com>
> Date: Tue, 6 Oct 2015 10:02:09 -0400
> Subject: [PATCH 09/11] TESTS: Restrictive permissions in check_and_open
>
> Check and open tests try to write into and read from created files.
> There is no reason to have executable permission, so this patch
> replaces SSS_DFL_X_UMASK with DFL_UMASK permissions.
>
ACK, test still works
> 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..
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()
More information about the sssd-devel
mailing list