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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@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_ad...@ipa.test.KDaxgn
-rw-------. 1 admin admins 1041 Oct 12 09:14
/tmp/ccache_ad...@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_ad...@ipa.test/
drwx------. 2 admin admins 4096 Oct 12 09:31
/tmp/ccaches/ccache_ad...@ipa.test/
# ll /tmp/ccaches/ccache_ad...@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 <pc...@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
>From 9f722882305360b14f56f7ff46d679bc9f8d3095 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/confdb/confdb.c | 2 +-
src/util/debug.c | 2 +-
src/util/server.c | 5 ++---
src/util/util.h | 3 +++
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index d811f7cbf597db5c5ee5fa658c8864233da8f2e0..0f76a3d140ec832467c8382df088ac0e279207c0 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -659,7 +659,7 @@ int confdb_init(TALLOC_CTX *mem_ctx,
return EIO;
}
- old_umask = umask(0177);
+ old_umask = umask(SSS_DFL_UMASK);
ret = ldb_connect(cdb->ldb, confdb_location, 0, NULL);
umask(old_umask);
diff --git a/src/util/debug.c b/src/util/debug.c
index 69df54386101973548108c3194a1bfd111f046f0..bd13fdecdbd37da8e13ed492c115570657d2588c 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -362,7 +362,7 @@ int open_debug_file_ex(const char *filename, FILE **filep, bool want_cloexec)
if (debug_file && !filep) fclose(debug_file);
- old_umask = umask(0177);
+ old_umask = umask(SSS_DFL_UMASK);
errno = 0;
f = fopen(logpath, "a");
if (f == NULL) {
diff --git a/src/util/server.c b/src/util/server.c
index 7e9b76f74ee5e76d2481eb425eff4811cc2e780e..036dace044c1e2c3efbb2411f39bdfd3f9616db4 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -490,9 +490,8 @@ int server_setup(const char *name, int flags,
setup_signals();
- /* we want default permissions on created files to be very strict,
- so set our umask to 0177 */
- umask(0177);
+ /* we want default permissions on created files to be very strict */
+ umask(SSS_DFL_UMASK);
if (flags & FLAGS_DAEMON) {
DEBUG(SSSDBG_IMPORTANT_INFO, "Becoming a daemon.\n");
diff --git a/src/util/util.h b/src/util/util.h
index f9fe1ca7189c6b2cdcb29f143005b20a2d969fee..9658d79fe9a0062b46188f2e7a97aaaebdeff29e 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -64,6 +64,9 @@
#define SSS_ATTRIBUTE_PRINTF(a1, a2)
#endif
+/** Default secure umask */
+#define SSS_DFL_UMASK 0177
+
extern const char *debug_prg_name;
extern int debug_level;
extern int debug_timestamps;
--
2.4.3
>From 0f88c44d463bcc6549ebf9e841c204ebf04c8550 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/responder/common/responder.h | 2 +-
src/responder/common/responder_common.c | 3 ++-
src/responder/pam/pamsrv.c | 3 ++-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 4d927cfe321bf3ad240b7c175568081ea73ab652..72c7f4e67e6013bada8d633c49dafa3941efe270 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -41,7 +41,7 @@ extern hash_table_t *dp_requests;
/* we want default permissions on created files to be very strict,
* so set our umask to 0177 */
-#define DFL_RSP_UMASK 0177
+#define DFL_RSP_UMASK SSS_DFL_UMASK
/* if there is a provider other than the special local */
#define NEED_CHECK_PROVIDER(provider) \
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 2097004cb0fc24d8b356f9d924243f948227ef58..baaf0412b4a70537a2523a98ff33d8f34f194b47 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -690,7 +690,8 @@ static int set_unix_socket(struct resp_ctx *rctx)
if (rctx->priv_sock_name != NULL ) {
/* create privileged pipe */
if (rctx->priv_lfd == -1) {
- ret = create_pipe_fd(rctx->priv_sock_name, &rctx->priv_lfd, 0177);
+ ret = create_pipe_fd(rctx->priv_sock_name, &rctx->priv_lfd,
+ DFL_RSP_UMASK);
if (ret != EOK) {
goto failed;
}
diff --git a/src/responder/pam/pamsrv.c b/src/responder/pam/pamsrv.c
index 3fe467c3cfc4c63b9c261065a17a54c20ea4a546..6ac770b7ac80676824cd572444359b96279902f7 100644
--- a/src/responder/pam/pamsrv.c
+++ b/src/responder/pam/pamsrv.c
@@ -396,7 +396,8 @@ int main(int argc, const char *argv[])
return 2;
}
- ret = create_pipe_fd(SSS_PAM_PRIV_SOCKET_NAME, &priv_pipe_fd, 0177);
+ ret = create_pipe_fd(SSS_PAM_PRIV_SOCKET_NAME, &priv_pipe_fd,
+ DFL_RSP_UMASK);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
"create_pipe_fd failed (priviledged pipe) [%d]: %s.\n",
--
2.4.3
>From 35febe23a1a01512bd04395db4eee9f0daf38dff Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/p11_child/p11_child_nss.c | 2 +-
src/providers/krb5/krb5_child.c | 2 +-
src/tests/check_and_open-tests.c | 2 +-
src/tests/debug-tests.c | 4 ++--
src/tests/util-tests.c | 2 +-
src/util/domain_info_utils.c | 4 ++--
src/util/util.h | 3 +++
7 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index 44ba6678893408dbfc0c6c7cfd5edcdaa789f518..123b9934870736d6a42cea969d963e622da15945 100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -482,7 +482,7 @@ int main(int argc, const char *argv[])
debug_level = SSSDBG_INVALID;
clearenv();
- umask(077);
+ umask(SSS_DFL_X_UMASK);
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 1edf10ab81d283c45e9c3343341ceaa524970e11..69b7687188c04498f6ef7c10a1b5ca602daca8ef 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(077);
+ umask(SSS_DFL_X_UMASK);
/* we create a new context here as the main process one may have been
* opened as root and contain possibly references (even open handles ?)
diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index e5981c8580f1a03afab252c20990230d9f13469b..25aee1fbfd74073e2d158ef10dc476a5b5ace381 100644
--- a/src/tests/check_and_open-tests.c
+++ b/src/tests/check_and_open-tests.c
@@ -48,7 +48,7 @@ void setup_check_and_open(void)
filename = strdup(FILENAME_TEMPLATE);
fail_unless(filename != NULL, "strdup failed");
- old_umask = umask(077);
+ old_umask = umask(SSS_DFL_X_UMASK);
ret = mkstemp(filename);
umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
diff --git a/src/tests/debug-tests.c b/src/tests/debug-tests.c
index 067209b1dda1c445b971bcec7108d0b886d55e53..8d92740143f49699bd4a1f43b0f0a81d6414df79 100644
--- a/src/tests/debug-tests.c
+++ b/src/tests/debug-tests.c
@@ -133,7 +133,7 @@ int test_helper_debug_check_message(int level)
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
- old_umask = umask(077);
+ old_umask = umask(SSS_DFL_X_UMASK);
fd = mkstemp(filename);
umask(old_umask);
if (fd == -1) {
@@ -273,7 +273,7 @@ int test_helper_debug_is_empty_message(int level)
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
- old_umask = umask(077);
+ old_umask = umask(SSS_DFL_X_UMASK);
fd = mkstemp(filename);
umask(old_umask);
if (fd == -1) {
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index bfdf078027250b8ff0ce0da2d37fbb20f391d06b..88c6727d46bb795154b5dd57c839e20a2a73d263 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -593,7 +593,7 @@ void setup_atomicio(void)
fail_unless(filename != NULL, "strdup failed");
atio_fd = -1;
- old_umask = umask(077);
+ old_umask = umask(SSS_DFL_X_UMASK);
ret = mkstemp(filename);
umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index ffbb9475b27a45c07e2e0936464c6e68ed682052..04e7d08d5f9a8ed21147bfd4a22cf4a574ddf2fd 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -312,7 +312,7 @@ sss_write_domain_mappings(struct sss_domain_info *domain)
goto done;
}
- old_mode = umask(077);
+ old_mode = umask(SSS_DFL_X_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
@@ -562,7 +562,7 @@ static errno_t sss_write_krb5_localauth_snippet(const char *path)
goto done;
}
- old_mode = umask(077);
+ old_mode = umask(SSS_DFL_X_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
diff --git a/src/util/util.h b/src/util/util.h
index 9658d79fe9a0062b46188f2e7a97aaaebdeff29e..063a97a63aa6cd360d7433e364f82fc0d6844a5e 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -67,6 +67,9 @@
/** Default secure umask */
#define SSS_DFL_UMASK 0177
+/** Secure mask with executable bit */
+#define SSS_DFL_X_UMASK 0077
+
extern const char *debug_prg_name;
extern int debug_level;
extern int debug_timestamps;
--
2.4.3
>From 372ddeb3676d4a158d7ea9975fdf3e90b1b9c29c Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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 72c7f4e67e6013bada8d633c49dafa3941efe270..f363c2074ab519c4266565aab937921a2eb44a46 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 SSS_DFL_UMASK
+/* Public sockets must be readable and writable by anybody on the system.
+ * So we set umask to 0111. */
+#define SCKT_RSP_UMASK 0111
+
/* if there is a provider other than the special local */
#define NEED_CHECK_PROVIDER(provider) \
(provider != NULL && strcmp(provider, "local") != 0)
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index baaf0412b4a70537a2523a98ff33d8f34f194b47..ebb30a458407ce5769f38ce0796572a9ae79532a 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -667,7 +667,7 @@ static int set_unix_socket(struct resp_ctx *rctx)
/* Set the umask so that permissions are set right on the socket.
* It must be readable and writable by anybody on the system. */
if (rctx->lfd == -1) {
- ret = create_pipe_fd(rctx->sock_name, &rctx->lfd, 0111);
+ ret = create_pipe_fd(rctx->sock_name, &rctx->lfd, SCKT_RSP_UMASK);
if (ret != EOK) {
return ret;
}
diff --git a/src/responder/pam/pamsrv.c b/src/responder/pam/pamsrv.c
index 6ac770b7ac80676824cd572444359b96279902f7..a63b52ec173342b6089adb5d7131cdc2d8a526d2 100644
--- a/src/responder/pam/pamsrv.c
+++ b/src/responder/pam/pamsrv.c
@@ -388,7 +388,7 @@ int main(int argc, const char *argv[])
/* Crate pipe file descriptors here before privileges are dropped
* in server_setup() */
- ret = create_pipe_fd(SSS_PAM_SOCKET_NAME, &pipe_fd, 0111);
+ ret = create_pipe_fd(SSS_PAM_SOCKET_NAME, &pipe_fd, SCKT_RSP_UMASK);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
"create_pipe_fd failed [%d]: %s.\n",
--
2.4.3
>From 03e5a04528c101ab27f8e681d24ead39e33f0c7d Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/p11_child/p11_child_nss.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index 123b9934870736d6a42cea969d963e622da15945..8a383a044dd295c911a1b12c6caeeffc57d00be6 100644
--- a/src/p11_child/p11_child_nss.c
+++ b/src/p11_child/p11_child_nss.c
@@ -481,8 +481,12 @@ int main(int argc, const char *argv[])
/* Set debug level to invalid value so we can decide if -d 0 was used. */
debug_level = SSSDBG_INVALID;
+ /*
+ * This child runs as root (setuid(0)), so we need clear environment and
+ * set permissions for security reasons.
+ */
clearenv();
- umask(SSS_DFL_X_UMASK);
+ umask(SSS_DFL_UMASK);
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
--
2.4.3
>From 56bf17d34ad3d5c4c67d9c56c544daf5d7619743 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/util/domain_info_utils.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 04e7d08d5f9a8ed21147bfd4a22cf4a574ddf2fd..baed132ce72824138be5d1dae4f9d815e927be24 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -312,7 +312,7 @@ sss_write_domain_mappings(struct sss_domain_info *domain)
goto done;
}
- old_mode = umask(SSS_DFL_X_UMASK);
+ old_mode = umask(SSS_DFL_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
@@ -562,7 +562,7 @@ static errno_t sss_write_krb5_localauth_snippet(const char *path)
goto done;
}
- old_mode = umask(SSS_DFL_X_UMASK);
+ old_mode = umask(SSS_DFL_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
--
2.4.3
>From c4812d10e0ee79073dfb1554cebedd104f9b05a3 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/tests/util-tests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index 88c6727d46bb795154b5dd57c839e20a2a73d263..df4cf969fb48c4834b5a84aa1700fc2ea4eb74b1 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -593,7 +593,7 @@ void setup_atomicio(void)
fail_unless(filename != NULL, "strdup failed");
atio_fd = -1;
- old_umask = umask(SSS_DFL_X_UMASK);
+ old_umask = umask(SSS_DFL_UMASK);
ret = mkstemp(filename);
umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
--
2.4.3
>From 6835222476889b09b8888eaa3a9802ea70e9d42d Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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
---
src/tests/debug-tests.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/tests/debug-tests.c b/src/tests/debug-tests.c
index 8d92740143f49699bd4a1f43b0f0a81d6414df79..d904d7eb8b5418608023faca0d62067f3106d23b 100644
--- a/src/tests/debug-tests.c
+++ b/src/tests/debug-tests.c
@@ -133,7 +133,7 @@ int test_helper_debug_check_message(int level)
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
- old_umask = umask(SSS_DFL_X_UMASK);
+ old_umask = umask(SSS_DFL_UMASK);
fd = mkstemp(filename);
umask(old_umask);
if (fd == -1) {
@@ -273,7 +273,7 @@ int test_helper_debug_is_empty_message(int level)
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
- old_umask = umask(SSS_DFL_X_UMASK);
+ old_umask = umask(SSS_DFL_UMASK);
fd = mkstemp(filename);
umask(old_umask);
if (fd == -1) {
--
2.4.3
>From b5ea62b2439566ac31f9c1d8b2e17416b79d4fe8 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@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.
Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
src/tests/check_and_open-tests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index 25aee1fbfd74073e2d158ef10dc476a5b5ace381..525e28aedd9ad02fffdebdb95cff9df153642ddb 100644
--- a/src/tests/check_and_open-tests.c
+++ b/src/tests/check_and_open-tests.c
@@ -48,7 +48,7 @@ void setup_check_and_open(void)
filename = strdup(FILENAME_TEMPLATE);
fail_unless(filename != NULL, "strdup failed");
- old_umask = umask(SSS_DFL_X_UMASK);
+ old_umask = umask(SSS_DFL_UMASK);
ret = mkstemp(filename);
umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
--
2.4.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
