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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)IPA.TEST.KDaxgn
-rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin(a)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(a)IPA.TEST/
drwx------. 2 admin admins 4096 Oct 12 09:31 /tmp/ccaches/ccache_admin(a)IPA.TEST/
# ll /tmp/ccaches/ccache_admin(a)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(a)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()