From d000375944e8ec965486cf019c3f75f4c06a4e10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Thu, 3 Feb 2022 14:14:21 +0100 Subject: [PATCH 1/5] Prevent file permissions errors The sysctl setting `fs.protected_regular` doesn't allow `O_CREAT` open on regular files that we don't own in world writable sticky directories (think `/tmp`). This causes permission denied error when writing HTML report to a temporary files created by the `mktemp` command executed as a normal user and then executing `sudo oscap`. See https://bugzilla.redhat.com/show_bug.cgi?id=2048571 If OpenSCAP fails to open the file because of permissions, it will retry to open the file without O_CREAT flag. This fixes only creation of the HTML report using the `--report` option, We will have to create a similar patch also for other output options such as `--results` or `--results-arf`. --- src/source/xslt.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/source/xslt.c b/src/source/xslt.c index 24c4c46e9..7a3a3f328 100644 --- a/src/source/xslt.c +++ b/src/source/xslt.c @@ -22,6 +22,7 @@ #include #endif +#include #include #include #include @@ -82,23 +83,38 @@ static int xccdf_ns_xslt_workaround(xmlDocPtr doc, xmlNodePtr node) static inline int save_stylesheet_result_to_file(xmlDoc *resulting_doc, xsltStylesheet *stylesheet, const char *outfile) { - FILE *f = NULL; - if (outfile) - f = fopen(outfile, "w"); - else - f = stdout; - - if (f == NULL) { - oscap_seterr(OSCAP_EFAMILY_OSCAP, "Could not open output file '%s'", outfile ? outfile : "stdout"); - return -1; + int fd = STDOUT_FILENO; + if (outfile) { +#ifdef OS_WINDOWS + fd = open(outfile, O_WRONLY|O_CREAT|O_TRUNC, S_IREAD|S_IWRITE); +#else + fd = open(outfile, O_WRONLY|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); +#endif + if (fd == -1) { + if (errno == EACCES) { + /* File already exists and we aren't allowed to create a new one + with the same name */ +#ifdef OS_WINDOWS + fd = open(outfile, O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE); +#else + fd = open(outfile, O_WRONLY|O_TRUNC, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); +#endif + } + if (fd == -1) { + oscap_seterr(OSCAP_EFAMILY_OSCAP, + "Could not open output file '%s': %s", + outfile, strerror(errno)); + return -1; + } + } } - int ret = xsltSaveResultToFile(f, resulting_doc, stylesheet); + int ret = xsltSaveResultToFd(fd, resulting_doc, stylesheet); if (ret < 0) { oscap_seterr(OSCAP_EFAMILY_OSCAP, "Could not save result document"); } - if (outfile && f) - fclose(f); + if (fd != STDOUT_FILENO) + close(fd); return ret; } From ad3c89a72c0aeb6c6ceab0873c51b07deba45701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Thu, 3 Feb 2022 15:08:52 +0100 Subject: [PATCH 2/5] Prevent permission access issues Very similar to the previous commit, but this time for saving XML documents. --- src/common/elements.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/common/elements.c b/src/common/elements.c index e512f5e3d..d0d9170f1 100644 --- a/src/common/elements.c +++ b/src/common/elements.c @@ -233,9 +233,22 @@ int oscap_xml_save_filename(const char *filename, xmlDocPtr doc) int fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); #endif - if (fd < 0) { - oscap_seterr(OSCAP_EFAMILY_GLIBC, "%s '%s'", strerror(errno), filename); - return -1; + if (fd == -1) { + if (errno == EACCES) { + /* File already exists and we aren't allowed to create a new one + with the same name */ +#ifdef OS_WINDOWS + fd = open(filename, O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE); +#else + fd = open(filename, O_WRONLY|O_TRUNC, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); +#endif + } + if (fd == -1) { + oscap_seterr(OSCAP_EFAMILY_GLIBC, + "%s '%s'", strerror(errno), filename); + return -1; + } } buff = xmlOutputBufferCreateFd(fd, NULL); From b2dc90fb80419e30d05676660a2069050693078d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Thu, 3 Feb 2022 15:37:31 +0100 Subject: [PATCH 3/5] Add a missing include --- src/source/xslt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/source/xslt.c b/src/source/xslt.c index 7a3a3f328..a763d6b59 100644 --- a/src/source/xslt.c +++ b/src/source/xslt.c @@ -30,6 +30,8 @@ #include #include #include +#include + #ifdef OS_WINDOWS #include #else From 13ff98bd744ea542bc782e388fdedb5b7f66e54b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Thu, 3 Feb 2022 16:22:56 +0100 Subject: [PATCH 4/5] Fix missing STDOUT_FILENO on Windows --- src/source/xslt.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/source/xslt.c b/src/source/xslt.c index a763d6b59..799d5170d 100644 --- a/src/source/xslt.c +++ b/src/source/xslt.c @@ -85,7 +85,11 @@ static int xccdf_ns_xslt_workaround(xmlDocPtr doc, xmlNodePtr node) static inline int save_stylesheet_result_to_file(xmlDoc *resulting_doc, xsltStylesheet *stylesheet, const char *outfile) { +#ifdef OS_WINDOWS + int fd = _fileno(stdout); +#else int fd = STDOUT_FILENO; +#endif if (outfile) { #ifdef OS_WINDOWS fd = open(outfile, O_WRONLY|O_CREAT|O_TRUNC, S_IREAD|S_IWRITE); @@ -115,7 +119,11 @@ static inline int save_stylesheet_result_to_file(xmlDoc *resulting_doc, xsltStyl if (ret < 0) { oscap_seterr(OSCAP_EFAMILY_OSCAP, "Could not save result document"); } +#ifdef OS_WINDOWS + if (fd != _fileno(stdout)) +#else if (fd != STDOUT_FILENO) +#endif close(fd); return ret; } From f00831513ec2cdde13c12820ff6cc0eef8105c65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Fri, 4 Feb 2022 12:19:50 +0100 Subject: [PATCH 5/5] Refactor: extract function oscap_open_writable --- src/common/elements.c | 26 ++-------------- src/common/util.c | 31 +++++++++++++++++++ src/common/util.h | 14 ++++++++- src/source/xslt.c | 27 ++-------------- tests/API/XCCDF/unittests/CMakeLists.txt | 2 ++ tests/API/XCCDF/unittests/test_oscap_common.c | 1 + 6 files changed, 53 insertions(+), 48 deletions(-) diff --git a/src/common/elements.c b/src/common/elements.c index d0d9170f1..de4be88f1 100644 --- a/src/common/elements.c +++ b/src/common/elements.c @@ -227,29 +227,9 @@ int oscap_xml_save_filename(const char *filename, xmlDocPtr doc) xmlCode = xmlSaveFormatFileEnc(filename, doc, "UTF-8", 1); } else { -#ifdef OS_WINDOWS - int fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, S_IREAD|S_IWRITE); -#else - int fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); -#endif - if (fd == -1) { - if (errno == EACCES) { - /* File already exists and we aren't allowed to create a new one - with the same name */ -#ifdef OS_WINDOWS - fd = open(filename, O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE); -#else - fd = open(filename, O_WRONLY|O_TRUNC, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); -#endif - } - if (fd == -1) { - oscap_seterr(OSCAP_EFAMILY_GLIBC, - "%s '%s'", strerror(errno), filename); - return -1; - } - } + int fd = oscap_open_writable(filename); + if (fd == -1) + return -1; buff = xmlOutputBufferCreateFd(fd, NULL); if (buff == NULL) { diff --git a/src/common/util.c b/src/common/util.c index 8f9f751e2..8ca2ad130 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -25,12 +25,14 @@ #include #endif +#include #include #include #include #include #include #include +#include #include "util.h" #include "_error.h" @@ -482,3 +484,32 @@ char *oscap_windows_error_message(unsigned long error_code) return error_message; } #endif + +int oscap_open_writable(const char *filename) +{ +#ifdef OS_WINDOWS + int fd = open(filename, O_WRONLY|O_CREAT|O_TRUNC, S_IREAD|S_IWRITE); +#else + int fd = open(filename, O_WRONLY|O_CREAT|O_TRUNC, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); +#endif + if (fd == -1) { + if (errno == EACCES) { + /* File already exists and we aren't allowed to create a new one + with the same name */ +#ifdef OS_WINDOWS + fd = open(filename, O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE); +#else + fd = open(filename, O_WRONLY|O_TRUNC, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); +#endif + } + if (fd == -1) { + oscap_seterr(OSCAP_EFAMILY_OSCAP, + "Could not open output file '%s': %s", + filename, strerror(errno)); + return -1; + } + } + return fd; +} diff --git a/src/common/util.h b/src/common/util.h index c48d92a52..5712f2f0b 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -524,4 +524,16 @@ wchar_t *oscap_windows_str_to_wstr(const char *str); char *oscap_windows_error_message(unsigned long error_code); #endif -#endif /* OSCAP_UTIL_H_ */ +/** + * Open a file for writing. + * The main difference from fopen() is that if the file exists but its opening + * for writing fails as permission denied, it will attempt to open it again + * without the O_CREAT flag. This is useful when writing to world-writeable + * directories with sticky bit such as /tmp on systems with fs.protected_regular + * turned on. + * @param filename name of the file to be opened + * @return file descriptor or -1 on error + */ +int oscap_open_writable(const char *filename); + +#endif /* OSCAP_UTIL_H_ */ diff --git a/src/source/xslt.c b/src/source/xslt.c index 799d5170d..906bfabdb 100644 --- a/src/source/xslt.c +++ b/src/source/xslt.c @@ -22,7 +22,6 @@ #include #endif -#include #include #include #include @@ -30,7 +29,6 @@ #include #include #include -#include #ifdef OS_WINDOWS #include @@ -91,29 +89,10 @@ static inline int save_stylesheet_result_to_file(xmlDoc *resulting_doc, xsltStyl int fd = STDOUT_FILENO; #endif if (outfile) { -#ifdef OS_WINDOWS - fd = open(outfile, O_WRONLY|O_CREAT|O_TRUNC, S_IREAD|S_IWRITE); -#else - fd = open(outfile, O_WRONLY|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); -#endif - if (fd == -1) { - if (errno == EACCES) { - /* File already exists and we aren't allowed to create a new one - with the same name */ -#ifdef OS_WINDOWS - fd = open(outfile, O_WRONLY|O_TRUNC, S_IREAD|S_IWRITE); -#else - fd = open(outfile, O_WRONLY|O_TRUNC, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH); -#endif - } - if (fd == -1) { - oscap_seterr(OSCAP_EFAMILY_OSCAP, - "Could not open output file '%s': %s", - outfile, strerror(errno)); - return -1; - } - } + fd = oscap_open_writable(outfile); } + if (fd == -1) + return -1; int ret = xsltSaveResultToFd(fd, resulting_doc, stylesheet); if (ret < 0) { diff --git a/tests/API/XCCDF/unittests/CMakeLists.txt b/tests/API/XCCDF/unittests/CMakeLists.txt index 97e1ab09f..a1c7da67f 100644 --- a/tests/API/XCCDF/unittests/CMakeLists.txt +++ b/tests/API/XCCDF/unittests/CMakeLists.txt @@ -2,6 +2,8 @@ add_oscap_test_executable(test_oscap_common "test_oscap_common.c" ${CMAKE_SOURCE_DIR}/src/common/util.c ${CMAKE_SOURCE_DIR}/src/common/list.c + ${CMAKE_SOURCE_DIR}/src/common/error.c + ${CMAKE_SOURCE_DIR}/src/common/err_queue.c ) add_oscap_test_executable(test_xccdf_overrides diff --git a/tests/API/XCCDF/unittests/test_oscap_common.c b/tests/API/XCCDF/unittests/test_oscap_common.c index 26ff51d48..939ba8750 100644 --- a/tests/API/XCCDF/unittests/test_oscap_common.c +++ b/tests/API/XCCDF/unittests/test_oscap_common.c @@ -28,6 +28,7 @@ #include #include "common/list.h" #include "common/util.h" +#include "common/_error.h" #include "oscap_assert.h" #define SEEN_LEN 9