Hi,
I am reviewing umask() in our code according to https://fedorahosted.org/sssd/ticket/2424
There are many use like umask(DFL_RSP_UMASK): src/responder/autofs/autofssrv.c:223 src/responder/ifp/ifpsrv.c:401 src/responder/nss/nsssrv.c:589 src/responder/pac/pacsrv.c:232 src/responder/pam/pamsrv.c:369 src/responder/ssh/sshsrv.c:209 src/responder/sudo/sudosrv.c:215 where DFL_RSP_UMASK is defined as 0177.
There are another three use of umask 0177: src/confdb/confdb.c:662 src/util/debug.c:365 src/util/server.c:495
And then I see many use of umask 077: src/p11_child/p11_child_nss.c:485 src/providers/krb5/krb5_child.c:723 src/tests/check_and_open-tests.c:51 src/tests/debug-tests.c:136 src/tests/debug-tests.c:276 src/tests/util-tests.c:596 src/util/domain_info_utils.c:312 src/util/domain_info_utils.c:562 src/tools/tools_util.c:503
I would like to ask you if we would like to use 0077 or 0177 as our very restrictive mask. I see that our code is not consistent on this question. I know the difference is small, but it is.
Then we have some unsecure use: src/providers/ipa/selinux_child.c:154: umask = 0 src/providers/krb5/krb5_ccache.c:188: umask = 0000 src/responder/nss/nsssrv_mmap_cache.c:1121: umask = 0022 but I think there is reason for it.
And the last one is at src/responder/common/responder_common.c:561: int create_pipe_fd(const char *sock_name, int *_fd, mode_t umaskval) We use it secure (0177) at: src/responder/common/responder_common.c:693 src/responder/pam/pamsrv.c:399
And not so secure: src/responder/common/responder_common.c:670 umask = 0111 src/responder/pam/pamsrv.c:391 umask = 0111 src/tests/cwrap/test_responder_common.c:173 umask = 0111 src/tests/cwrap/test_responder_common.c:179 umask = 0000
So, what could I do? Maybe we could have only one very secure umask and maybe we could have CONSTANT for every use of umask. Any another ideas?
Regards
Petr
On Thu, Sep 10, 2015 at 12:27:17PM +0200, Petr Cech wrote:
Hi,
I am reviewing umask() in our code according to https://fedorahosted.org/sssd/ticket/2424
There are many use like umask(DFL_RSP_UMASK): src/responder/autofs/autofssrv.c:223 src/responder/ifp/ifpsrv.c:401 src/responder/nss/nsssrv.c:589 src/responder/pac/pacsrv.c:232 src/responder/pam/pamsrv.c:369 src/responder/ssh/sshsrv.c:209 src/responder/sudo/sudosrv.c:215 where DFL_RSP_UMASK is defined as 0177.
There are another three use of umask 0177: src/confdb/confdb.c:662 src/util/debug.c:365 src/util/server.c:495
And then I see many use of umask 077: src/p11_child/p11_child_nss.c:485 src/providers/krb5/krb5_child.c:723 src/tests/check_and_open-tests.c:51 src/tests/debug-tests.c:136 src/tests/debug-tests.c:276 src/tests/util-tests.c:596 src/util/domain_info_utils.c:312 src/util/domain_info_utils.c:562 src/tools/tools_util.c:503
I would like to ask you if we would like to use 0077 or 0177 as our very restrictive mask. I see that our code is not consistent on this question. I know the difference is small, but it is.
I guess 0177 should be used.
Then we have some unsecure use: src/providers/ipa/selinux_child.c:154: umask = 0 src/providers/krb5/krb5_ccache.c:188: umask = 0000 src/responder/nss/nsssrv_mmap_cache.c:1121: umask = 0022 but I think there is reason for it.
Yes, it would be nice if there was always a comment explaining the umask.
And the last one is at src/responder/common/responder_common.c:561: int create_pipe_fd(const char *sock_name, int *_fd, mode_t umaskval) We use it secure (0177) at: src/responder/common/responder_common.c:693 src/responder/pam/pamsrv.c:399
If this is in responder, would it make sense to just use DFL_RSP_UMASK ?
And not so secure: src/responder/common/responder_common.c:670 umask = 0111
This one has a comment explaining why the umask it is the way it is, but would it make sense to add a note about public/private sockets as well (maybe not to the code but to the InternalsDocs) and #define a constant for the public pipes?
src/responder/pam/pamsrv.c:391 umask = 0111 src/tests/cwrap/test_responder_common.c:173 umask = 0111 src/tests/cwrap/test_responder_common.c:179 umask = 0000
So, what could I do? Maybe we could have only one very secure umask and maybe we could have CONSTANT for every use of umask. Any another ideas?
I like this idea, the constant could describe why we need this particular umask better than the number also.
Regards
Petr _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 09/11/2015 01:47 PM, Jakub Hrozek wrote:
On Thu, Sep 10, 2015 at 12:27:17PM +0200, Petr Cech wrote:
Hi,
I am reviewing umask() in our code according to https://fedorahosted.org/sssd/ticket/2424
There are many use like umask(DFL_RSP_UMASK): src/responder/autofs/autofssrv.c:223 src/responder/ifp/ifpsrv.c:401 src/responder/nss/nsssrv.c:589 src/responder/pac/pacsrv.c:232 src/responder/pam/pamsrv.c:369 src/responder/ssh/sshsrv.c:209 src/responder/sudo/sudosrv.c:215 where DFL_RSP_UMASK is defined as 0177.
There are another three use of umask 0177: src/confdb/confdb.c:662 src/util/debug.c:365 src/util/server.c:495
And then I see many use of umask 077: src/p11_child/p11_child_nss.c:485 src/providers/krb5/krb5_child.c:723 src/tests/check_and_open-tests.c:51 src/tests/debug-tests.c:136 src/tests/debug-tests.c:276 src/tests/util-tests.c:596 src/util/domain_info_utils.c:312 src/util/domain_info_utils.c:562 src/tools/tools_util.c:503
I would like to ask you if we would like to use 0077 or 0177 as our very restrictive mask. I see that our code is not consistent on this question. I know the difference is small, but it is.
I guess 0177 should be used.
Then we have some unsecure use: src/providers/ipa/selinux_child.c:154: umask = 0 src/providers/krb5/krb5_ccache.c:188: umask = 0000 src/responder/nss/nsssrv_mmap_cache.c:1121: umask = 0022 but I think there is reason for it.
Yes, it would be nice if there was always a comment explaining the umask.
And the last one is at src/responder/common/responder_common.c:561: int create_pipe_fd(const char *sock_name, int *_fd, mode_t umaskval) We use it secure (0177) at: src/responder/common/responder_common.c:693 src/responder/pam/pamsrv.c:399
If this is in responder, would it make sense to just use DFL_RSP_UMASK ?
And not so secure: src/responder/common/responder_common.c:670 umask = 0111
This one has a comment explaining why the umask it is the way it is, but would it make sense to add a note about public/private sockets as well (maybe not to the code but to the InternalsDocs) and #define a constant for the public pipes?
src/responder/pam/pamsrv.c:391 umask = 0111 src/tests/cwrap/test_responder_common.c:173 umask = 0111 src/tests/cwrap/test_responder_common.c:179 umask = 0000
So, what could I do? Maybe we could have only one very secure umask and maybe we could have CONSTANT for every use of umask. Any another ideas?
I like this idea, the constant could describe why we need this particular umask better than the number also.
Regards
Petr _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks, Jakub, for comments. There is a patch attached. Petr
On 10/01/2015 01:19 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 12:38:49PM +0200, Petr Cech wrote:
Bump.
Thanks for reply, Jakub.
Why was 077 changed for 0177?
This change is something, which I think was discussed earlier in this thread.
# pcech: # > I would like to ask you if we would like to use 0077 or 0177 as our very # > restrictive mask. I see that our code is not consistent on this question. I # > know the difference is small, but it is. # # jhrozek: # I guess 0177 should be used.
I think that we work only with files, not with directories, I should check it again.
So, if it is risky, I will changed it. :-)
About the name -- shouldn't we say just "SSS_DFL_UMASK" ? We are a security project, therefore restrictive by default :-)
You're right, we are security project by default, so I changed the constant name.
On Thu, Oct 01, 2015 at 02:06:50PM +0200, Petr Cech wrote:
On 10/01/2015 01:19 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 12:38:49PM +0200, Petr Cech wrote:
Bump.
Thanks for reply, Jakub.
Why was 077 changed for 0177?
This change is something, which I think was discussed earlier in this thread.
# pcech: # > I would like to ask you if we would like to use 0077 or 0177 as our very # > restrictive mask. I see that our code is not consistent on this question. I # > know the difference is small, but it is. # # jhrozek: # I guess 0177 should be used.
Ah, then we misunderstood each other. What I meant was umask what we should use going forward, when adding new umask calls to SSSD. The reason I was proposing using 177 over 077 is simply that it's even more restrictive - not even the owner can set the executable permissions.
I'm not opposed to restricting permissions further, but i) there should be a patch per change, otherwise this kind of changes gets tricky to review, fast and ii) definitely not part as this patch.
On Sun, Oct 04, 2015 at 09:15:05PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 02:06:50PM +0200, Petr Cech wrote:
On 10/01/2015 01:19 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 12:38:49PM +0200, Petr Cech wrote:
Bump.
Thanks for reply, Jakub.
Why was 077 changed for 0177?
This change is something, which I think was discussed earlier in this thread.
# pcech: # > I would like to ask you if we would like to use 0077 or 0177 as our very # > restrictive mask. I see that our code is not consistent on this question. I # > know the difference is small, but it is. # # jhrozek: # I guess 0177 should be used.
Ah, then we misunderstood each other. What I meant was umask what we should use going forward, when adding new umask calls to SSSD. The reason I was proposing using 177 over 077 is simply that it's even more restrictive - not even the owner can set the executable permissions.
I'm not opposed to restricting permissions further, but i) there should be a patch per change, otherwise this kind of changes gets tricky to review, fast and ii) definitely not part as this patch.
OK, this might have been still confusing :-)
With this patch, I think we can have one more #define along with the defult that also allows u=x, maybe called DFL_X_UMASK or similar. Then, if you think the umasks should be tightened, please send a patch per change with an explanation of the the change is desired in the commit message.
On Sun, Oct 04, 2015 at 09:20:17PM +0200, Jakub Hrozek wrote:
On Sun, Oct 04, 2015 at 09:15:05PM +0200, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 02:06:50PM +0200, Petr Cech wrote:
On 10/01/2015 01:19 PM, Jakub Hrozek wrote:
On Thu, Oct 01, 2015 at 12:38:49PM +0200, Petr Cech wrote:
Bump.
Thanks for reply, Jakub.
Why was 077 changed for 0177?
This change is something, which I think was discussed earlier in this thread.
# pcech: # > I would like to ask you if we would like to use 0077 or 0177 as our very # > restrictive mask. I see that our code is not consistent on this question. I # > know the difference is small, but it is. # # jhrozek: # I guess 0177 should be used.
Ah, then we misunderstood each other. What I meant was umask what we should use going forward, when adding new umask calls to SSSD. The reason I was proposing using 177 over 077 is simply that it's even more restrictive - not even the owner can set the executable permissions.
I'm not opposed to restricting permissions further, but i) there should be a patch per change, otherwise this kind of changes gets tricky to review, fast and ii) definitely not part as this patch.
OK, this might have been still confusing :-)
With this patch, I think we can have one more #define along with the defult that also allows u=x, maybe called DFL_X_UMASK or similar.
Sorry for spamming.
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.
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
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@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@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.
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@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.
ACK
From 1cfd7467ac939e2d12c18f8852402ea9c3305379 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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@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.
ACKed also by Sumit.
From 820c4edd0cc0ba2a43d363cbbb79aab2fcad6b37 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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.
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@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@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.
ACK, test still works
From c9cebbb9d628e7514c965ddbde7bfd45ad51e378 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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@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@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31 /tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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@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()
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@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@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.
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@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.
ACK
From 1cfd7467ac939e2d12c18f8852402ea9c3305379 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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@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.
ACKed also by Sumit.
From 820c4edd0cc0ba2a43d363cbbb79aab2fcad6b37 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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.
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@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@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.
ACK, test still works
From c9cebbb9d628e7514c965ddbde7bfd45ad51e378 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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@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@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31 /tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31 /tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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@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.
On Wed, Oct 14, 2015 at 01:26:23PM +0200, Jakub Hrozek wrote:
ACK to the attached patches.
CI: http://sssd-ci.duckdns.org/logs/job/30/28/summary.html
master: * c1584502dec8ae19dfd89c6e598cc7648dfd78a6 * c4a1191d673e4368f1831cbeb4d75b15e51ff6db * 4230e6faa907b693ccf66b3a29f9be57b87aab14 * a0ad4f2a4a342214acd4ab4b77cc5ccce22b35ad * ae627e216689b0a5834f36aaaa007ed584ef033d * 2f6a94e30458df92fb26c3d810f613d1e4cff99b * f8e337540d280f944098cd4dd7d670e2f7166b54 * d9c2a21119a6d04203060ad54fa8d20f17f5c0b7 * c299f997e20011536e365bc18e59e73f68629d2c
On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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..
I checked it again. It is OK.
btw I tested both FILE ccache: krb5_ccname_template =FILE:/tmp/ccache_%p.XXXXXX the result looked OK to me: # ll /tmp/ccache_admin@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31/tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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 Cechpcech@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.
OK, SSS_DFL_X_UMASK is still here, but not used in code.
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..
I rewrote DFL_X to DFL in sss_unique_file().
...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 third patch is about redudant constant.
And at the end, there are may uses of umask() in CI tests, which I leave how they are. They could be test relevant. Maybe I will touch it in some future patch.
The last umask like constant is 644, which is connected to chmod(), open(), etc. Do we want to have a constant for it?
Regards
Petr
On 10/21/2015 03:19 PM, Petr Cech wrote:
On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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 creatingany 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..
I checked it again. It is OK.
btw I tested both FILE ccache: krb5_ccname_template =FILE:/tmp/ccache_%p.XXXXXX the result looked OK to me: # ll /tmp/ccache_admin@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31/tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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 mayhave been
* opened as root and contain possibly references (even openhandles ?)
-- 2.4.3
From 6085c5ce86e6ba79f29d2c18f6fceca9bab5cecb Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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.
OK, SSS_DFL_X_UMASK is still here, but not used in code.
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..
I rewrote DFL_X to DFL in sss_unique_file().
...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 third patch is about redudant constant.
And at the end, there are may uses of umask() in CI tests, which I leave how they are. They could be test relevant. Maybe I will touch it in some future patch.
The last umask like constant is 644, which is connected to chmod(), open(), etc. Do we want to have a constant for it?
Regards
Petr
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
bump
On Thu, Nov 05, 2015 at 12:22:00PM +0100, Petr Cech wrote:
On 10/21/2015 03:19 PM, Petr Cech wrote:
On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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 creatingany 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..
I checked it again. It is OK.
btw I tested both FILE ccache: krb5_ccname_template =FILE:/tmp/ccache_%p.XXXXXX the result looked OK to me: # ll /tmp/ccache_admin@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31/tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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 mayhave been
* opened as root and contain possibly references (even openhandles ?)
-- 2.4.3
From 6085c5ce86e6ba79f29d2c18f6fceca9bab5cecb Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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.
OK, SSS_DFL_X_UMASK is still here, but not used in code.
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..
I rewrote DFL_X to DFL in sss_unique_file().
...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 third patch is about redudant constant.
And at the end, there are may uses of umask() in CI tests, which I leave how they are. They could be test relevant. Maybe I will touch it in some future patch.
The last umask like constant is 644, which is connected to chmod(), open(), etc. Do we want to have a constant for it?
Regards
Petr
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
bump
I was reviewing the patches this morning :-)
ACK
On (05/11/15 12:58), Jakub Hrozek wrote:
On Thu, Nov 05, 2015 at 12:22:00PM +0100, Petr Cech wrote:
On 10/21/2015 03:19 PM, Petr Cech wrote:
On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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 creatingany 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..
I checked it again. It is OK.
btw I tested both FILE ccache: krb5_ccname_template =FILE:/tmp/ccache_%p.XXXXXX the result looked OK to me: # ll /tmp/ccache_admin@IPA.TEST.KDaxgn -rw-------. 1 admin admins 1041 Oct 12 09:14 /tmp/ccache_admin@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@IPA.TEST/ drwx------. 2 admin admins 4096 Oct 12 09:31/tmp/ccaches/ccache_admin@IPA.TEST/ # ll /tmp/ccaches/ccache_admin@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 mayhave been
* opened as root and contain possibly references (even openhandles ?)
-- 2.4.3
From 6085c5ce86e6ba79f29d2c18f6fceca9bab5cecb Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@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.
OK, SSS_DFL_X_UMASK is still here, but not used in code.
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..
I rewrote DFL_X to DFL in sss_unique_file().
...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 third patch is about redudant constant.
And at the end, there are may uses of umask() in CI tests, which I leave how they are. They could be test relevant. Maybe I will touch it in some future patch.
The last umask like constant is 644, which is connected to chmod(), open(), etc. Do we want to have a constant for it?
Regards
Petr
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
bump
I was reviewing the patches this morning :-)
ACK
master: * 691387507c256e49845d4ca2a7bd558a2b41e4bb * 56e067109659886408789c936d37c1e86fe46695 * fb75e886c2f203fe8c10e572cd4d8c635941678d
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Petr Cech