Blob Blame History Raw
commit e3d03db82853049f65f16dc40c03f3f7f617ffb5
Author: Frank Ch. Eigler <fche@redhat.com>
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 <fche@redhat.com>
Date:   Sun Dec 13 21:19:15 2020 -0500

    PR27067 <<< corrected bug# for previous commit