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 commit cd5b72a538a404011d27d86ff958355ac2c45b8d Author: Frank Ch. Eigler Date: Sun Jan 24 14:45:54 2021 -0500 PR27067: set procfs traceNN files' uid/gid too commit e3d03db828 neglected to include the proper calls to set the procfs traceNN files to the correct uid/gid ownership. With those files left as uid/gid=0/0, stapio running with a user with stapusr/stapdev privileges couldn't fopenat() those files. Now they can again. This problem became obvious after commit 4706ab3ca5c0, which makes STAP_TRANS_PROCFS the default. diff --git a/runtime/transport/procfs.c b/runtime/transport/procfs.c index 97a6e123a..69591a235 100644 --- a/runtime/transport/procfs.c +++ b/runtime/transport/procfs.c @@ -336,12 +336,14 @@ __stp_procfs_relay_create_buf_file_callback(const char *filename, if (parent != _stp_procfs_module_dir_path.dentry) goto out; - pde = proc_create (filename, 0600, + pde = proc_create (filename, 0400, _stp_procfs_module_dir, & relay_procfs_operations); if (pde == NULL) goto out; + proc_set_user(pde, KUIDT_INIT(_stp_uid), KGIDT_INIT(_stp_gid)); + rc = snprintf(fullpath, sizeof(fullpath), "/proc/systemtap/%s/%s", THIS_MODULE->name, filename);