coredump: use %d in kernel core pattern

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 <qsa@qualys.com>
(cherry-picked from 0c49e0049b7665bb7769a13ef346fef92e1ad4d6)

diff --git a/man/systemd-coredump.xml b/man/systemd-coredump.xml
index 4ab4d0625a6fa8f456610e9136ac05639ee54104..e3c368cfadbee70af6f583e853b5c0faf1c77fbd 100644 (file)
--- a/man/systemd-coredump.xml
+++ b/man/systemd-coredump.xml
@@ -329,6 +329,18 @@ COREDUMP_FILENAME=/var/lib/systemd/coredump/core.Web….552351.….zst
         </listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term><varname>COREDUMP_DUMPABLE=</varname></term>
+
+        <listitem><para>The <constant>PR_GET_DUMPABLE</constant> field as reported by the kernel, see
+        <citerefentry
+        project='man-pages'><refentrytitle>prctl</refentrytitle><manvolnum>2</manvolnum></citerefentry>.
+        </para>
+
+        <xi:include href="version-info.xml" xpointer="v258"/>
+        </listitem>
+      </varlistentry>
+
       <varlistentry>
         <term><varname>COREDUMP_OPEN_FDS=</varname></term>
 

diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c
index 245df1a7339c1dcd3905e1840afddc8ff12b6f61..17de92aa203ad4b7b59aa76ea07ff10c0b2b3fb1 100644 (file)
--- a/src/coredump/coredump.c
+++ b/src/coredump/coredump.c
@@ -108,6 +108,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,
@@ -136,6 +137,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=",
@@ -146,6 +148,7 @@ typedef struct Context {
         PidRef pidref;
         uid_t uid;
         gid_t gid;
+        unsigned dumpable;
         int signo;
         uint64_t rlimit;
         bool is_pid1;
@@ -433,14 +436,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;
 }
@@ -1083,6 +1088,16 @@ static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) {
         if (r < 0)
                 log_warning_errno(r, "Failed to parse resource limit \"%s\", ignoring: %m", context->meta[META_ARGV_RLIMIT]);
 
+        /* 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 90c080bdfefafd1a28eb29d3a616fe207b1ea9bf..a550c87258f6b84153101d6730a581674467de57 100644 (file)
--- 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=|{{LIBEXECDIR}}/systemd-coredump %P %u %g %s %t %c %h
+kernel.core_pattern=|{{LIBEXECDIR}}/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

diff --git a/test/units/TEST-87-AUX-UTILS-VM.coredump.sh b/test/units/TEST-87-AUX-UTILS-VM.coredump.sh
index 6ad7e29e28a26f33c48dbcd2ef1610487a10f40c..ce4a2c9ed1538e82070a378c6dfac7c16ef79cf5 100755 (executable)
--- a/test/units/TEST-87-AUX-UTILS-VM.coredump.sh
+++ b/test/units/TEST-87-AUX-UTILS-VM.coredump.sh
@@ -198,12 +198,17 @@ journalctl -b -n 1 --output=export --output-fields=MESSAGE,COREDUMP COREDUMP_EXE
     /usr/lib/systemd/systemd-coredump --backtrace $$ 0 0 6 1679509900 12345
 journalctl -b -n 1 --output=export --output-fields=MESSAGE,COREDUMP COREDUMP_EXE="/usr/bin/test-dump" |
     /usr/lib/systemd/systemd-coredump --backtrace $$ 0 0 6 1679509901 12345 mymachine
+journalctl -b -n 1 --output=export --output-fields=MESSAGE,COREDUMP COREDUMP_EXE="/usr/bin/test-dump" |
+    /usr/lib/systemd/systemd-coredump --backtrace $$ 0 0 6 1679509902 12345 youmachine 1
 # Wait a bit for the coredumps to get processed
 timeout 30 bash -c "while [[ \$(coredumpctl list -q --no-legend $$ | wc -l) -lt 2 ]]; do sleep 1; done"
 coredumpctl info $$
 coredumpctl info COREDUMP_TIMESTAMP=1679509900000000
 coredumpctl info COREDUMP_TIMESTAMP=1679509901000000
 coredumpctl info COREDUMP_HOSTNAME="mymachine"
+coredumpctl info COREDUMP_TIMESTAMP=1679509902000000
+coredumpctl info COREDUMP_HOSTNAME="youmachine"
+coredumpctl info COREDUMP_DUMPABLE="1"
 
 # This used to cause a stack overflow
 systemd-run -t --property CoredumpFilter=all ls /tmp