From 43ac4469c1d51db2e5e587264e9fa6f438cb4e5c Mon Sep 17 00:00:00 2001 From: Qingfang Deng Date: Fri, 29 May 2026 09:55:08 +0800 Subject: [PATCH] pppd: fix TOCTOU in protected file access The proper way to fix TOCTOU is to avoid multiple evaluations of a path. Eliminate the check-then-open antipattern when opening files. Instead, open files first and validate the resulting file descriptor, and run scripts through fexecve(). This fixes the TOCTOU window, and avoids overly strict directory ownership checks that could break existing configurations. Signed-off-by: Qingfang Deng --- configure.ac | 1 + pppd/auth.c | 146 +++++++++++++++++++++++++------------------- pppd/main.c | 40 ++++++++---- pppd/options.c | 56 +++++++++++++---- pppd/pppd-private.h | 2 +- pppd/pppd.h | 2 +- pppd/utils.c | 84 ++++++------------------- 7 files changed, 177 insertions(+), 154 deletions(-) diff --git a/configure.ac b/configure.ac index 69d4bd24..2427fe69 100644 --- a/configure.ac +++ b/configure.ac @@ -93,6 +93,7 @@ AC_CHECK_SIZEOF(unsigned short) # Checks for library functions. AC_CHECK_FUNCS([ \ + fexecve \ mmap \ logwtmp \ strerror]) diff --git a/pppd/auth.c b/pppd/auth.c index 38ddcc84..91c3fe0e 100644 --- a/pppd/auth.c +++ b/pppd/auth.c @@ -307,7 +307,7 @@ static int setupapfile (char **); static int privgroup (char **); static int set_noauth_addr (char **); static int set_permitted_number (char **); -static void check_access (FILE *, char *); +static void check_access (int, const char *); static int wordlist_count (struct wordlist *); static void check_maxoctets (void *); @@ -532,7 +532,7 @@ setupapfile(char **argv) free(fname); return 0; } - check_access(ufile, fname); + check_access(fileno(ufile), fname); uafname = fname; /* get username */ @@ -1543,8 +1543,7 @@ check_passwd(int unit, * Open the file of pap secrets and scan for a suitable secret * for authenticating this user. */ - if (!ppp_check_access(path_upapfile, &filename, 0, 0)) - return UPAP_AUTHNAK; + filename = path_upapfile; addrs = opts = NULL; ret = UPAP_AUTHNAK; f = fopen(filename, "r"); @@ -1552,7 +1551,13 @@ check_passwd(int unit, error("Can't open PAP password file %s: %m", filename); } else { - check_access(f, filename); + int fd = fileno(f); + + if (!ppp_check_access(fd, filename, 0)) { + fclose(f); + return UPAP_AUTHNAK; + } + check_access(fd, filename); if (scan_authfile(f, user, our_name, secret, &addrs, &opts, filename, 0) < 0) { warn("no PAP secret found for %s", user); } else { @@ -1586,7 +1591,6 @@ check_passwd(int unit, } fclose(f); } - free(filename); if (ret == UPAP_AUTHNAK) { if (**msg == 0) @@ -1646,21 +1650,24 @@ null_login(int unit) * Open the file of pap secrets and scan for a suitable secret. */ if (ret <= 0) { - if (!ppp_check_access(path_upapfile, &filename, 0, 0)) - return 0; + int fd; + + filename = path_upapfile; addrs = NULL; f = fopen(filename, "r"); - if (f == NULL) { - free(filename); + if (f == NULL) + return 0; + fd = fileno(f); + if (!ppp_check_access(fd, filename, 0)) { + fclose(f); return 0; } - check_access(f, filename); + check_access(fd, filename); i = scan_authfile(f, "", our_name, secret, &addrs, &opts, filename, 0); ret = i >= 0 && secret[0] == 0; BZERO(secret, sizeof(secret)); fclose(f); - free(filename); } if (ret) @@ -1701,7 +1708,7 @@ get_pap_passwd(char *passwd) f = fopen(filename, "r"); if (f == NULL) return 0; - check_access(f, filename); + check_access(fileno(f), filename); ret = scan_authfile(f, user, (remote_name[0]? remote_name: NULL), secret, NULL, NULL, filename, 0); @@ -1734,18 +1741,18 @@ have_pap_secret(int *lacks_ipp) return ret; } - if (!ppp_check_access(path_upapfile, &filename, 0, 0)) - return 0; + filename = path_upapfile; f = fopen(filename, "r"); - if (f == NULL) { - free(filename); + if (f == NULL) return 0; - } + if (!ppp_check_access(fileno(f), filename, 0)) { + fclose(f); + return 0; + } ret = scan_authfile(f, (explicit_remote? remote_name: NULL), our_name, NULL, &addrs, NULL, filename, 0); fclose(f); - free(filename); if (ret >= 0 && !some_ip_ok(addrs)) { if (lacks_ipp != 0) *lacks_ipp = 1; @@ -1780,11 +1787,13 @@ have_chap_secret(char *client, char *server, } } - if (!ppp_check_access(path_chapfile, &filename, 0, 0)) - return 0; + filename = path_chapfile; f = fopen(filename, "r"); - if (f == NULL) { - free(filename); + if (f == NULL) + return 0; + + if (!ppp_check_access(fileno(f), filename, 0)) { + fclose(f); return 0; } @@ -1795,7 +1804,6 @@ have_chap_secret(char *client, char *server, ret = scan_authfile(f, client, server, NULL, &addrs, NULL, filename, 0); fclose(f); - free(filename); if (ret >= 0 && need_ip && !some_ip_ok(addrs)) { if (lacks_ipp != 0) *lacks_ipp = 1; @@ -1822,11 +1830,13 @@ have_srp_secret(char *client, char *server, int need_ip, int *lacks_ipp) char *filename; struct wordlist *addrs; - if (!ppp_check_access(PPP_PATH_SRPFILE, &filename, 0, 0)) - return 0; + filename = PPP_PATH_SRPFILE; f = fopen(filename, "r"); - if (f == NULL) { - free(filename); + if (f == NULL) + return 0; + + if (!ppp_check_access(fileno(f), filename, 0)) { + fclose(f); return 0; } @@ -1837,7 +1847,6 @@ have_srp_secret(char *client, char *server, int need_ip, int *lacks_ipp) ret = scan_authfile(f, client, server, NULL, &addrs, NULL, filename, 0); fclose(f); - free(filename); if (ret >= 0 && need_ip && !some_ip_ok(addrs)) { if (lacks_ipp != 0) *lacks_ipp = 1; @@ -1874,22 +1883,27 @@ get_secret(int unit, char *client, char *server, return 0; } } else { - if (!ppp_check_access(path_chapfile, &filename, 0, 0)) - return 0; + int fd; + + filename = path_chapfile; addrs = NULL; secbuf[0] = 0; f = fopen(filename, "r"); if (f == NULL) { error("Can't open chap secret file %s: %m", filename); - free(filename); return 0; } - check_access(f, filename); + + fd = fileno(f); + if (!ppp_check_access(fd, filename, 0)) { + fclose(f); + return 0; + } + check_access(fd, filename); ret = scan_authfile(f, client, server, secbuf, &addrs, &opts, filename, 0); fclose(f); - free(filename); if (ret < 0) return 0; @@ -1931,23 +1945,27 @@ get_srp_secret(int unit, char *client, char *server, if (!am_server && passwd[0] != '\0') { strlcpy(secret, passwd, MAXWORDLEN); } else { - if (!ppp_check_access(PPP_PATH_SRPFILE, &filename, 0, 0)) - return 0; - addrs = NULL; + int fd; + filename = PPP_PATH_SRPFILE; + addrs = NULL; fp = fopen(filename, "r"); if (fp == NULL) { error("Can't open srp secret file %s: %m", filename); - free(filename); return 0; } - check_access(fp, filename); + + fd = fileno(fp); + if (!ppp_check_access(fd, filename, 0)) { + fclose(fp); + return 0; + } + check_access(fd, filename); secret[0] = '\0'; ret = scan_authfile(fp, client, server, secret, &addrs, &opts, filename, am_server); fclose(fp); - free(filename); if (ret < 0) return 0; @@ -2214,11 +2232,11 @@ auth_number(void) * check_access - complain if a secret file has too-liberal permissions. */ static void -check_access(FILE *f, char *filename) +check_access(int fd, const char *filename) { struct stat sbuf; - if (fstat(fileno(f), &sbuf) < 0) { + if (fstat(fd, &sbuf) < 0) { warn("cannot stat secret file %s: %m", filename); } else if ((sbuf.st_mode & (S_IRWXG | S_IRWXO)) != 0) { warn("Warning - secret file %s has world and/or group access", @@ -2326,25 +2344,25 @@ scan_authfile(FILE *f, char *client, char *server, * Special syntax: @/pathname means read secret from file. */ if (word[0] == '@' && word[1] == '/') { - char *realname; + int fd; strlcpy(atfile, word+1, sizeof(atfile)); - if (!ppp_check_access(atfile, &realname, 0, 0)) - continue; - if ((sf = fopen(realname, "r")) == NULL) { + if ((sf = fopen(atfile, "r")) == NULL) { warn("can't open indirect secret file %s", atfile); - free(realname); continue; } - check_access(sf, atfile); + fd = fileno(sf); + if (!ppp_check_access(fd, atfile, 0)) { + fclose(sf); + continue; + } + check_access(fd, atfile); if (!getword(sf, word, &xxx, atfile)) { warn("no secret in indirect secret file %s", atfile); fclose(sf); - free(realname); continue; } fclose(sf); - free(realname); } strlcpy(lsecret, word, sizeof(lsecret)); } @@ -2503,14 +2521,15 @@ have_eaptls_secret_server(char *client, char *server, char cacertfile[MAXWORDLEN]; char pkfile[MAXWORDLEN]; - if (!ppp_check_access(PPP_PATH_EAPTLSSERVFILE, &filename, 0, 0)) - return 0; + filename = PPP_PATH_EAPTLSSERVFILE; f = fopen(filename, "r"); - if (f == NULL) { - free(filename); + if (f == NULL) return 0; - } + if (!ppp_check_access(fileno(f), filename, 0)) { + fclose(f); + return 0; + } if (client != NULL && client[0] == 0) client = NULL; else if (server != NULL && server[0] == 0) @@ -2522,7 +2541,6 @@ have_eaptls_secret_server(char *client, char *server, 0); fclose(f); - free(filename); /* if (ret >= 0 && !eaptls_init_ssl(1, cacertfile, servcertfile, @@ -2782,26 +2800,28 @@ get_eaptls_secret(int unit, char *client, char *server, } else { + int fd; + filename = (am_server ? PPP_PATH_EAPTLSSERVFILE : PPP_PATH_EAPTLSCLIFILE); addrs = NULL; - if (!ppp_check_access(filename, &filename, 0, 0)) - return 0; fp = fopen(filename, "r"); - if (fp == NULL) - { + if (fp == NULL) { error("Can't open eap-tls secret file %s: %m", filename); - free(filename); return 0; } - check_access(fp, filename); + fd = fileno(fp); + if (!ppp_check_access(fd, filename, 0)) { + fclose(fp); + return 0; + } + check_access(fd, filename); ret = scan_authfile_eaptls(fp, client, server, clicertfile, servcertfile, cacertfile, pkfile, &addrs, &opts, filename, 0); fclose(fp); - free(filename); if (ret < 0) return 0; } diff --git a/pppd/main.c b/pppd/main.c index 34d07199..23f11241 100644 --- a/pppd/main.c +++ b/pppd/main.c @@ -1923,22 +1923,31 @@ update_script_environment(void) pid_t run_program(char *prog, char * const *args, int must_exist, void (*done)(void *), void *arg, int wait) { - int pid, status, ret; - char *rpath; + int fd, pid, status, ret; /* * First check if the file exists and is executable by root, * and couldn't have been modified by a non-root process. */ - if (!ppp_check_access(prog, &rpath, must_exist, 1)) + fd = open(prog, O_RDONLY); + if (fd < 0) { + if (errno != ENOENT || must_exist) + error("Can't access %s: %m", prog); return 0; + } + if (!ppp_check_access(fd, prog, 1)) { + close(fd); + 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); + close(fd); return -1; } if (pid != 0) { + close(fd); if (debug) dbglog("Script %s started (pid %d)", prog, pid); record_child(pid, prog, done, arg, 0); @@ -1950,7 +1959,6 @@ run_program(char *prog, char * const *args, int must_exist, void (*done)(void *) } forget_child(pid, status); } - free(rpath); return pid; } @@ -1978,14 +1986,24 @@ run_program(char *prog, char * const *args, int must_exist, void (*done)(void *) /* run the program */ update_script_environment(); - execve(rpath, args, script_env); - if (must_exist || errno != ENOENT) { - /* have to reopen the log, there's nowhere else - for the message to go. */ - reopen_log(); - syslog(LOG_ERR, "Can't execute %s: %m", rpath); - closelog(); +#ifdef HAVE_FEXECVE + fexecve(fd, args, script_env); +#else + { + char fdpath[32]; + + snprintf(fdpath, sizeof(fdpath), "/dev/fd/%d", fd); + execve(fdpath, args, script_env); + if (errno == ENOENT) { + snprintf(fdpath, sizeof(fdpath), "/proc/self/fd/%d", fd); + execve(fdpath, args, script_env); + } } +#endif + /* have to reopen the log, there's nowhere else for the message to go. */ + reopen_log(); + syslog(LOG_ERR, "Can't execute %s: %m", prog); + closelog(); _exit(99); } diff --git a/pppd/options.c b/pppd/options.c index e0fa95f9..2995a68d 100644 --- a/pppd/options.c +++ b/pppd/options.c @@ -202,6 +202,8 @@ static struct option *find_option(char *name); static int process_option(struct option *, char *, char **); static int n_arguments(struct option *); static int number_option(char *, u_int32_t *, int); +static int ppp_options_open(const char *, int, int, FILE **); +static int ppp_options_from_fp(FILE *, const char *, int); /* * Structure to store extra lists of options. @@ -571,14 +573,23 @@ int ppp_options_from_file(char *filename, int must_exist, int check_prot, int priv) { FILE *f; - int i, newline, ret, err; - struct option *opt; - int oldpriv, n; - char *oldsource; + + if (!ppp_options_open(filename, must_exist, check_prot, &f)) + return 0; + if (f == NULL) + return 1; + return ppp_options_from_fp(f, filename, priv); +} + +/* + * ppp_options_open - Open a file for options + */ +static int +ppp_options_open(const char *filename, int must_exist, int check_prot, FILE **fp) +{ uid_t euid; - char *argv[MAXARGS]; - char args[MAXARGS][MAXWORDLEN]; - char cmd[MAXWORDLEN]; + FILE *f; + int err; euid = geteuid(); if (check_prot && seteuid(getuid()) == -1) { @@ -589,6 +600,7 @@ ppp_options_from_file(char *filename, int must_exist, int check_prot, int priv) err = errno; if (check_prot && seteuid(euid) == -1) fatal("unable to regain privileges"); + *fp = f; if (f == NULL) { errno = err; if (!must_exist) { @@ -599,6 +611,22 @@ ppp_options_from_file(char *filename, int must_exist, int check_prot, int priv) ppp_option_error("Can't open options file %s: %m", filename); return 0; } + return 1; +} + +/* + * Parse options from an open FILE pointer. + */ +static int +ppp_options_from_fp(FILE *f, const char *filename, int priv) +{ + int i, newline, ret; + struct option *opt; + int oldpriv, n; + char *oldsource; + char *argv[MAXARGS]; + char args[MAXARGS][MAXWORDLEN]; + char cmd[MAXWORDLEN]; oldpriv = privileged_option; privileged_option = priv; @@ -1354,7 +1382,7 @@ readable(int fd) * \ is ignored. */ int -getword(FILE *f, char *word, int *newlinep, char *filename) +getword(FILE *f, char *word, int *newlinep, const char *filename) { int c, len, escape; int quoted, comment; @@ -1649,7 +1677,7 @@ callfile(char **argv) { char *fname, *arg, *p; int l, ok; - char *realname; + FILE *f; arg = *argv; ok = 1; @@ -1678,15 +1706,19 @@ callfile(char **argv) slprintf(fname, l, "%s%s", PPP_PATH_PEERFILES, arg); ppp_script_setenv("CALL_FILE", arg, 0); - if (!ppp_check_access(fname, &realname, 1, 0)) { + if (!ppp_options_open(fname, 1, 1, &f)) { + free(fname); + return 0; + } + if (!ppp_check_access(fileno(f), fname, 0)) { free(fname); + fclose(f); return 0; } - ok = ppp_options_from_file(realname, 1, 1, 1); + ok = ppp_options_from_fp(f, fname, 1); free(fname); - free(realname); return ok; } diff --git a/pppd/pppd-private.h b/pppd/pppd-private.h index b332f450..8a259abe 100644 --- a/pppd/pppd-private.h +++ b/pppd/pppd-private.h @@ -471,7 +471,7 @@ int get_first_ether_hwaddr(unsigned char *addr); int setipaddr(char *, char **, int); /* Set local/remote ip addresses */ int parse_args(int argc, char **argv); /* Parse options from arguments given */ -int getword(FILE *f, char *word, int *newlinep, char *filename); +int getword(FILE *f, char *word, int *newlinep, const char *filename); /* Read a word from a file */ int options_from_user(void); /* Parse options from user's .ppprc */ int options_for_tty(void); /* Parse options from /etc/ppp/options.tty */ diff --git a/pppd/pppd.h b/pppd/pppd.h index 752940da..8858d91d 100644 --- a/pppd/pppd.h +++ b/pppd/pppd.h @@ -300,7 +300,7 @@ void pr_log(void *, char *, ...); void end_pr_log(void); /* Check that a file can safely be used */ -int ppp_check_access(const char *path, char **path_to_use, int must_exist, int exec); +int ppp_check_access(int fd, const char *path, int exec); /* * Get the current exist status of pppd diff --git a/pppd/utils.c b/pppd/utils.c index 2f6304e5..b43c2f13 100644 --- a/pppd/utils.c +++ b/pppd/utils.c @@ -75,98 +75,50 @@ struct buffer_info { }; /* - * Check that a file is owned by root, not writable by group or other, - * and that all the directories in the path leading to it is likewise - * owned by root and not writable by group or other. The real path to - * the file is returned in *path_to_use, which should be freed after - * use (real meaning not containing any symlinks or ".." components). + * Check that a file descriptor is owned by root, not writable by group or + * other. * If exec is true, check for execute permission, otherwise for read * permission. + * Note: the path argument is only used for log messages. * Returns 1 if OK; if not, prints an error message and returns 0. */ int -ppp_check_access(const char *path, char **path_to_use, int must_exist, int exec) +ppp_check_access(int fd, const char *path, int exec) { - char *rpath, *slash, *part; struct stat sbuf; int perm; - /* - * Resolve symlinks such that we eliminate a few potential ToCToU - * attack avenues (eg, changing symlinks). - */ - rpath = realpath(path, NULL); - if (!rpath) { - if (errno == ENOMEM) - fatal("Insufficient memory for real path"); - if (must_exist || errno != ENOENT) - error("Can't access %s: %m", path); - return 0; + if (fstat(fd, &sbuf) != 0) { + error("Can't stat %v: %m", path); + goto err; } - /* - * 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). - */ - part = "/"; - slash = rpath; - for (;;) { - bool ok = false; - const char *pname = (slash? part: "it"); - - if (lstat(part, &sbuf) != 0) { - if (must_exist || errno != ENOENT) { - if (!slash) { - error("Can't access %v: %m", rpath); - } else { - error("Can't use %v, because of error accessing", path); - error("path component %v: %m"); - } - } - goto err; - } - - if (sbuf.st_uid != 0) { - error("Can't safely use %v because %v is not owned by root", - path, pname); - goto err; - } - - if (0 != (sbuf.st_mode & (S_IWGRP | S_IWOTH))) { - error("Can't safely use %v because %v is group or other writable", - path, pname); - goto err; - } - - if (!slash) - break; - - *slash = '/'; - part = rpath; + if (!S_ISREG(sbuf.st_mode)) { + error("Can't use %v: not a regular file", path); + goto err; + } - slash = strchr(slash + 1, '/'); - if (slash) - *slash = 0; + if (sbuf.st_uid != 0) { + error("Can't safely use %v because it is not owned by root", path); + goto err; } - if (!S_ISREG(sbuf.st_mode)) { - error("Can't use %v: not a regular file", rpath); + if (0 != (sbuf.st_mode & (S_IWGRP | S_IWOTH))) { + error("Can't safely use %v because it is group or other writable", + path); goto err; } perm = exec? S_IXUSR : S_IRUSR; if ((sbuf.st_mode & perm) == 0) { - error("Can't use %v: not %sable by root", rpath, + error("Can't use %v: not %sable by root", path, exec? "execut": "read"); goto err; } - *path_to_use = rpath; return 1; err: - free(rpath); return 0; }