From 34730da103be8362b2a11d8ff92e4f9f9c32856e Mon Sep 17 00:00:00 2001
From: Jaco Kroon <jaco@uls.co.za>
Date: Wed, 9 Jul 2025 13:35:58 +0200
Subject: [PATCH] Stricter checks on scripts.

Perform symbolic link path resolution before performing checks (avoid
symlink change attacks).

Ensure the path leading to the executable is root:* owned, and not
writable by anyone else.

Ensure the executable itself is:

Owned by root.
Executable for root.
Not writable by group or other.

Doesn't look like we need to check FACLs (the bits for additional users
are mixed into group permission bits according to my testing).

Signed-off-by: Jaco Kroon <jaco@uls.co.za>
---
 pppd/main.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/pppd/main.c b/pppd/main.c
index 65dac0e2..6eee91e9 100644
--- a/pppd/main.c
+++ b/pppd/main.c
@@ -1914,10 +1914,52 @@ update_script_environment(void)
  * reap_kids) iff the return value is > 0.
  */
 pid_t
-run_program(char *prog, char * const *args, int must_exist, void (*done)(void *), void *arg, int wait)
+run_program(char *_prog, char * const *args, int must_exist, void (*done)(void *), void *arg, int wait)
 {
     int pid, status, ret;
     struct stat sbuf;
+    char prog[PATH_MAX];
+    char *slash;
+
+    /*
+     * Resolve symlinks such that we eliminate a few potential ToCToU
+     * attack avenues (eg, changing symlinks).
+     */
+    if (!realpath(_prog, prog)) {
+	if (must_exist || errno != ENOENT)
+	    warn("Can't execute %s: %m", prog);
+	return 0;
+    }
+
+    /*
+     * full check the entire path, must be root: owned, and NOT be writable
+     * to group/other (which incorporates FACL bits somehow, so we can ignore
+     * explicit FACL checks).
+     */
+    do {
+	bool ok = false;
+	if (fstatat(AT_FDCWD, slash ? prog : "/", &sbuf, AT_SYMLINK_NOFOLLOW) != 0) {
+	    warn("Script path check failure: %s: %m", slash ? prog : "/");
+	} else if (sbuf.st_uid != 0 /* getuid() ? */) {
+	    warn("Script path %s is not root owned.", slash ? prog : "/");
+	} else if (0 != (sbuf.st_mode & (S_IWGRP | S_IWOTH))) {
+	    warn("Script path %s is group or other writable.", slash ? prog : "/");
+	} else {
+	    ok = true;
+	}
+
+	if (slash)
+	    *slash = '/';
+
+	if (!ok) {
+	    warn("Can't execute %s: error checking path as per above");
+	    return 0;
+	}
+
+	slash = strchr((slash ? slash : prog) + 1, '/');
+	if (slash)
+	    *slash = 0;
+    } while (slash);
 
     /*
      * First check if the file exists and is executable.
@@ -1926,13 +1968,27 @@ run_program(char *prog, char * const *args, int must_exist, void (*done)(void *)
      * might be accessible only to root.
      */
     errno = EINVAL;
-    if (stat(prog, &sbuf) < 0 || !S_ISREG(sbuf.st_mode)
-	|| (sbuf.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) == 0) {
+    if (fstatat(AT_FDCWD, prog, &sbuf, AT_SYMLINK_NOFOLLOW) != 0) {
 	if (must_exist || errno != ENOENT)
 	    warn("Can't execute %s: %m", prog);
 	return 0;
     }
 
+    if (sbuf.st_uid != 0 /* getuid() ? */) {
+	warn("Can't execute %s: Not root owned.", prog);
+	return 0;
+    }
+
+    if (0 != (sbuf.st_mode & (S_IWGRP | S_IWOTH))) {
+	warn("Can't execute %s: Writable by group or other.", prog);
+	return 0;
+    }
+
+    if (!S_ISREG(sbuf.st_mode) || (sbuf.st_mode & (S_IXUSR)) == 0) {
+	warn("Can't execute %s: Not a regular executable (for root) file.");
+	return 0;
+    }
+
     pid = ppp_safe_fork(fd_devnull, fd_devnull, fd_devnull);
     if (pid == -1) {
 	error("Failed to create child process for %s: %m", prog);