diff --git a/5000-coredump-adjust-whitespace.patch b/5000-coredump-adjust-whitespace.patch new file mode 100644 index 00000000..ab21a57e --- /dev/null +++ b/5000-coredump-adjust-whitespace.patch @@ -0,0 +1,101 @@ +From 2f427f96135fbf3743eb58cfa9216fb605f0891d Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= +Date: Tue, 29 Nov 2022 09:00:16 +0100 +Subject: [PATCH 5000/5001] coredump: adjust whitespace + +--- + src/coredump/coredump.c | 56 ++++++++++++++++++++--------------------- + 1 file changed, 28 insertions(+), 28 deletions(-) + +diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c +index 98e7492811..7a181bdeeb 100644 +--- a/src/coredump/coredump.c ++++ b/src/coredump/coredump.c +@@ -110,16 +110,16 @@ enum { + }; + + static const char * const meta_field_names[_META_MAX] = { +- [META_ARGV_PID] = "COREDUMP_PID=", +- [META_ARGV_UID] = "COREDUMP_UID=", +- [META_ARGV_GID] = "COREDUMP_GID=", +- [META_ARGV_SIGNAL] = "COREDUMP_SIGNAL=", +- [META_ARGV_TIMESTAMP] = "COREDUMP_TIMESTAMP=", +- [META_ARGV_RLIMIT] = "COREDUMP_RLIMIT=", +- [META_ARGV_HOSTNAME] = "COREDUMP_HOSTNAME=", +- [META_COMM] = "COREDUMP_COMM=", +- [META_EXE] = "COREDUMP_EXE=", +- [META_UNIT] = "COREDUMP_UNIT=", ++ [META_ARGV_PID] = "COREDUMP_PID=", ++ [META_ARGV_UID] = "COREDUMP_UID=", ++ [META_ARGV_GID] = "COREDUMP_GID=", ++ [META_ARGV_SIGNAL] = "COREDUMP_SIGNAL=", ++ [META_ARGV_TIMESTAMP] = "COREDUMP_TIMESTAMP=", ++ [META_ARGV_RLIMIT] = "COREDUMP_RLIMIT=", ++ [META_ARGV_HOSTNAME] = "COREDUMP_HOSTNAME=", ++ [META_COMM] = "COREDUMP_COMM=", ++ [META_EXE] = "COREDUMP_EXE=", ++ [META_UNIT] = "COREDUMP_UNIT=", + }; + + typedef struct Context { +@@ -138,9 +138,9 @@ typedef enum CoredumpStorage { + } CoredumpStorage; + + static const char* const coredump_storage_table[_COREDUMP_STORAGE_MAX] = { +- [COREDUMP_STORAGE_NONE] = "none", ++ [COREDUMP_STORAGE_NONE] = "none", + [COREDUMP_STORAGE_EXTERNAL] = "external", +- [COREDUMP_STORAGE_JOURNAL] = "journal", ++ [COREDUMP_STORAGE_JOURNAL] = "journal", + }; + + DEFINE_PRIVATE_STRING_TABLE_LOOKUP(coredump_storage, CoredumpStorage); +@@ -156,13 +156,13 @@ static uint64_t arg_max_use = UINT64_MAX; + + static int parse_config(void) { + static const ConfigTableItem items[] = { +- { "Coredump", "Storage", config_parse_coredump_storage, 0, &arg_storage }, +- { "Coredump", "Compress", config_parse_bool, 0, &arg_compress }, +- { "Coredump", "ProcessSizeMax", config_parse_iec_uint64, 0, &arg_process_size_max }, +- { "Coredump", "ExternalSizeMax", config_parse_iec_uint64_infinity, 0, &arg_external_size_max }, +- { "Coredump", "JournalSizeMax", config_parse_iec_size, 0, &arg_journal_size_max }, +- { "Coredump", "KeepFree", config_parse_iec_uint64, 0, &arg_keep_free }, +- { "Coredump", "MaxUse", config_parse_iec_uint64, 0, &arg_max_use }, ++ { "Coredump", "Storage", config_parse_coredump_storage, 0, &arg_storage }, ++ { "Coredump", "Compress", config_parse_bool, 0, &arg_compress }, ++ { "Coredump", "ProcessSizeMax", config_parse_iec_uint64, 0, &arg_process_size_max }, ++ { "Coredump", "ExternalSizeMax", config_parse_iec_uint64_infinity, 0, &arg_external_size_max }, ++ { "Coredump", "JournalSizeMax", config_parse_iec_size, 0, &arg_journal_size_max }, ++ { "Coredump", "KeepFree", config_parse_iec_uint64, 0, &arg_keep_free }, ++ { "Coredump", "MaxUse", config_parse_iec_uint64, 0, &arg_max_use }, + {} + }; + +@@ -208,15 +208,15 @@ static int fix_acl(int fd, uid_t uid) { + static int fix_xattr(int fd, const Context *context) { + + static const char * const xattrs[_META_MAX] = { +- [META_ARGV_PID] = "user.coredump.pid", +- [META_ARGV_UID] = "user.coredump.uid", +- [META_ARGV_GID] = "user.coredump.gid", +- [META_ARGV_SIGNAL] = "user.coredump.signal", +- [META_ARGV_TIMESTAMP] = "user.coredump.timestamp", +- [META_ARGV_RLIMIT] = "user.coredump.rlimit", +- [META_ARGV_HOSTNAME] = "user.coredump.hostname", +- [META_COMM] = "user.coredump.comm", +- [META_EXE] = "user.coredump.exe", ++ [META_ARGV_PID] = "user.coredump.pid", ++ [META_ARGV_UID] = "user.coredump.uid", ++ [META_ARGV_GID] = "user.coredump.gid", ++ [META_ARGV_SIGNAL] = "user.coredump.signal", ++ [META_ARGV_TIMESTAMP] = "user.coredump.timestamp", ++ [META_ARGV_RLIMIT] = "user.coredump.rlimit", ++ [META_ARGV_HOSTNAME] = "user.coredump.hostname", ++ [META_COMM] = "user.coredump.comm", ++ [META_EXE] = "user.coredump.exe", + }; + + int r = 0; +-- +2.35.3 + diff --git a/5001-coredump-do-not-allow-user-to-access-coredumps-with-.patch b/5001-coredump-do-not-allow-user-to-access-coredumps-with-.patch new file mode 100644 index 00000000..aedc307c --- /dev/null +++ b/5001-coredump-do-not-allow-user-to-access-coredumps-with-.patch @@ -0,0 +1,385 @@ +From 87cad85ebec62e63893df46ff78becf82e984bee Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= +Date: Mon, 28 Nov 2022 12:12:55 +0100 +Subject: [PATCH 5001/5001] coredump: do not allow user to access coredumps + with changed uid/gid/capabilities + +When the user starts a program which elevates its permissions via setuid, +setgid, or capabilities set on the file, it may access additional information +which would then be visible in the coredump. We shouldn't make the the coredump +visible to the user in such cases. + +Reported-by: Matthias Gerstner + +This reads the /proc//auxv file and attaches it to the process metadata as +PROC_AUXV. Before the coredump is submitted, it is parsed and if either +at_secure was set (which the kernel will do for processes that are setuid, +setgid, or setcap), or if the effective uid/gid don't match uid/gid, the file +is not made accessible to the user. If we can't access this data, we assume the +file should not be made accessible either. In principle we could also access +the auxv data from a note in the core file, but that is much more complex and +it seems better to use the stand-alone file that is provided by the kernel. + +Attaching auxv is both convient for this patch (because this way it's passed +between the stages along with other fields), but I think it makes sense to save +it in general. + +We use the information early in the core file to figure out if the program was +32-bit or 64-bit and its endianness. This way we don't need heuristics to guess +whether the format of the auxv structure. This test might reject some cases on +fringe architecutes. But the impact would be limited: we just won't grant the +user permissions to view the coredump file. If people report that we're missing +some cases, we can always enhance this to support more architectures. + +I tested auxv parsing on amd64, 32-bit program on amd64, arm64, arm32, and +ppc64el, but not the whole coredump handling. + +[fbui: fixes bsc#1205000] +[fbui: fixes CVE-2022-4415] +--- + src/basic/io-util.h | 9 ++ + src/coredump/coredump.c | 196 +++++++++++++++++++++++++++++++++++++--- + 2 files changed, 192 insertions(+), 13 deletions(-) + +diff --git a/src/basic/io-util.h b/src/basic/io-util.h +index 39728e06bc..3afb134266 100644 +--- a/src/basic/io-util.h ++++ b/src/basic/io-util.h +@@ -91,7 +91,16 @@ struct iovec_wrapper *iovw_new(void); + struct iovec_wrapper *iovw_free(struct iovec_wrapper *iovw); + struct iovec_wrapper *iovw_free_free(struct iovec_wrapper *iovw); + void iovw_free_contents(struct iovec_wrapper *iovw, bool free_vectors); ++ + int iovw_put(struct iovec_wrapper *iovw, void *data, size_t len); ++static inline int iovw_consume(struct iovec_wrapper *iovw, void *data, size_t len) { ++ /* Move data into iovw or free on error */ ++ int r = iovw_put(iovw, data, len); ++ if (r < 0) ++ free(data); ++ return r; ++} ++ + int iovw_put_string_field(struct iovec_wrapper *iovw, const char *field, const char *value); + int iovw_put_string_field_free(struct iovec_wrapper *iovw, const char *field, char *value); + void iovw_rebase(struct iovec_wrapper *iovw, char *old, char *new); +diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c +index 7a181bdeeb..ea3d8c415a 100644 +--- a/src/coredump/coredump.c ++++ b/src/coredump/coredump.c +@@ -4,6 +4,7 @@ + #include + #include + #include ++#include + #include + #include + +@@ -106,6 +107,7 @@ enum { + + META_EXE = _META_MANDATORY_MAX, + META_UNIT, ++ META_PROC_AUXV, + _META_MAX + }; + +@@ -120,10 +122,12 @@ static const char * const meta_field_names[_META_MAX] = { + [META_COMM] = "COREDUMP_COMM=", + [META_EXE] = "COREDUMP_EXE=", + [META_UNIT] = "COREDUMP_UNIT=", ++ [META_PROC_AUXV] = "COREDUMP_PROC_AUXV=", + }; + + typedef struct Context { + const char *meta[_META_MAX]; ++ size_t meta_size[_META_MAX]; + pid_t pid; + bool is_pid1; + bool is_journald; +@@ -185,13 +189,16 @@ static uint64_t storage_size_max(void) { + return 0; + } + +-static int fix_acl(int fd, uid_t uid) { ++static int fix_acl(int fd, uid_t uid, bool allow_user) { ++ assert(fd >= 0); ++ assert(uid_is_valid(uid)); + + #if HAVE_ACL + int r; + +- assert(fd >= 0); +- assert(uid_is_valid(uid)); ++ /* We don't allow users to read coredumps if the uid or capabilities were changed. */ ++ if (!allow_user) ++ return 0; + + if (uid_is_system(uid) || uid_is_dynamic(uid) || uid == UID_NOBODY) + return 0; +@@ -251,7 +258,8 @@ static int fix_permissions( + const char *filename, + const char *target, + const Context *context, +- uid_t uid) { ++ uid_t uid, ++ bool allow_user) { + + int r; + +@@ -261,7 +269,7 @@ static int fix_permissions( + + /* Ignore errors on these */ + (void) fchmod(fd, 0640); +- (void) fix_acl(fd, uid); ++ (void) fix_acl(fd, uid, allow_user); + (void) fix_xattr(fd, context); + + r = fsync_full(fd); +@@ -331,6 +339,153 @@ static int make_filename(const Context *context, char **ret) { + return 0; + } + ++static int parse_auxv64( ++ const uint64_t *auxv, ++ size_t size_bytes, ++ int *at_secure, ++ uid_t *uid, ++ uid_t *euid, ++ gid_t *gid, ++ gid_t *egid) { ++ ++ assert(auxv || size_bytes == 0); ++ ++ if (size_bytes % (2 * sizeof(uint64_t)) != 0) ++ return log_warning_errno(SYNTHETIC_ERRNO(EIO), "Incomplete auxv structure (%zu bytes).", size_bytes); ++ ++ size_t words = size_bytes / sizeof(uint64_t); ++ ++ /* Note that we set output variables even on error. */ ++ ++ for (size_t i = 0; i + 1 < words; i += 2) ++ switch (auxv[i]) { ++ case AT_SECURE: ++ *at_secure = auxv[i + 1] != 0; ++ break; ++ case AT_UID: ++ *uid = auxv[i + 1]; ++ break; ++ case AT_EUID: ++ *euid = auxv[i + 1]; ++ break; ++ case AT_GID: ++ *gid = auxv[i + 1]; ++ break; ++ case AT_EGID: ++ *egid = auxv[i + 1]; ++ break; ++ case AT_NULL: ++ if (auxv[i + 1] != 0) ++ goto error; ++ return 0; ++ } ++ error: ++ return log_warning_errno(SYNTHETIC_ERRNO(ENODATA), ++ "AT_NULL terminator not found, cannot parse auxv structure."); ++} ++ ++static int parse_auxv32( ++ const uint32_t *auxv, ++ size_t size_bytes, ++ int *at_secure, ++ uid_t *uid, ++ uid_t *euid, ++ gid_t *gid, ++ gid_t *egid) { ++ ++ assert(auxv || size_bytes == 0); ++ ++ size_t words = size_bytes / sizeof(uint32_t); ++ ++ if (size_bytes % (2 * sizeof(uint32_t)) != 0) ++ return log_warning_errno(SYNTHETIC_ERRNO(EIO), "Incomplete auxv structure (%zu bytes).", size_bytes); ++ ++ /* Note that we set output variables even on error. */ ++ ++ for (size_t i = 0; i + 1 < words; i += 2) ++ switch (auxv[i]) { ++ case AT_SECURE: ++ *at_secure = auxv[i + 1] != 0; ++ break; ++ case AT_UID: ++ *uid = auxv[i + 1]; ++ break; ++ case AT_EUID: ++ *euid = auxv[i + 1]; ++ break; ++ case AT_GID: ++ *gid = auxv[i + 1]; ++ break; ++ case AT_EGID: ++ *egid = auxv[i + 1]; ++ break; ++ case AT_NULL: ++ if (auxv[i + 1] != 0) ++ goto error; ++ return 0; ++ } ++ error: ++ return log_warning_errno(SYNTHETIC_ERRNO(ENODATA), ++ "AT_NULL terminator not found, cannot parse auxv structure."); ++} ++ ++static int grant_user_access(int core_fd, const Context *context) { ++ int at_secure = -1; ++ uid_t uid = UID_INVALID, euid = UID_INVALID; ++ uid_t gid = GID_INVALID, egid = GID_INVALID; ++ int r; ++ ++ assert(core_fd >= 0); ++ assert(context); ++ ++ if (!context->meta[META_PROC_AUXV]) ++ return log_warning_errno(SYNTHETIC_ERRNO(ENODATA), "No auxv data, not adjusting permissions."); ++ ++ uint8_t elf[EI_NIDENT]; ++ errno = 0; ++ if (pread(core_fd, &elf, sizeof(elf), 0) != sizeof(elf)) ++ return log_warning_errno(errno_or_else(EIO), ++ "Failed to pread from coredump fd: %s", STRERROR_OR_EOF(errno)); ++ ++ if (elf[EI_MAG0] != ELFMAG0 || ++ elf[EI_MAG1] != ELFMAG1 || ++ elf[EI_MAG2] != ELFMAG2 || ++ elf[EI_MAG3] != ELFMAG3 || ++ elf[EI_VERSION] != EV_CURRENT) ++ return log_info_errno(SYNTHETIC_ERRNO(EUCLEAN), ++ "Core file does not have ELF header, not adjusting permissions."); ++ if (!IN_SET(elf[EI_CLASS], ELFCLASS32, ELFCLASS64) || ++ !IN_SET(elf[EI_DATA], ELFDATA2LSB, ELFDATA2MSB)) ++ return log_info_errno(SYNTHETIC_ERRNO(EUCLEAN), ++ "Core file has strange ELF class, not adjusting permissions."); ++ ++ if ((elf[EI_DATA] == ELFDATA2LSB) != (__BYTE_ORDER == __LITTLE_ENDIAN)) ++ return log_info_errno(SYNTHETIC_ERRNO(EUCLEAN), ++ "Core file has non-native endianness, not adjusting permissions."); ++ ++ if (elf[EI_CLASS] == ELFCLASS64) ++ r = parse_auxv64((const uint64_t*) context->meta[META_PROC_AUXV], ++ context->meta_size[META_PROC_AUXV], ++ &at_secure, &uid, &euid, &gid, &egid); ++ else ++ r = parse_auxv32((const uint32_t*) context->meta[META_PROC_AUXV], ++ context->meta_size[META_PROC_AUXV], ++ &at_secure, &uid, &euid, &gid, &egid); ++ 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. */ ++ bool ret = ++ 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)", ++ ret ? "permit" : "restrict", ++ uid, euid, gid, egid, yes_no(at_secure)); ++ return ret; ++} ++ + static int save_external_coredump( + const Context *context, + int input_fd, +@@ -453,6 +608,8 @@ static int save_external_coredump( + context->meta[META_ARGV_PID], context->meta[META_COMM]); + truncated = r == 1; + ++ bool allow_user = grant_user_access(fd, context) > 0; ++ + #if HAVE_COMPRESSION + if (arg_compress) { + _cleanup_(unlink_and_freep) char *tmp_compressed = NULL; +@@ -490,7 +647,7 @@ static int save_external_coredump( + uncompressed_size += partial_uncompressed_size; + } + +- r = fix_permissions(fd_compressed, tmp_compressed, fn_compressed, context, uid); ++ r = fix_permissions(fd_compressed, tmp_compressed, fn_compressed, context, uid, allow_user); + if (r < 0) + return r; + +@@ -517,7 +674,7 @@ static int save_external_coredump( + "SIZE_LIMIT=%"PRIu64, max_size, + "MESSAGE_ID=" SD_MESSAGE_TRUNCATED_CORE_STR); + +- r = fix_permissions(fd, tmp, fn, context, uid); ++ r = fix_permissions(fd, tmp, fn, context, uid, allow_user); + if (r < 0) + return log_error_errno(r, "Failed to fix permissions and finalize coredump %s into %s: %m", coredump_tmpfile_name(tmp), fn); + +@@ -765,7 +922,7 @@ static int change_uid_gid(const Context *context) { + } + + static int submit_coredump( +- Context *context, ++ const Context *context, + struct iovec_wrapper *iovw, + int input_fd) { + +@@ -944,16 +1101,15 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { + struct iovec *iovec = iovw->iovec + n; + + for (size_t i = 0; i < ELEMENTSOF(meta_field_names); i++) { +- char *p; +- + /* Note that these strings are NUL terminated, because we made sure that a + * trailing NUL byte is in the buffer, though not included in the iov_len + * count (see process_socket() and gather_pid_metadata_*()) */ + assert(((char*) iovec->iov_base)[iovec->iov_len] == 0); + +- p = startswith(iovec->iov_base, meta_field_names[i]); ++ const char *p = startswith(iovec->iov_base, meta_field_names[i]); + if (p) { + context->meta[i] = p; ++ context->meta_size[i] = iovec->iov_len - strlen(meta_field_names[i]); + break; + } + } +@@ -1190,6 +1346,7 @@ static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { + uid_t owner_uid; + pid_t pid; + char *t; ++ size_t size; + const char *p; + int r; + +@@ -1254,13 +1411,26 @@ static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { + (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_LIMITS=", t); + + p = procfs_file_alloca(pid, "cgroup"); +- if (read_full_virtual_file(p, &t, NULL) >=0) ++ if (read_full_virtual_file(p, &t, NULL) >= 0) + (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_CGROUP=", t); + + p = procfs_file_alloca(pid, "mountinfo"); +- if (read_full_virtual_file(p, &t, NULL) >=0) ++ if (read_full_virtual_file(p, &t, NULL) >= 0) + (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_MOUNTINFO=", t); + ++ /* We attach /proc/auxv here. ELF coredumps also contain a note for this (NT_AUXV), see elf(5). */ ++ p = procfs_file_alloca(pid, "auxv"); ++ if (read_full_virtual_file(p, &t, &size) >= 0) { ++ char *buf = malloc(strlen("COREDUMP_PROC_AUXV=") + size + 1); ++ if (buf) { ++ /* Add a dummy terminator to make save_context() happy. */ ++ *((uint8_t*) mempcpy(stpcpy(buf, "COREDUMP_PROC_AUXV="), t, size)) = '\0'; ++ (void) iovw_consume(iovw, buf, size + strlen("COREDUMP_PROC_AUXV=")); ++ } ++ ++ free(t); ++ } ++ + if (get_process_cwd(pid, &t) >= 0) + (void) iovw_put_string_field_free(iovw, "COREDUMP_CWD=", t); + +-- +2.35.3 + diff --git a/systemd.changes b/systemd.changes index c887dc75..7d06424f 100644 --- a/systemd.changes +++ b/systemd.changes @@ -1,3 +1,12 @@ +------------------------------------------------------------------- +Mon Dec 19 07:50:15 UTC 2022 - Franck Bui + +- Fix systemd-coredump to not allow user to access coredumps with changed + uid/gid/capabilities (bsc#1205000 CVE-2022-4415) + + Add 5000-coredump-adjust-whitespace.patch + Add 5001-coredump-do-not-allow-user-to-access-coredumps-with-.patch + ------------------------------------------------------------------- Fri Dec 9 08:28:50 UTC 2022 - Franck Bui diff --git a/systemd.spec b/systemd.spec index ad7989d5..4037f598 100644 --- a/systemd.spec +++ b/systemd.spec @@ -216,6 +216,8 @@ Patch1000: 1000-Revert-getty-Pass-tty-to-use-by-agetty-via-stdin.patch # very few cases, some stuff might be broken in upstream and need to be fixed # quickly. But even in these cases, the patches are temporary and should be # removed as soon as a fix is merged by upstream. +Patch5000: 5000-coredump-adjust-whitespace.patch +Patch5001: 5001-coredump-do-not-allow-user-to-access-coredumps-with-.patch %description Systemd is a system and service manager, compatible with SysV and LSB