diff --git a/SOURCES/rhbz1902696.patch b/SOURCES/rhbz1902696.patch new file mode 100644 index 0000000..dfe2812 --- /dev/null +++ b/SOURCES/rhbz1902696.patch @@ -0,0 +1,184 @@ +commit e3d03db82853049f65f16dc40c03f3f7f617ffb5 +Author: Frank Ch. Eigler +Date: Sun Dec 13 21:05:23 2020 -0500 + + PR23512: fix staprun/stapio operation via less-than-root privileges + + Commit 7615cae790c899bc8a82841c75c8ea9c6fa54df3 for PR26665 introduced + a regression in handling stapusr/stapdev/stapsys gid invocation of + staprun/stapio. This patch simplifies the relevant code in + staprun/ctl.c, init_ctl_channel(), to rely on openat/etc. to populate + and use the relay_basedir_fd as much as possible. Also, we now avoid + unnecessary use of access(), which was checking against the wrong + (real rather than effective) uid/gid. + +diff --git a/staprun/ctl.c b/staprun/ctl.c +index 4be68af..da3417b 100644 +--- a/staprun/ctl.c ++++ b/staprun/ctl.c +@@ -14,111 +14,70 @@ + + #define CTL_CHANNEL_NAME ".cmd" + ++ ++#ifndef HAVE_OPENAT ++#error "need openat" ++#endif ++ ++ ++// This function does multiple things: ++// ++// 1) if needed, open the running module's directory (the one that ++// contains .ctl), stash fd in relay_basedir_fd; this will be ++// passed to stapio children via -F$fd for privilege passing ++// ++// 2) (re)open the running module's .ctl file, stash fd in the ++// control_channel global; this will be used all over the place. ++// ++// Return 0 on success. ++// ++// See also PR14245, PR26665, RHBZ1902696 = PR23512 ++// + int init_ctl_channel(const char *name, int verb) + { +- char buf[PATH_MAX] = ""; // the .ctl file name +- char buf2[PATH_MAX] = ""; // other tmp stuff +- struct statfs st; +- + (void) verb; +- if (0) goto out; /* just to defeat gcc warnings */ + +- /* Before trying to open the control channel, make sure it +- * isn't already open. */ +- close_ctl_channel(); ++ // Already got them both? ++ if (control_channel >= 0 && relay_basedir_fd >= 0) ++ return 0; + +-#ifdef HAVE_OPENAT +- if (relay_basedir_fd >= 0) { +- strncpy(buf, CTL_CHANNEL_NAME, PATH_MAX - 1); +- control_channel = openat_cloexec(relay_basedir_fd, +- CTL_CHANNEL_NAME, O_RDWR, 0); +- dbug(2, "Opened %s (%d)\n", CTL_CHANNEL_NAME, control_channel); ++ // Need relay_basedir_fd .... ok try /sys/kernel/debug/systemtap/ ++ if (relay_basedir_fd < 0) { ++ char buf[PATH_MAX] = ""; ++ struct statfs st; + +- /* NB: Extra real-id access check as below */ +- if (faccessat(relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) != 0){ +- close(control_channel); +- return -5; +- } +- if (control_channel >= 0) +- goto out; /* It's OK to bypass the [f]access[at] check below, +- since this would only occur the *second* time +- staprun tries this gig, or within unprivileged stapio. */ ++ if (sprintf_chk(buf, "/sys/kernel/debug/systemtap/%s", name)) ++ return -EINVAL; ++ ++ if (statfs("/sys/kernel/debug", &st) == 0 && (int)st.f_type == (int)DEBUGFS_MAGIC) ++ relay_basedir_fd = open (buf, O_DIRECTORY | O_RDONLY); + } +- /* PR14245, NB: we fall through to /sys ... /proc searching, +- in case the relay_basedir_fd option wasn't given (i.e., for +- early in staprun), or if errors out for some reason. */ +-#endif +- + +- // See if we have the .ctl file in debugfs +- if (sprintf_chk(buf2, "/sys/kernel/debug/systemtap/%s/%s", +- name, CTL_CHANNEL_NAME)) +- return -1; +- if (statfs("/sys/kernel/debug", &st) == 0 && (int)st.f_type == (int)DEBUGFS_MAGIC && +- (access (buf2, W_OK)==0)) { +- /* PR14245: allow subsequent operations, and if +- necessary, staprun->stapio forks, to reuse an fd for +- directory lookups (even if some parent directories have +- perms 0700. */ +- strcpy(buf, buf2); // committed ++ // Still need relay_basedir_fd ... ok try /proc/systemtap/ ++ if (relay_basedir_fd < 0) { ++ char buf[PATH_MAX] = ""; + +-#ifdef HAVE_OPENAT +- if (! sprintf_chk(buf2, "/sys/kernel/debug/systemtap/%s", name)) { +- relay_basedir_fd = open (buf2, O_DIRECTORY | O_RDONLY); +- } +-#endif +- } +- +- // PR26665: try /proc/systemtap/... also +- // (STP_TRANSPORT_1 used to use this for other purposes.) +- if (sprintf_chk(buf2, "/proc/systemtap/%s/%s", +- name, CTL_CHANNEL_NAME)) +- return -1; +- if (relay_basedir_fd < 0 && (access(buf2, W_OK)==0)) { +- strcpy(buf, buf2); // committed ++ if (sprintf_chk(buf, "/proc/systemtap/%s", name)) ++ return -EINVAL; + +-#ifdef HAVE_OPENAT +- if (! sprintf_chk(buf2, "/proc/systemtap/%s", name)) { +- relay_basedir_fd = open (buf2, O_DIRECTORY | O_RDONLY); +- } +-#endif ++ relay_basedir_fd = open (buf, O_DIRECTORY | O_RDONLY); + } + +- /* At this point, we have buf, which is the full path to the .ctl file, +- and we may have a relay_basedir_fd, which is useful to pass across +- staprun->stapio fork/execs. */ +- +- control_channel = open_cloexec(buf, O_RDWR, 0); +- dbug(2, "Opened %s (%d)\n", buf, control_channel); +- +- /* NB: Even if open() succeeded with effective-UID permissions, we +- * need the access() check to make sure real-UID permissions are also +- * sufficient. When we run under the setuid staprun, effective and +- * real UID may not be the same. Specifically, we want to prevent +- * a local stapusr from trying to attach to a different stapusr's module. +- * +- * The access() is done *after* open() to avoid any TOCTOU-style race +- * condition. We believe it's probably safe either way, as the file +- * we're trying to access connot be modified by a typical user, but +- * better safe than sorry. +- */ +-#ifdef HAVE_OPENAT +- if (control_channel >= 0 && relay_basedir_fd >= 0) { +- if (faccessat (relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) == 0) +- goto out; +- /* else fall through */ ++ // Got relay_basedir_fd, need .ctl ++ if (relay_basedir_fd >= 0) { ++ // verify that the ctl file is accessible to our real uid/gid ++ if (faccessat(relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) != 0) ++ return -EPERM; ++ ++ control_channel = openat_cloexec(relay_basedir_fd, ++ CTL_CHANNEL_NAME, O_RDWR, 0); + } +-#endif +- if (control_channel >= 0 && access(buf, R_OK|W_OK) != 0) { +- close(control_channel); +- return -5; +- } + +-out: +- if (control_channel < 0) { ++ // Fell through ++ if (relay_basedir_fd < 0 || control_channel < 0) { + err(_("Cannot attach to module %s control channel; not running?\n"), + name); +- return -3; ++ return -EINVAL; + } + return 0; + } + +commit 1120422c2822be9e00d8d11cab3fb381d2ce0cce +Author: Frank Ch. Eigler +Date: Sun Dec 13 21:19:15 2020 -0500 + + PR27067 <<< corrected bug# for previous commit diff --git a/SPECS/systemtap.spec b/SPECS/systemtap.spec index f010abb..be724b7 100644 --- a/SPECS/systemtap.spec +++ b/SPECS/systemtap.spec @@ -89,7 +89,7 @@ Name: systemtap Version: 4.4 -Release: 3%{?release_override}%{?dist} +Release: 4%{?release_override}%{?dist} # for version, see also configure.ac @@ -127,6 +127,7 @@ Source: ftp://sourceware.org/pub/systemtap/releases/systemtap-%{version}.tar.gz Patch1: rhbz1873492.patch Patch2: rhbz1898288.patch +Patch3: rhbz1902696.patch # Build* BuildRequires: gcc-c++ @@ -515,6 +516,7 @@ systemtap-runtime-virthost machine to execute systemtap scripts. %setup -q %patch1 -p1 %patch2 -p1 +%patch3 -p1 %build @@ -1234,6 +1236,9 @@ done # PRERELEASE %changelog +* Tue Dec 15 2020 Frank Ch. Eigler - 4.4-4 +- rhbz1902696 fix invocation as stapusr vs. root + * Tue Nov 17 2020 Frank Ch. Eigler - 4.4-3 - rhbz1873492 related: rhel8 kernel_is_locked_down detection