|
| 1 | +From 254ab8d2a7866679cee006d844d078774cbac3c9 Mon Sep 17 00:00:00 2001 |
| 2 | +From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= < [email protected]> |
| 3 | +Date: Tue, 29 Apr 2025 14:47:59 +0200 |
| 4 | +Subject: [PATCH] coredump: use %d in kernel core pattern |
| 5 | +MIME-Version: 1.0 |
| 6 | +Content-Type: text/plain; charset=UTF-8 |
| 7 | +Content-Transfer-Encoding: 8bit |
| 8 | + |
| 9 | +The kernel provides %d which is documented as |
| 10 | +"dump mode—same as value returned by prctl(2) PR_GET_DUMPABLE". |
| 11 | + |
| 12 | +We already query /proc/pid/auxv for this information, but unfortunately this |
| 13 | +check is subject to a race, because the crashed process may be replaced by an |
| 14 | +attacker before we read this data, for example replacing a SUID process that |
| 15 | +was killed by a signal with another process that is not SUID, tricking us into |
| 16 | +making the coredump of the original process readable by the attacker. |
| 17 | + |
| 18 | +With this patch, we effectively add one more check to the list of conditions |
| 19 | +that need be satisfied if we are to make the coredump accessible to the user. |
| 20 | + |
| 21 | +Reportedy-by: Qualys Security Advisory < [email protected]> |
| 22 | + |
| 23 | +(cherry-picked from commit 0c49e0049b7665bb7769a13ef346fef92e1ad4d6) |
| 24 | +(cherry-picked from commit c58a8a6ec9817275bb4babaa2c08e0e35090d4e3) |
| 25 | +(cherry picked from commit 19d439189ab85dd7222bdd59fd442bbcc8ea99a7) |
| 26 | + |
| 27 | +Upstream Patch Reference: https://github.com/systemd/systemd-stable/commit/254ab8d2a7866679cee006d844d078774cbac3c9.patch |
| 28 | +--- |
| 29 | + man/systemd-coredump.xml | 12 ++++++++++++ |
| 30 | + src/coredump/coredump.c | 21 ++++++++++++++++++--- |
| 31 | + sysctl.d/50-coredump.conf.in | 2 +- |
| 32 | + 3 files changed, 31 insertions(+), 4 deletions(-) |
| 33 | + |
| 34 | +diff --git a/man/systemd-coredump.xml b/man/systemd-coredump.xml |
| 35 | +index cb9f477..ba7cad1 100644 |
| 36 | +--- a/man/systemd-coredump.xml |
| 37 | ++++ b/man/systemd-coredump.xml |
| 38 | +@@ -259,6 +259,18 @@ COREDUMP_FILENAME=/var/lib/systemd/coredump/core.Web….552351.….zst |
| 39 | + </listitem> |
| 40 | + </varlistentry> |
| 41 | + |
| 42 | ++ <varlistentry> |
| 43 | ++ <term><varname>COREDUMP_DUMPABLE=</varname></term> |
| 44 | ++ |
| 45 | ++ <listitem><para>The <constant>PR_GET_DUMPABLE</constant> field as reported by the kernel, see |
| 46 | ++ <citerefentry |
| 47 | ++ project='man-pages'><refentrytitle>prctl</refentrytitle><manvolnum>2</manvolnum></citerefentry>. |
| 48 | ++ </para> |
| 49 | ++ |
| 50 | ++ <xi:include href="version-info.xml" xpointer="v258"/> |
| 51 | ++ </listitem> |
| 52 | ++ </varlistentry> |
| 53 | ++ |
| 54 | + <varlistentry> |
| 55 | + <term><varname>COREDUMP_OPEN_FDS=</varname></term> |
| 56 | + |
| 57 | +diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c |
| 58 | +index 742a8bc..1b7d014 100644 |
| 59 | +--- a/src/coredump/coredump.c |
| 60 | ++++ b/src/coredump/coredump.c |
| 61 | +@@ -85,6 +85,7 @@ enum { |
| 62 | + META_ARGV_TIMESTAMP, /* %t: time of dump, expressed as seconds since the Epoch (we expand this to µs granularity) */ |
| 63 | + META_ARGV_RLIMIT, /* %c: core file size soft resource limit */ |
| 64 | + META_ARGV_HOSTNAME, /* %h: hostname */ |
| 65 | ++ META_ARGV_DUMPABLE, /* %d: as set by the kernel */ |
| 66 | + _META_ARGV_MAX, |
| 67 | + |
| 68 | + /* The following indexes are cached for a couple of special fields we use (and |
| 69 | +@@ -112,6 +113,7 @@ static const char * const meta_field_names[_META_MAX] = { |
| 70 | + [META_ARGV_TIMESTAMP] = "COREDUMP_TIMESTAMP=", |
| 71 | + [META_ARGV_RLIMIT] = "COREDUMP_RLIMIT=", |
| 72 | + [META_ARGV_HOSTNAME] = "COREDUMP_HOSTNAME=", |
| 73 | ++ [META_ARGV_DUMPABLE] = "COREDUMP_DUMPABLE=", |
| 74 | + [META_COMM] = "COREDUMP_COMM=", |
| 75 | + [META_EXE] = "COREDUMP_EXE=", |
| 76 | + [META_UNIT] = "COREDUMP_UNIT=", |
| 77 | +@@ -122,6 +124,7 @@ typedef struct Context { |
| 78 | + const char *meta[_META_MAX]; |
| 79 | + size_t meta_size[_META_MAX]; |
| 80 | + pid_t pid; |
| 81 | ++ unsigned dumpable; |
| 82 | + bool is_pid1; |
| 83 | + bool is_journald; |
| 84 | + } Context; |
| 85 | +@@ -467,14 +470,16 @@ static int grant_user_access(int core_fd, const Context *context) { |
| 86 | + if (r < 0) |
| 87 | + return r; |
| 88 | + |
| 89 | +- /* We allow access if we got all the data and at_secure is not set and |
| 90 | +- * the uid/gid matches euid/egid. */ |
| 91 | ++ /* We allow access if dumpable on the command line was exactly 1, we got all the data, |
| 92 | ++ * at_secure is not set, and the uid/gid match euid/egid. */ |
| 93 | + bool ret = |
| 94 | ++ context->dumpable == 1 && |
| 95 | + at_secure == 0 && |
| 96 | + uid != UID_INVALID && euid != UID_INVALID && uid == euid && |
| 97 | + gid != GID_INVALID && egid != GID_INVALID && gid == egid; |
| 98 | +- log_debug("Will %s access (uid="UID_FMT " euid="UID_FMT " gid="GID_FMT " egid="GID_FMT " at_secure=%s)", |
| 99 | ++ log_debug("Will %s access (dumpable=%u uid="UID_FMT " euid="UID_FMT " gid="GID_FMT " egid="GID_FMT " at_secure=%s)", |
| 100 | + ret ? "permit" : "restrict", |
| 101 | ++ context->dumpable, |
| 102 | + uid, euid, gid, egid, yes_no(at_secure)); |
| 103 | + return ret; |
| 104 | + } |
| 105 | +@@ -1103,6 +1108,16 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { |
| 106 | + if (r < 0) |
| 107 | + return log_error_errno(r, "Failed to parse PID \"%s\": %m", context->meta[META_ARGV_PID]); |
| 108 | + |
| 109 | ++ /* The value is set to contents of /proc/sys/fs/suid_dumpable, which we set to 2, |
| 110 | ++ * if the process is marked as not dumpable, see PR_SET_DUMPABLE(2const). */ |
| 111 | ++ if (context->meta[META_ARGV_DUMPABLE]) { |
| 112 | ++ r = safe_atou(context->meta[META_ARGV_DUMPABLE], &context->dumpable); |
| 113 | ++ if (r < 0) |
| 114 | ++ return log_error_errno(r, "Failed to parse dumpable field \"%s\": %m", context->meta[META_ARGV_DUMPABLE]); |
| 115 | ++ if (context->dumpable > 2) |
| 116 | ++ log_notice("Got unexpected %%d/dumpable value %u.", context->dumpable); |
| 117 | ++ } |
| 118 | ++ |
| 119 | + unit = context->meta[META_UNIT]; |
| 120 | + context->is_pid1 = streq(context->meta[META_ARGV_PID], "1") || streq_ptr(unit, SPECIAL_INIT_SCOPE); |
| 121 | + context->is_journald = streq_ptr(unit, SPECIAL_JOURNALD_SERVICE); |
| 122 | +diff --git a/sysctl.d/50-coredump.conf.in b/sysctl.d/50-coredump.conf.in |
| 123 | +index 5fb551a..9c10a89 100644 |
| 124 | +--- a/sysctl.d/50-coredump.conf.in |
| 125 | ++++ b/sysctl.d/50-coredump.conf.in |
| 126 | +@@ -13,7 +13,7 @@ |
| 127 | + # the core dump. |
| 128 | + # |
| 129 | + # See systemd-coredump(8) and core(5). |
| 130 | +-kernel.core_pattern=|{{ROOTLIBEXECDIR}}/systemd-coredump %P %u %g %s %t %c %h |
| 131 | ++kernel.core_pattern=|{{ROOTLIBEXECDIR}}/systemd-coredump %P %u %g %s %t %c %h %d |
| 132 | + |
| 133 | + # Allow 16 coredumps to be dispatched in parallel by the kernel. |
| 134 | + # We collect metadata from /proc/%P/, and thus need to make sure the crashed |
| 135 | +-- |
| 136 | +2.45.2 |
| 137 | + |
0 commit comments