From ebbd30c237449e2bfff4c96ece7b21f1c5dcda3c Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Thu, 14 Nov 2024 10:03:00 +0200 Subject: [PATCH 1/5] Fix invalid use of open(..., O_CREAT) in openpgp.cert.d The mode argument to open() is not optional when O_CREAT is used. The compiler doesn't check for it by default but it breaks the build under _FORTIFY_SOURCE Fixes build regression on default Fedora flags introduced in commit 6e19c1602c1118938d556453ee33357ea0006d3d --- lib/keystore.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/keystore.cc b/lib/keystore.cc index da4e12e289..a64ef07d6a 100644 --- a/lib/keystore.cc +++ b/lib/keystore.cc @@ -164,7 +164,7 @@ static int acquire_write_lock(rpmtxn txn) goto exit; } - if ((fd = open(lockpath, O_WRONLY|O_CREAT)) == -1) { + if ((fd = open(lockpath, O_WRONLY|O_CREAT, 644)) == -1) { rpmlog(RPMLOG_ERR, _("Can't create writelock for keyring at %s: %s\n"), keyringpath, strerror(errno)); } else if (flock(fd, LOCK_EX|LOCK_NB)) { rpmlog(RPMLOG_ERR, _("Can't acquire writelock for keyring at %s\n"), keyringpath); From 1e7e405fa8880cdc6dcd91683db30696bfd2afce Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Thu, 14 Nov 2024 10:51:56 +0200 Subject: [PATCH 2/5] Kludge around removeSBITS() not properly checking errors for now fchmodat() on glibc has warn_unused_result attribute and so this emits a compiler warning with -fhardened, and that in turn breaks the build with -Werror. Using += is enough to trick the compiler to think we actually used the value for something. We SHOULD handle failure here somehow intelligently (eg differentiate between ENOENT and possible other harmless errors etc) but can't dive to that just now. --- lib/fsm.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/fsm.cc b/lib/fsm.cc index ec0303400c..63580c25ad 100644 --- a/lib/fsm.cc +++ b/lib/fsm.cc @@ -474,13 +474,15 @@ static void removeSBITS(int dirfd, const char *path) struct stat stb; int flags = AT_SYMLINK_NOFOLLOW; if (fstatat(dirfd, path, &stb, flags) == 0 && S_ISREG(stb.st_mode)) { + /* XXX TODO: actually check for the rc, but what to do there? */ + int rc = 0; /* We now know it's not a link so no need to worry about following */ if ((stb.st_mode & 06000) != 0) { - (void) fchmodat(dirfd, path, stb.st_mode & 0777, 0); + rc += fchmodat(dirfd, path, stb.st_mode & 0777, 0); } #ifdef WITH_CAP if (stb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) { - (void) cap_set_fileat(dirfd, path, NULL); + rc += cap_set_fileat(dirfd, path, NULL); } #endif } From e79d187aed64fff0393611d4ad67617d5acd9afe Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Thu, 14 Nov 2024 11:02:40 +0200 Subject: [PATCH 3/5] Handle error return from audit_log_user_comm_message() audit_log_user_comm_message has warn_unused_result attribute and so this emits a compiler warning with -fhardened, and that in turn breaks the build with -Werror. Emit a warning if audit log message fails, but suppress ECONNREFUSED to silence spurious warnings in environments where audit daemon isn't available, such as containers (like our test-suite) or rescue images. This isn't entirely ideal but is consistent with what we do in similar cases in eg systemd_inhibit (708e61307bc3fd027b016fdf5a1d1a5274c1843c) and dbus_announce (071be753cf05b2753efb0d3c51d4fa59b9641d09) plugins. For extra entertainment, something in the GH CI environment causes runroot_user tests to fail with EPERM, whereas no such errors occur locally. Filter it out too. --- plugins/audit.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/plugins/audit.c b/plugins/audit.c index a4309f729e..0bbbdb3cb3 100644 --- a/plugins/audit.c +++ b/plugins/audit.c @@ -1,8 +1,10 @@ #include "system.h" +#include #include #include +#include #include #include #include @@ -82,8 +84,16 @@ static rpmRC audit_tsm_post(rpmPlugin plugin, rpmts ts, int res) rasprintf(&eventTxt, "op=%s %s sw_type=rpm key_enforce=%u gpg_res=%u %s", op, nevra, enforce, verified, dir); - audit_log_user_comm_message(auditFd, AUDIT_SOFTWARE_UPDATE, - eventTxt, NULL, NULL, NULL, NULL, result); + + if (audit_log_user_comm_message(auditFd, AUDIT_SOFTWARE_UPDATE, + eventTxt, NULL, NULL, NULL, NULL, result) <= 0) + { + /* Filter out noise from containers and other novelties */ + int ignore = (errno == ECONNREFUSED || errno == EPERM); + rpmlog(ignore ? RPMLOG_DEBUG : RPMLOG_WARNING, + _("logging an audit message failed: %s\n"), + strerror(errno)); + } free(nevra); free(eventTxt); } From f11ad96d7bc80331bfe3a563d871d3b7561151b4 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Thu, 14 Nov 2024 11:28:55 +0200 Subject: [PATCH 4/5] Oops, use same optimization level etc on C and C++ compilers in CI This probably explains some of the oddities and seemingly missing warnings we've been seeing... --- tests/Dockerfile.fedora | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Dockerfile.fedora b/tests/Dockerfile.fedora index dc12be49ee..4fa82e1ca2 100644 --- a/tests/Dockerfile.fedora +++ b/tests/Dockerfile.fedora @@ -87,6 +87,7 @@ RUN chmod -R a-w . WORKDIR /srv/build ENV CFLAGS="-Og -g" +ENV CXXFLAGS="-Og -g" RUN cmake \ -DENABLE_WERROR=ON \ -DENABLE_ASAN=ON \ From b44af5a3733e986216f76a413439e57663ece5b9 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Thu, 14 Nov 2024 11:04:04 +0200 Subject: [PATCH 5/5] Always build with -fhardened if the compiler supports it This enables all manner of highly useful safety checks that we generally want enabled. --- CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 923265171b..dfb0997d8c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -421,8 +421,8 @@ if (ENABLE_ASAN OR ENABLE_UBSAN) add_compile_options(-fno-omit-frame-pointer) endif() -# try to ensure some compiler sanity -foreach (flag -fno-strict-overflow -fno-delete-null-pointer-checks) +# try to ensure some compiler sanity and hardening options where supported +foreach (flag -fno-strict-overflow -fno-delete-null-pointer-checks -fhardened) check_c_compiler_flag(${flag} found) if (found) add_compile_options(${flag})