checkpath: fix CVE-2018-21269

This walks the directory path to the file we are going to manipulate to make
sure that when we create the file and change the ownership and permissions
we are working on the same file.
Also, all non-terminal symbolic links must be owned by root. This will
keep a non-root user from making a symbolic link as described in the
bug. If root creates the symbolic link, it is assumed to be trusted.

On non-linux platforms, we no longer follow non-terminal symbolic links
by default. If you need to do that, add the -s option on the checkpath
command line, but keep in mind that this is not secure.

This fixes #201.
This commit is contained in:
William Hubbs 2020-11-20 09:15:59 -06:00
parent aac1734a70
commit b6fef599bf
2 changed files with 102 additions and 7 deletions

View File

@ -461,6 +461,7 @@ Mark the service as inactive.
.Op Fl p , -pipe .Op Fl p , -pipe
.Op Fl m , -mode Ar mode .Op Fl m , -mode Ar mode
.Op Fl o , -owner Ar owner .Op Fl o , -owner Ar owner
.Op Fl s , -symlinks
.Op Fl W , -writable .Op Fl W , -writable
.Op Fl q , -quiet .Op Fl q , -quiet
.Ar path ... .Ar path ...
@ -481,6 +482,11 @@ or with names, and are separated by a colon.
The truncate options (-D and -F) cause the directory or file to be The truncate options (-D and -F) cause the directory or file to be
cleared of all contents. cleared of all contents.
.Pp .Pp
If -s is not specified on a non-linux platform, checkpath will refuse to
allow non-terminal symbolic links to exist in the path. This is for
security reasons so that a non-root user can't create a symbolic link to
a root-owned file and take ownership of that file.
.Pp
If -W is specified, checkpath checks to see if the first path given on If -W is specified, checkpath checks to see if the first path given on
the command line is writable. This is different from how the test the command line is writable. This is different from how the test
command in the shell works, because it also checks to make sure the file command in the shell works, because it also checks to make sure the file

View File

@ -16,6 +16,7 @@
* except according to the terms contained in the LICENSE file. * except according to the terms contained in the LICENSE file.
*/ */
#define _GNU_SOURCE
#include <sys/types.h> #include <sys/types.h>
#include <sys/stat.h> #include <sys/stat.h>
@ -23,6 +24,7 @@
#include <fcntl.h> #include <fcntl.h>
#include <getopt.h> #include <getopt.h>
#include <grp.h> #include <grp.h>
#include <libgen.h>
#include <pwd.h> #include <pwd.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
@ -44,7 +46,7 @@ typedef enum {
const char *applet = NULL; const char *applet = NULL;
const char *extraopts ="path1 [path2] [...]"; const char *extraopts ="path1 [path2] [...]";
const char *getoptstring = "dDfFpm:o:W" getoptstring_COMMON; const char *getoptstring = "dDfFpm:o:sW" getoptstring_COMMON;
const struct option longopts[] = { const struct option longopts[] = {
{ "directory", 0, NULL, 'd'}, { "directory", 0, NULL, 'd'},
{ "directory-truncate", 0, NULL, 'D'}, { "directory-truncate", 0, NULL, 'D'},
@ -53,6 +55,7 @@ const struct option longopts[] = {
{ "pipe", 0, NULL, 'p'}, { "pipe", 0, NULL, 'p'},
{ "mode", 1, NULL, 'm'}, { "mode", 1, NULL, 'm'},
{ "owner", 1, NULL, 'o'}, { "owner", 1, NULL, 'o'},
{ "symlinks", 0, NULL, 's'},
{ "writable", 0, NULL, 'W'}, { "writable", 0, NULL, 'W'},
longopts_COMMON longopts_COMMON
}; };
@ -64,15 +67,92 @@ const char * const longopts_help[] = {
"Create a named pipe (FIFO) if not exists", "Create a named pipe (FIFO) if not exists",
"Mode to check", "Mode to check",
"Owner to check (user:group)", "Owner to check (user:group)",
"follow symbolic links (irrelivent on linux)",
"Check whether the path is writable or not", "Check whether the path is writable or not",
longopts_help_COMMON longopts_help_COMMON
}; };
const char *usagestring = NULL; const char *usagestring = NULL;
static int get_dirfd(char *path, bool symlinks) {
char *ch;
char *item;
char *linkpath = NULL;
char *path_dupe;
char *str;
int components = 0;
int dirfd;
int flags = 0;
int new_dirfd;
struct stat st;
ssize_t linksize;
if (!path || *path != '/')
eerrorx("%s: empty or relative path", applet);
dirfd = openat(dirfd, "/", O_RDONLY);
if (dirfd == -1)
eerrorx("%s: unable to open the root directory: %s",
applet, strerror(errno));
path_dupe = xstrdup(path);
ch = path_dupe;
while (*ch) {
if (*ch == '/')
components++;
ch++;
}
item = strtok(path_dupe, "/");
#ifdef O_PATH
flags |= O_PATH;
#endif
if (!symlinks)
flags |= O_NOFOLLOW;
flags |= O_RDONLY;
while (dirfd > 0 && item && components > 1) {
str = xstrdup(linkpath ? linkpath : item);
new_dirfd = openat(dirfd, str, flags);
if (new_dirfd == -1)
eerrorx("%s: %s: could not open %s: %s", applet, path, str,
strerror(errno));
if (fstat(new_dirfd, &st) == -1)
eerrorx("%s: %s: unable to stat %s: %s", applet, path, item,
strerror(errno));
if (S_ISLNK(st.st_mode) ) {
if (st.st_uid != 0)
eerrorx("%s: %s: synbolic link %s not owned by root",
applet, path, str);
linksize = st.st_size+1;
if (linkpath)
free(linkpath);
linkpath = xmalloc(linksize);
memset(linkpath, 0, linksize);
if (readlinkat(new_dirfd, "", linkpath, linksize) != st.st_size)
eerrorx("%s: symbolic link destination changed", applet);
/*
* now follow the symlink.
*/
close(new_dirfd);
} else {
close(dirfd);
dirfd = new_dirfd;
free(linkpath);
linkpath = NULL;
item = strtok(NULL, "/");
components--;
}
}
free(path_dupe);
if (linkpath) {
free(linkpath);
linkpath = NULL;
}
return dirfd;
}
static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode, static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
inode_t type, bool trunc, bool chowner, bool selinux_on) inode_t type, bool trunc, bool chowner, bool symlinks, bool selinux_on)
{ {
struct stat st; struct stat st;
char *name = NULL;
int dirfd;
int fd; int fd;
int flags; int flags;
int r; int r;
@ -93,14 +173,16 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
#endif #endif
if (trunc) if (trunc)
flags |= O_TRUNC; flags |= O_TRUNC;
readfd = open(path, readflags); xasprintf(&name, "%s", basename_c(path));
dirfd = get_dirfd(path, symlinks);
readfd = openat(dirfd, name, readflags);
if (readfd == -1 || (type == inode_file && trunc)) { if (readfd == -1 || (type == inode_file && trunc)) {
if (type == inode_file) { if (type == inode_file) {
einfo("%s: creating file", path); einfo("%s: creating file", path);
if (!mode) /* 664 */ if (!mode) /* 664 */
mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH; mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH;
u = umask(0); u = umask(0);
fd = open(path, flags, mode); fd = openat(dirfd, name, flags, mode);
umask(u); umask(u);
if (fd == -1) { if (fd == -1) {
eerror("%s: open: %s", applet, strerror(errno)); eerror("%s: open: %s", applet, strerror(errno));
@ -122,7 +204,7 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
strerror (errno)); strerror (errno));
return -1; return -1;
} }
readfd = open(path, readflags); readfd = openat(dirfd, name, readflags);
if (readfd == -1) { if (readfd == -1) {
eerror("%s: unable to open directory: %s", applet, eerror("%s: unable to open directory: %s", applet,
strerror(errno)); strerror(errno));
@ -140,7 +222,7 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
strerror (errno)); strerror (errno));
return -1; return -1;
} }
readfd = open(path, readflags); readfd = openat(dirfd, name, readflags);
if (readfd == -1) { if (readfd == -1) {
eerror("%s: unable to open fifo: %s", applet, eerror("%s: unable to open fifo: %s", applet,
strerror(errno)); strerror(errno));
@ -259,6 +341,7 @@ int main(int argc, char **argv)
int retval = EXIT_SUCCESS; int retval = EXIT_SUCCESS;
bool trunc = false; bool trunc = false;
bool chowner = false; bool chowner = false;
bool symlinks = false;
bool writable = false; bool writable = false;
bool selinux_on = false; bool selinux_on = false;
@ -293,6 +376,11 @@ int main(int argc, char **argv)
eerrorx("%s: owner `%s' not found", eerrorx("%s: owner `%s' not found",
applet, optarg); applet, optarg);
break; break;
case 's':
#ifndef O_PATH
symlinks = true;
#endif
break;
case 'W': case 'W':
writable = true; writable = true;
break; break;
@ -320,7 +408,8 @@ int main(int argc, char **argv)
while (optind < argc) { while (optind < argc) {
if (writable) if (writable)
exit(!is_writable(argv[optind])); exit(!is_writable(argv[optind]));
if (do_check(argv[optind], uid, gid, mode, type, trunc, chowner, selinux_on)) if (do_check(argv[optind], uid, gid, mode, type, trunc, chowner,
symlinks, selinux_on))
retval = EXIT_FAILURE; retval = EXIT_FAILURE;
optind++; optind++;
} }