From b7888e460a12fb55066f41764e5e2c71f307fee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= Date: Wed, 4 Oct 2023 16:38:12 +0200 Subject: [PATCH] PGP: Set a default creation SELinux labels on GnuPG directories libdnf used to precreate the directory in /run/user to make sure a GnuPG agent executed by GPGME library places its socket there. The directories there are normally created and removed by systemd (logind PAM session). libdnf created them for a case when a package manager is invoked out of systemd session, before the super user logs in. E.g. by a timer job to cache repository metadata. A problem was when this out-of-session process was a SELinux-confined process creating files with its own SELinux label different from a DNF program. Then the directory was created with a SELinux label different from the one expected by systemd and when logging out a corresponding user, the mismatching label clashed with systemd. The same issue was with temporary GnuPG home directories created by libdnf under /tmp. This patch fixes both the isseus by restoring a SELinux label of those directories to the label defined in a default SELinux file context database. Obviously the database cannot have a record for a nonspecific /tmp/tmpdir.XXXXXX (a mkdtemp() template) directory names. Therefore I changed their names to more specific /tmp/libdnf.XXXXXX. Once a SELinux policy updates the database, directories under /tmp will get a correct label. There is yet another problem with accessing /var/cache/dnf/*/pubring, but that seems to be pure SELinux policy problem. This patch adds a new -DENABLE_SELINUX=OFF CMake option to disable the new dependency on libselinux. A default behavior is to support SELinux. Implementation details: I used selabel_lookup() + setfscreatecon() + mkdtemp() + setfscreatecon() sequence instead of mkdtemp() + selinux_restorecon() sequence because the later polutes stderr if a SELinux policy does not define the default context. One could supress stderr messages with selinux_set_callback(), but its effect cannot be restored. I also kept the sequence in one function and reused it for creating /run/user/$PID directories because the code is simpler than spliting the function into three parts. https://issues.redhat.com/browse/RHEL-6421 --- CMakeLists.txt | 7 +++ libdnf.spec | 11 +++- libdnf/CMakeLists.txt | 4 ++ libdnf/repo/Repo.cpp | 121 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 129 insertions(+), 14 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d895b2bf1c..e5829e6a32 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -34,6 +34,7 @@ option(WITH_MAN "Enables hawkey man page generation" ON) option(WITH_ZCHUNK "Build with zchunk support" ON) option(ENABLE_RHSM_SUPPORT "Build with Red Hat Subscription Manager support?" OFF) option(ENABLE_SOLV_URPMREORDER "Build with support for URPM-like solution reordering?" OFF) +option(ENABLE_SELINUX "Restore SELinux labels on GnuPG directories" ON) # build options - debugging @@ -83,6 +84,12 @@ if(ENABLE_RHSM_SUPPORT) include_directories(${RHSM_INCLUDE_DIRS}) endif() +if(ENABLE_SELINUX) + pkg_check_modules(SELINUX REQUIRED libselinux) + include_directories(${SELINUX_INCLUDE_DIRS}) + add_definitions(-DENABLE_SELINUX=1) +endif() + # glibc: check if fnmatch.h has FNM_CASEFOLD symbol include(CheckSymbolExists) diff --git a/libdnf.spec b/libdnf.spec index aa51dd2823..df482f54fb 100644 --- a/libdnf.spec +++ b/libdnf.spec @@ -42,6 +42,8 @@ %bcond_with rhsm %endif +%bcond_without selinux + %if 0%{?rhel} %bcond_with zchunk %else @@ -84,6 +86,9 @@ BuildRequires: pkgconfig(sqlite3) BuildRequires: pkgconfig(json-c) BuildRequires: pkgconfig(cppunit) BuildRequires: pkgconfig(libcrypto) +%if %{with selinux} +BuildRequires: pkgconfig(libselinux) +%endif BuildRequires: pkgconfig(modulemd-2.0) >= %{libmodulemd_version} BuildRequires: pkgconfig(smartcols) BuildRequires: gettext @@ -205,7 +210,8 @@ pushd build-py2 %define __builddir build-py2 %endif %cmake -DPYTHON_DESIRED:FILEPATH=%{__python2} -DWITH_MAN=OFF ../ %{!?with_zchunk:-DWITH_ZCHUNK=OFF} %{!?with_valgrind:-DDISABLE_VALGRIND=1} %{_cmake_opts} -DLIBDNF_MAJOR_VERSION=%{libdnf_major_version} -DLIBDNF_MINOR_VERSION=%{libdnf_minor_version} -DLIBDNF_MICRO_VERSION=%{libdnf_micro_version} \ - -DWITH_SANITIZERS=%{?with_sanitizers:ON}%{!?with_sanitizers:OFF} + -DWITH_SANITIZERS=%{?with_sanitizers:ON}%{!?with_sanitizers:OFF} \ + -DENABLE_SELINUX=%{?with_selinux:ON}%{!?with_selinux:OFF} %make_build popd %endif @@ -219,7 +225,8 @@ pushd build-py3 %define __builddir build-py3 %endif %cmake -DPYTHON_DESIRED:FILEPATH=%{__python3} -DWITH_GIR=0 -DWITH_MAN=0 -Dgtkdoc=0 ../ %{!?with_zchunk:-DWITH_ZCHUNK=OFF} %{!?with_valgrind:-DDISABLE_VALGRIND=1} %{_cmake_opts} -DLIBDNF_MAJOR_VERSION=%{libdnf_major_version} -DLIBDNF_MINOR_VERSION=%{libdnf_minor_version} -DLIBDNF_MICRO_VERSION=%{libdnf_micro_version} \ - -DWITH_SANITIZERS=%{?with_sanitizers:ON}%{!?with_sanitizers:OFF} + -DWITH_SANITIZERS=%{?with_sanitizers:ON}%{!?with_sanitizers:OFF} \ + -DENABLE_SELINUX=%{?with_selinux:ON}%{!?with_selinux:OFF} %make_build popd %endif diff --git a/libdnf/CMakeLists.txt b/libdnf/CMakeLists.txt index 998a6f94a3..10b1523094 100644 --- a/libdnf/CMakeLists.txt +++ b/libdnf/CMakeLists.txt @@ -89,6 +89,10 @@ if(ENABLE_RHSM_SUPPORT) target_link_libraries(libdnf ${RHSM_LIBRARIES}) endif() +if(ENABLE_SELINUX) + target_link_libraries(libdnf ${SELINUX_LIBRARIES}) +endif() + set(DNF_SO_VERSION 2) set_target_properties(libdnf PROPERTIES OUTPUT_NAME "dnf") set_target_properties(libdnf PROPERTIES SOVERSION ${DNF_SO_VERSION}) diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp index d61a24a501..68b82cccd3 100644 --- a/libdnf/repo/Repo.cpp +++ b/libdnf/repo/Repo.cpp @@ -51,6 +51,11 @@ #include +#if ENABLE_SELINUX +#include +#include +#endif + #include #include #include @@ -649,6 +654,78 @@ std::unique_ptr Repo::Impl::lrHandleInitRemote(const char *destdir) return h; } +/* + * @brief Create a temporary directory. + * + * Creates a temporary directory with 0700 mode attempting to set a proper + * SELinux file context. Encountered errors are logged at debug level to + * a global logger. + * + * @param name_template As an input value it is a template according to + * mkdtemp(3). As an output value it will contain the created directory name. + * + * @return 0 if the directory was created, -1 if it wasn't. SELinux failures + * are not considered an error. + */ +static int create_temporary_directory(char *name_template) { + auto logger(Log::getLogger()); + int retval = 0; +#if ENABLE_SELINUX + char *old_default_context = NULL; + char *new_default_context = NULL; + int old_default_context_was_retrieved= 0; + struct selabel_handle *labeling_handle = NULL; + + /* A purpose of this piece of code is to deal with applications whose + * security policy overrides a file context for temporary files but don't + * know that libdnf executes GnuPG which expects a default file context. */ + if (0 == getfscreatecon(&old_default_context)) { + old_default_context_was_retrieved = 1; + } else { + logger->debug(tfm::format("Failed to retrieve a default SELinux context")); + } + + labeling_handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + if (NULL == labeling_handle) { + logger->debug(tfm::format("Failed to open a SELinux labeling handle: %s", + strerror(errno))); + } else { + if (selabel_lookup(labeling_handle, &new_default_context, name_template, 0700)) { + /* Here we could hard-code "system_u:object_r:user_tmp_t:s0", but + * that value should be really defined in default file context + * SELinux policy. Only log that the policy is incpomplete. */ + logger->debug(tfm::format("Failed to look up a default SELinux label for \"%s\"", + name_template)); + } else { + if (setfscreatecon(new_default_context)) { + logger->debug(tfm::format("Failed to set default SELinux context to \"%s\"", + new_default_context)); + } + freecon(new_default_context); + } + selabel_close(labeling_handle); + } +#endif + + /* mkdtemp() assures 0700 mode. */ + if (NULL == mkdtemp(name_template)) { + logger->debug(tfm::format("Failed to create a directory \"%s\": %s", + name_template, strerror(errno))); + retval = -1; + } + +#if ENABLE_SELINUX + if (old_default_context_was_retrieved) { + if (setfscreatecon(old_default_context)) { + logger->debug(tfm::format("Failed to restore a default SELinux context")); + } + } + freecon(old_default_context); +#endif + + return retval; +} + static void gpgImportKey(gpgme_ctx_t context, int keyFd) { auto logger(Log::getLogger()); @@ -703,8 +780,8 @@ static std::vector rawkey2infos(int fd) { std::unique_ptr::type> context(ctx); // set GPG home dir - char tmpdir[] = "/tmp/tmpdir.XXXXXX"; - mkdtemp(tmpdir); + char tmpdir[] = "/tmp/libdnf.XXXXXX"; + create_temporary_directory(tmpdir); Finalizer tmpDirRemover([&tmpdir](){ dnf_remove_recursive(tmpdir, NULL); }); @@ -853,6 +930,13 @@ std::vector Repo::Impl::retrieve(const std::string & url) * would cause a race condition with calling gpgme_release(), see [2], [3], * [4]. * + * Current solution precreating /run/user/$UID showed problematic when this + * library was used out of a systemd-logind session from a programm with an + * unexpected SELinux context. Then /run/user/$UID, normally maintained by + * systemd, was assigned a SELinux label unexpected by systemd causing errors + * on a user logout [5]. We remedy it by restoring the label according to + * a file context policy. + * * Since the agent doesn't clean up its sockets properly, by creating this * directory we make sure they are in a place that is not causing trouble with * container images. @@ -861,14 +945,27 @@ std::vector Repo::Impl::retrieve(const std::string & url) * [2] https://bugzilla.redhat.com/show_bug.cgi?id=1769831 * [3] https://github.com/rpm-software-management/microdnf/issues/50 * [4] https://bugzilla.redhat.com/show_bug.cgi?id=1781601 + * [5] https://issues.redhat.com/browse/RHEL-6421 */ static void ensure_socket_dir_exists() { auto logger(Log::getLogger()); + char tmpdir[] = "/run/user/libdnf.XXXXXX"; std::string dirname = "/run/user/" + std::to_string(getuid()); - int res = mkdir(dirname.c_str(), 0700); - if (res != 0 && errno != EEXIST) { - logger->debug(tfm::format("Failed to create directory \"%s\": %d - %s", - dirname, errno, strerror(errno))); + + /* create_temporary_directory() assures 0700 mode and tries its best to + * correct a SELinux label. */ + if (create_temporary_directory(tmpdir)) { + return; + } + + /* Set the desired name. */ + if (rename(tmpdir, dirname.c_str())) { + if (errno != EEXIST && errno != ENOTEMPTY && errno != EBUSY) { + logger->debug(tfm::format("Failed to rename \"%s\" directory to \"%s\": %s", + tmpdir, dirname, strerror(errno))); + } + rmdir(tmpdir); + return; } } @@ -1151,8 +1248,8 @@ void Repo::Impl::addCountmeFlag(LrHandle *handle) { bool Repo::Impl::isMetalinkInSync() { auto logger(Log::getLogger()); - char tmpdir[] = "/tmp/tmpdir.XXXXXX"; - mkdtemp(tmpdir); + char tmpdir[] = "/tmp/libdnf.XXXXXX"; + create_temporary_directory(tmpdir); Finalizer tmpDirRemover([&tmpdir](){ dnf_remove_recursive(tmpdir, NULL); }); @@ -1221,8 +1318,8 @@ bool Repo::Impl::isRepomdInSync() { auto logger(Log::getLogger()); LrYumRepo *yum_repo; - char tmpdir[] = "/tmp/tmpdir.XXXXXX"; - mkdtemp(tmpdir); + char tmpdir[] = "/tmp/libdnf.XXXXXX"; + create_temporary_directory(tmpdir); Finalizer tmpDirRemover([&tmpdir](){ dnf_remove_recursive(tmpdir, NULL); }); @@ -1260,8 +1357,8 @@ void Repo::Impl::fetch(const std::string & destdir, std::unique_ptr && throw RepoError(tfm::format(_("Cannot create repo destination directory \"%s\": %s"), destdir, errTxt)); } - auto tmpdir = destdir + "/tmpdir.XXXXXX"; - if (!mkdtemp(&tmpdir.front())) { + auto tmpdir = destdir + "/libdnf.XXXXXX"; + if (create_temporary_directory(&tmpdir.front())) { const char * errTxt = strerror(errno); throw RepoError(tfm::format(_("Cannot create repo temporary directory \"%s\": %s"), tmpdir.c_str(), errTxt));