[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