From 89730dea979b2d22fd548b622cd88bac99ff1d6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 29 Apr 2025 14:47:59 +0200 Subject: [PATCH 3/3] coredump: use %d in kernel core pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kernel provides %d which is documented as "dump mode—same as value returned by prctl(2) PR_GET_DUMPABLE". We already query /proc/pid/auxv for this information, but unfortunately this check is subject to a race, because the crashed process may be replaced by an attacker before we read this data, for example replacing a SUID process that was killed by a signal with another process that is not SUID, tricking us into making the coredump of the original process readable by the attacker. With this patch, we effectively add one more check to the list of conditions that need be satisfied if we are to make the coredump accessible to the user. Reportedy-by: Qualys Security Advisory (cherry-picked from commit 0c49e0049b7665bb7769a13ef346fef92e1ad4d6) (cherry-picked from commit c58a8a6ec9817275bb4babaa2c08e0e35090d4e3) (cherry picked from commit 19d439189ab85dd7222bdd59fd442bbcc8ea99a7) (cherry picked from commit 254ab8d2a7866679cee006d844d078774cbac3c9) (cherry picked from commit 7fc7aa5a4d28d7768dfd1eb85be385c3ea949168) (cherry picked from commit 19b228662e0fcc6596c0395a0af8486a4b3f1627) CVE: CVE-2025-4598 Upstream-Status: Backport [https://github.com/systemd/systemd-stable/commit/2eb46dce078334805c547cbcf5e6462cf9d2f9f0] Signed-off-by: Chen Qi --- man/systemd-coredump.xml | 12 ++++++++++++ src/coredump/coredump.c | 21 ++++++++++++++++++--- sysctl.d/50-coredump.conf.in | 2 +- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/man/systemd-coredump.xml b/man/systemd-coredump.xml index cb9f47745b..ba7cad12bc 100644 --- a/man/systemd-coredump.xml +++ b/man/systemd-coredump.xml @@ -259,6 +259,18 @@ COREDUMP_FILENAME=/var/lib/systemd/coredump/core.Web….552351.….zst + + COREDUMP_DUMPABLE= + + The PR_GET_DUMPABLE field as reported by the kernel, see + prctl2. + + + + + + COREDUMP_OPEN_FDS= diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 0b27086288..aca6a2eb6b 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -87,6 +87,7 @@ typedef enum { _META_ARGV_REQUIRED, /* The fields below were added to kernel/core_pattern at later points, so they might be missing. */ META_ARGV_HOSTNAME = _META_ARGV_REQUIRED, /* %h: hostname */ + META_ARGV_DUMPABLE, /* %d: as set by the kernel */ /* If new fields are added, they should be added here, to maintain compatibility * with callers which don't know about the new fields. */ _META_ARGV_MAX, @@ -115,6 +116,7 @@ static const char * const meta_field_names[_META_MAX] = { [META_ARGV_TIMESTAMP] = "COREDUMP_TIMESTAMP=", [META_ARGV_RLIMIT] = "COREDUMP_RLIMIT=", [META_ARGV_HOSTNAME] = "COREDUMP_HOSTNAME=", + [META_ARGV_DUMPABLE] = "COREDUMP_DUMPABLE=", [META_COMM] = "COREDUMP_COMM=", [META_EXE] = "COREDUMP_EXE=", [META_UNIT] = "COREDUMP_UNIT=", @@ -125,6 +127,7 @@ typedef struct Context { const char *meta[_META_MAX]; size_t meta_size[_META_MAX]; pid_t pid; + unsigned dumpable; bool is_pid1; bool is_journald; } Context; @@ -470,14 +473,16 @@ static int grant_user_access(int core_fd, const Context *context) { if (r < 0) return r; - /* We allow access if we got all the data and at_secure is not set and - * the uid/gid matches euid/egid. */ + /* We allow access if dumpable on the command line was exactly 1, we got all the data, + * at_secure is not set, and the uid/gid match euid/egid. */ bool ret = + context->dumpable == 1 && at_secure == 0 && uid != UID_INVALID && euid != UID_INVALID && uid == euid && gid != GID_INVALID && egid != GID_INVALID && gid == egid; - log_debug("Will %s access (uid="UID_FMT " euid="UID_FMT " gid="GID_FMT " egid="GID_FMT " at_secure=%s)", + log_debug("Will %s access (dumpable=%u uid="UID_FMT " euid="UID_FMT " gid="GID_FMT " egid="GID_FMT " at_secure=%s)", ret ? "permit" : "restrict", + context->dumpable, uid, euid, gid, egid, yes_no(at_secure)); return ret; } @@ -1102,6 +1107,16 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { if (r < 0) return log_error_errno(r, "Failed to parse PID \"%s\": %m", context->meta[META_ARGV_PID]); + /* The value is set to contents of /proc/sys/fs/suid_dumpable, which we set to 2, + * if the process is marked as not dumpable, see PR_SET_DUMPABLE(2const). */ + if (context->meta[META_ARGV_DUMPABLE]) { + r = safe_atou(context->meta[META_ARGV_DUMPABLE], &context->dumpable); + if (r < 0) + return log_error_errno(r, "Failed to parse dumpable field \"%s\": %m", context->meta[META_ARGV_DUMPABLE]); + if (context->dumpable > 2) + log_notice("Got unexpected %%d/dumpable value %u.", context->dumpable); + } + unit = context->meta[META_UNIT]; context->is_pid1 = streq(context->meta[META_ARGV_PID], "1") || streq_ptr(unit, SPECIAL_INIT_SCOPE); context->is_journald = streq_ptr(unit, SPECIAL_JOURNALD_SERVICE); diff --git a/sysctl.d/50-coredump.conf.in b/sysctl.d/50-coredump.conf.in index 5fb551a8cf..9c10a89828 100644 --- a/sysctl.d/50-coredump.conf.in +++ b/sysctl.d/50-coredump.conf.in @@ -13,7 +13,7 @@ # the core dump. # # See systemd-coredump(8) and core(5). -kernel.core_pattern=|{{ROOTLIBEXECDIR}}/systemd-coredump %P %u %g %s %t %c %h +kernel.core_pattern=|{{ROOTLIBEXECDIR}}/systemd-coredump %P %u %g %s %t %c %h %d # Allow 16 coredumps to be dispatched in parallel by the kernel. # We collect metadata from /proc/%P/, and thus need to make sure the crashed -- 2.34.1