On Mon, 2011-12-19 at 09:27 +0100, Jan Zelený wrote: > > Patch 0001: Guarantee NULL-termination when reading the pidfile > > I think it would be better to use > > pid_str[len] = '\0'; > > instead of > > pid_str[pidlen] = '\0';
Good idea. Fixed. > > > Patch 0002: Always set umask when using mkstemp() > > For the record, this does fix 12397 and not 12398 as it is claimed in the > patch. Would you please also include 12396 and 12398 to this patch? Done. Thanks for the review. New patches attached.
From baa544c6bf895d7e41c494012f7c00839e9681b9 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Fri, 16 Dec 2011 10:45:46 -0500
Subject: [PATCH 1/2] Reorder pidfile() function to guarantee NULL-termination
Coverity 12400
---
src/util/server.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/server.c b/src/util/server.c
index 0c9501b1f468be32a6e7fcc924c1c419069f1204..a9be4be1a602fe31507a6ed30e3cfe9b88c372c6 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -117,9 +117,6 @@ int pidfile(const char *path, const char *name)
fd = open(file, O_RDONLY, 0644);
err = errno;
if (fd != -1) {
-
- pid_str[pidlen] = '\0';
-
len = 0;
while ((ret = read(fd, pid_str + len, pidlen - len)) != 0) {
if (ret == -1) {
@@ -141,6 +138,9 @@ int pidfile(const char *path, const char *name)
}
}
+ /* Ensure NULL-termination */
+ pid_str[len] = '\0';
+
if (ret == 0) {
/* let's check the pid */
--
1.7.7.4
From a85115c78897996a163eb6e96dd69315c43af288 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Fri, 16 Dec 2011 11:13:55 -0500
Subject: [PATCH 2/2] Securely set umask when using mkstemp
Coverity 12394, 12395, 12396, 12397 and 12398
---
src/providers/krb5/krb5_child.c | 3 +++
src/providers/krb5/krb5_common.c | 3 +++
src/tests/check_and_open-tests.c | 4 ++++
src/tests/debug-tests.c | 8 ++++++++
4 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index fe87210947cb7b826d3c9b18beb9fbf96ef5a734..7543e934e4bd39e1e87b9a317eb3dcf5cefb02ac 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -230,6 +230,7 @@ static krb5_error_code create_ccache_file(krb5_context ctx,
char *tmp_ccname;
krb5_creds *l_cred;
TALLOC_CTX *tmp_ctx = NULL;
+ mode_t old_umask;
if (strncmp(ccname, "FILE:", 5) == 0) {
cc_file_name = ccname + 5;
@@ -258,7 +259,9 @@ static krb5_error_code create_ccache_file(krb5_context ctx,
}
tmp_ccname = talloc_asprintf_append(tmp_ccname, "/.krb5cc_dummy_XXXXXX");
+ old_umask = umask(077);
fd = mkstemp(tmp_ccname);
+ umask(old_umask);
if (fd == -1) {
DEBUG(1, ("mkstemp failed [%d][%s].\n", errno, strerror(errno)));
kerr = errno;
diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c
index a065727a7b410074c9e5c20b09166e792fc67e95..c2cb94b61463cbdaf3f4fa5a5cb311af55b4b960 100644
--- a/src/providers/krb5/krb5_common.c
+++ b/src/providers/krb5/krb5_common.c
@@ -290,6 +290,7 @@ errno_t write_krb5info_file(const char *realm, const char *server,
const char *name_tmpl = NULL;
int server_len;
ssize_t written;
+ mode_t old_umask;
if (realm == NULL || *realm == '\0' || server == NULL || *server == '\0' ||
service == NULL || service == '\0') {
@@ -328,7 +329,9 @@ errno_t write_krb5info_file(const char *realm, const char *server,
goto done;
}
+ old_umask = umask(077);
fd = mkstemp(tmp_name);
+ umask(old_umask);
if (fd == -1) {
ret = errno;
DEBUG(1, ("mkstemp failed [%d][%s].\n", ret, strerror(ret)));
diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index 8e1bc988017889c596a322c47b09c29bb73e359f..6ff40d0fa486febba9dd60f7ca891d10e41eba43 100644
--- a/src/tests/check_and_open-tests.c
+++ b/src/tests/check_and_open-tests.c
@@ -43,10 +43,14 @@ int fd;
void setup_check_and_open(void)
{
int ret;
+ mode_t old_umask;
filename = strdup(FILENAME_TEMPLATE);
fail_unless(filename != NULL, "strdup failed");
+
+ old_umask = umask(077);
ret = mkstemp(filename);
+ umask(old_umask);
fail_unless(ret != -1, "mkstemp failed [%d][%s]", errno, strerror(errno));
close(ret);
diff --git a/src/tests/debug-tests.c b/src/tests/debug-tests.c
index 8a338fb5f4c8796fb6fd67771b41b1aaf901d6ab..40dd2e98b848b5a267a6c8464cb7f3275a92ee9c 100644
--- a/src/tests/debug-tests.c
+++ b/src/tests/debug-tests.c
@@ -191,10 +191,14 @@ int test_helper_debug_check_message(int level, int msgmode)
int fd;
int ret;
int _errno = 0;
+ mode_t old_umask;
FILE *file = NULL;
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
+
+ old_umask = umask(077);
fd = mkstemp(filename);
+ umask(old_umask);
if (fd == -1) {
_errno = errno;
talloc_free(ctx);
@@ -331,10 +335,14 @@ int test_helper_debug_is_empty_message(int level, int msgmode)
int filesize;
int ret;
int _errno = 0;
+ mode_t old_umask;
FILE *file;
strncpy(filename, "sssd_debug_tests.XXXXXX", 24);
+
+ old_umask = umask(077);
fd = mkstemp(filename);
+ umask(old_umask);
if (fd == -1) {
return DEBUG_TEST_ERROR;
}
--
1.7.7.4
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
