diff options
Diffstat (limited to 'debian/patches/coredump-do-not-allow-user-to-access-coredumps-with-chang.patch')
| -rw-r--r-- | debian/patches/coredump-do-not-allow-user-to-access-coredumps-with-chang.patch | 388 |
1 files changed, 388 insertions, 0 deletions
diff --git a/debian/patches/coredump-do-not-allow-user-to-access-coredumps-with-chang.patch b/debian/patches/coredump-do-not-allow-user-to-access-coredumps-with-chang.patch new file mode 100644 index 0000000..f1029d3 --- /dev/null +++ b/debian/patches/coredump-do-not-allow-user-to-access-coredumps-with-chang.patch @@ -0,0 +1,388 @@ +From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> +Date: Mon, 28 Nov 2022 12:12:55 +0100 +Subject: 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 <mgerstner@suse.de> + +This reads the /proc/<pid>/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. + +(cherry picked from commit 3e4d0f6cf99f8677edd6a237382a65bfe758de03) +(cherry picked from commit 9b75a3d0502d6741c8ecb7175794345f8eb3827c) +(cherry picked from commit efca5283dc791a07171f80eef84e14fdb58fad57) +(cherry picked from commit 1d5e0e9910500f3c3584485f77bfc35e601036e3) +(cherry picked from commit 8215e1527d859e77dd1378fd7e42bbd32130edb3) +(cherry picked from commit 786df410b1cb3a2294c9a5d118c958525e7439e6) +--- + src/basic/io-util.h | 9 +++ + src/coredump/coredump.c | 200 ++++++++++++++++++++++++++++++++++++++++++++---- + 2 files changed, 194 insertions(+), 15 deletions(-) + +diff --git a/src/basic/io-util.h b/src/basic/io-util.h +index d817714..dacec71 100644 +--- a/src/basic/io-util.h ++++ b/src/basic/io-util.h +@@ -85,7 +85,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 0a1cb91..b60dff3 100644 +--- a/src/coredump/coredump.c ++++ b/src/coredump/coredump.c +@@ -3,6 +3,7 @@ + #include <errno.h> + #include <stdio.h> + #include <sys/prctl.h> ++#include <sys/auxv.h> + #include <sys/xattr.h> + #include <unistd.h> + +@@ -96,6 +97,7 @@ enum { + + META_EXE = _META_MANDATORY_MAX, + META_UNIT, ++ META_PROC_AUXV, + _META_MAX + }; + +@@ -110,10 +112,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; +@@ -175,14 +179,17 @@ static uint64_t storage_size_max(void) { + return 0; + } + +-static int fix_acl(int fd, uid_t uid) { +- +-#if HAVE_ACL +- int r; +- ++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; ++ ++ /* 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; + +@@ -242,7 +249,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; + +@@ -252,7 +260,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); + + if (fsync(fd) < 0) +@@ -323,6 +331,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", errno != 0 ? strerror_safe(errno) : "Unexpected EOF"); ++ ++ 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, +@@ -403,6 +558,8 @@ static int save_external_coredump( + goto fail; + } + ++ bool allow_user = grant_user_access(fd, context) > 0; ++ + #if HAVE_COMPRESSION + /* If we will remove the coredump anyway, do not compress. */ + if (arg_compress && !maybe_remove_external_coredump(NULL, st.st_size)) { +@@ -428,7 +585,7 @@ static int save_external_coredump( + goto fail_compressed; + } + +- 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) + goto fail_compressed; + +@@ -451,7 +608,7 @@ static int save_external_coredump( + uncompressed: + #endif + +- r = fix_permissions(fd, tmp, fn, context, uid); ++ r = fix_permissions(fd, tmp, fn, context, uid, allow_user); + if (r < 0) + goto fail; + +@@ -700,7 +857,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) { + +@@ -822,16 +979,15 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { + struct iovec *iovec = iovw->iovec + n; + + for (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]); + count++; + break; + } +@@ -1074,6 +1230,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; + +@@ -1139,13 +1296,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_file(p, &t, NULL) >=0) ++ if (read_full_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_file(p, &t, NULL) >=0) ++ if (read_full_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); + |