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