From: James Jones Date: Mon, 14 Nov 2022 15:22:53 +0000 (-0600) Subject: Make correct changes to deal with toctou issues (CIDs below) (#4793) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a95ab269a6019389d311bf6ea898f92c19c870cb;p=thirdparty%2Ffreeradius-server.git Make correct changes to deal with toctou issues (CIDs below) (#4793) 1271307, 1445217: use *at() calls to work around toctou --- diff --git a/src/lib/server/cf_file.c b/src/lib/server/cf_file.c index 6e02ffbd08c..909e0542a36 100644 --- a/src/lib/server/cf_file.c +++ b/src/lib/server/cf_file.c @@ -40,6 +40,7 @@ RCSID("$Id$") #include #include #include +#include #include #include @@ -529,12 +530,22 @@ static int cf_file_open(CONF_SECTION *cs, char const *filename, bool from_dir, F */ if (from_dir) { cf_file_t my_file; + char const *r; + int dirfd; my_file.cs = cs; my_file.filename = filename; - /* coverity[fs_check_call] */ - if (stat(filename, &my_file.buf) < 0) goto error; + /* + * Find and open the directory containing filename so we can use + * the "at"functions to avoid time of check/time of use insecurities. + */ + if (fr_dirfd(&dirfd, &r, filename) < 0) { + ERROR("Failed to open directory containing %s", filename); + return -1; + } + + if (fstatat(dirfd, r, &my_file.buf, 0) < 0) goto error; file = fr_rb_find(tree, &my_file); @@ -547,20 +558,26 @@ static int cf_file_open(CONF_SECTION *cs, char const *filename, bool from_dir, F * However, if the file WAS read from a wildcard * $INCLUDE directory, then we read it again. */ - if (file && !file->from_dir) return 1; + if (file && !file->from_dir) { + if (dirfd != AT_FDCWD) close(dirfd); + return 1; + } + fd = openat(dirfd, r, O_RDONLY, 0); + fp = (fd < 0) ? NULL : fdopen(fd, "r"); + if (dirfd != AT_FDCWD) close(dirfd); + } else { + fp = fopen(filename, "r"); + if (fp) fd = fileno(fp); } DEBUG2("including configuration file %s", filename); - fp = fopen(filename, "r"); if (!fp) { error: ERROR("Unable to open file \"%s\": %s", filename, fr_syserror(errno)); return -1; } - fd = fileno(fp); - MEM(file = talloc(tree, cf_file_t)); file->filename = talloc_strdup(file, filename); /* The rest of the code expects this to be a talloced buffer */ diff --git a/src/lib/util/file.c b/src/lib/util/file.c index 0cd84a787c0..4272102331d 100644 --- a/src/lib/util/file.c +++ b/src/lib/util/file.c @@ -378,3 +378,34 @@ char const *fr_cwd_strip(char const *filename) return filename; } + +/** From a pathname, return fd and filename needed for *at() functions + * + * @param[in] dirfd points to place to store the dirfd + * @param[in] filename points to placd to store a pointer into pathname + * that points to the filename + * @param[in] pathname the full pathname of the file + * + * @return + * - -1 on error + * - 0 on success + */ +int fr_dirfd(int *dirfd, char const **filename, char const *pathname) +{ + char const *last_slash = strrchr(pathname, '/'); + + if (last_slash == NULL) { + *filename = pathname; + *dirfd = AT_FDCWD; + return 0; + } + { + char dirpath[(last_slash - pathname) + 1]; + + memcpy(dirpath, pathname, last_slash - pathname); + dirpath[last_slash - pathname] = '\0'; + *filename = last_slash + 1; + *dirfd = open(dirpath, O_DIRECTORY); + return (*dirfd < 0) ? -1 : 0; + } +} diff --git a/src/lib/util/file.h b/src/lib/util/file.h index 357e756d336..29d3b16c069 100644 --- a/src/lib/util/file.h +++ b/src/lib/util/file.h @@ -55,6 +55,8 @@ int fr_unlink(char const *filename); char const *fr_cwd_strip(char const *filename); +int fr_dirfd(int *dirfd, char const **filename, char const *pathname) CC_HINT(nonnull); + #ifdef __cplusplus } #endif diff --git a/src/listen/control/proto_control_unix.c b/src/listen/control/proto_control_unix.c index 97417e01c89..6f520fbd6d4 100644 --- a/src/listen/control/proto_control_unix.c +++ b/src/listen/control/proto_control_unix.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "proto_control.h" @@ -384,6 +385,8 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, #endif { int sockfd; + int dirfd; + char const *r; size_t len; socklen_t socklen; struct sockaddr_un salocal; @@ -411,14 +414,23 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, socklen = SUN_LEN(&salocal); + /* + * Find and open the directory containing path so we can use the "at" + * functions to avoid time of check/time of use insecurities. + */ + if (fr_dirfd(&dirfd, &r, path) < 0) { + fr_strerror_printf("Failed to open directory containing %s", path); + return -1; + } + /* * Check the path. */ - /* coverity[fs_check_call] */ - if (stat(path, &buf) < 0) { + if (fstatat(dirfd, r, &buf, 0) < 0) { if (errno != ENOENT) { fr_strerror_printf("Failed to stat %s: %s", path, fr_syserror(errno)); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } @@ -435,6 +447,7 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, ) { fr_strerror_printf("Cannot turn %s into socket", path); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } @@ -444,6 +457,7 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, if (buf.st_uid != geteuid()) { fr_strerror_printf("We do not own %s", path); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } @@ -456,12 +470,14 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, fr_strerror_printf("Control socket '%s' is already in use", path); close(client_fd); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } - if (unlink(path) < 0) { + if (unlinkat(dirfd, r, 0) < 0) { fr_strerror_printf("Failed to delete %s: %s", path, fr_syserror(errno)); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } } @@ -469,6 +485,7 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, if (bind(sockfd, (struct sockaddr *)&salocal, socklen) < 0) { fr_strerror_printf("Failed binding to %s: %s", path, fr_syserror(errno)); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } @@ -476,12 +493,15 @@ static int fr_server_domain_socket_peercred(char const *path, uid_t UNUSED uid, * FIXME: There's a race condition here. But Linux * doesn't seem to permit fchmod on domain sockets. */ - if (chmod(path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) < 0) { + if (fchmodat(dirfd, r, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP, 0) < 0) { fr_strerror_printf("Failed setting permissions on %s: %s", path, fr_syserror(errno)); close(sockfd); + if (dirfd != AT_FDCWD) close(dirfd); return -1; } + if (dirfd != AT_FDCWD) close(dirfd); + if (listen(sockfd, 8) < 0) { fr_strerror_printf("Failed listening to %s: %s", path, fr_syserror(errno)); close(sockfd);