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
>From 1da3d15cf5cfcd72742cb05be9a144ab40db7d29 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Tue, 15 Sep 2015 10:50:37 -0400
Subject: [PATCH] REFACTOR: Review of umask() function
We have many uses of umask() in our code. This patch substitute
values with constants and add comments at some cases.
Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
src/confdb/confdb.c | 2 +-
src/p11_child/p11_child_nss.c | 2 +-
src/providers/krb5/krb5_ccache.c | 1 +
src/providers/krb5/krb5_child.c | 2 +-
src/responder/common/responder_common.c | 3 ++-
src/responder/pam/pamsrv.c | 3 ++-
src/tests/check_and_open-tests.c | 2 +-
src/tests/debug-tests.c | 4 ++--
src/tests/util-tests.c | 2 +-
src/util/debug.c | 2 +-
src/util/domain_info_utils.c | 4 ++--
src/util/server.c | 2 +-
src/util/util.h | 2 ++
13 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 3a8a1c01b92e62302ac4f787ccd085be9d8f05c3..d71a50724d292bbea7d49e650062e11066c6ff77 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_VERY_RESTRICTIVE_UMASK);
ret = ldb_connect(cdb->ldb, confdb_location, 0, NULL);
umask(old_umask);
diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
index 44ba6678893408dbfc0c6c7cfd5edcdaa789f518..d999e7485f5f67792502400084dbd603f1558a8d 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_VERY_RESTRICTIVE_UMASK);
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while ((opt = poptGetNextOpt(pc)) != -1) {
diff --git a/src/providers/krb5/krb5_ccache.c b/src/providers/krb5/krb5_ccache.c
index f9bb25efd4ca3257845c3b157667d21d24299f4a..5de596f341a53958f312d114c1f95c4728d9d5df 100644
--- a/src/providers/krb5/krb5_ccache.c
+++ b/src/providers/krb5/krb5_ccache.c
@@ -185,6 +185,7 @@ static errno_t create_ccache_dir(const char *ccdirname, uid_t uid, gid_t gid)
"Creating directory [%s].\n", li->s);
new_dir_mode = 0700;
+ /* We need umask 0000 because we will create directory. */
old_umask = umask(0000);
ret = mkdir(li->s, new_dir_mode);
umask(old_umask);
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 1edf10ab81d283c45e9c3343341ceaa524970e11..6f9bad4e4caad3ca6f6c277397669ca800a6e64e 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_VERY_RESTRICTIVE_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/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 36e7f15948632e9c637886dee259b494e46ceecb..1dc9d8ce757d80865e7b21d75fa79d0621b924ec 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",
diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index e5981c8580f1a03afab252c20990230d9f13469b..c66579214f9cd8c77efb294fcfb086ca19fe98c2 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_VERY_RESTRICTIVE_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..1e221dfd23dcb49e07b91fa0969b3302b5c21477 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_VERY_RESTRICTIVE_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_VERY_RESTRICTIVE_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..523b282f39ccc8645e8eab18ef238202d59395d4 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_VERY_RESTRICTIVE_UMASK);
ret = mkstemp(filename);
umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
diff --git a/src/util/debug.c b/src/util/debug.c
index 69df54386101973548108c3194a1bfd111f046f0..479aed0cab96c708c10920d553de362085ebe2a6 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_VERY_RESTRICTIVE_UMASK);
errno = 0;
f = fopen(logpath, "a");
if (f == NULL) {
diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 4eabcff7a0e0af342ec3833d24da26ede0cb5148..f031950e254ce2b9bf25fc901ede85c0ba41d211 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -309,7 +309,7 @@ sss_write_domain_mappings(struct sss_domain_info *domain)
goto done;
}
- old_mode = umask(077);
+ old_mode = umask(SSS_VERY_RESTRICTIVE_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
@@ -559,7 +559,7 @@ static errno_t sss_write_krb5_localauth_snippet(const char *path)
goto done;
}
- old_mode = umask(077);
+ old_mode = umask(SSS_VERY_RESTRICTIVE_UMASK);
fd = mkstemp(tmp_file);
umask(old_mode);
if (fd < 0) {
diff --git a/src/util/server.c b/src/util/server.c
index 7e9b76f74ee5e76d2481eb425eff4811cc2e780e..f0826d99458a01328547a7dd9c1dab6fe840356a 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -492,7 +492,7 @@ int server_setup(const char *name, int flags,
/* we want default permissions on created files to be very strict,
so set our umask to 0177 */
- umask(0177);
+ umask(SSS_VERY_RESTRICTIVE_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 3e29e748768fc2cac547d16d61b449f1b555a56f..25bcda06eea9b2f6ee6704dec6500bd1eb7e0fcc 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -64,6 +64,8 @@
#define SSS_ATTRIBUTE_PRINTF(a1, a2)
#endif
+#define SSS_VERY_RESTRICTIVE_UMASK 0177
+
extern const char *debug_prg_name;
extern int debug_level;
extern int debug_timestamps;
--
2.4.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
