[SSSD] Review of umask() in SSSD

Petr Cech pcech at redhat.com
Tue Oct 13 14:40:55 UTC 2015


On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
> 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
Done.
>
>>  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
>
Done.
>>  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..
>
There is no ACK, so I will work on it later.

> 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.

Thanks for carefull review.
Petr
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-REFACTOR-umask-0177-umask-SSS_DFL_UMASK.patch
Type: text/x-patch
Size: 2611 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-REFACTOR-DFL_RSP_UMASK-constant-in-responder-code.patch
Type: text/x-patch
Size: 2579 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-REFACTOR-umask-077-umask-SSS_DFL_X_UMASK.patch
Type: text/x-patch
Size: 5038 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-REFACTOR-SCKT_RSP_UMASK-constant-in-responder-code.patch
Type: text/x-patch
Size: 2678 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-P11_CHILD_NSS-More-restrictive-permissions.patch
Type: text/x-patch
Size: 1288 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-UTILS-More-restrictive-permissions-in-domain_info.patch
Type: text/x-patch
Size: 1404 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-UTIL-TESTS-More-restrictive-permissions.patch
Type: text/x-patch
Size: 1079 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-TESTS-More-restrictive-permissions-in-debug_tests.patch
Type: text/x-patch
Size: 1332 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-TESTS-Restrictive-permissions-in-check_and_open.patch
Type: text/x-patch
Size: 1175 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/c884d204/attachment-0017.bin>


More information about the sssd-devel mailing list