From 1951e7282043dfe1268d492aea056b554baedb75 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Fri, 24 Apr 2015 10:57:23 +0200 Subject: [LIBREPORT PATCH] lib: fix races in dump directory handling code Florian Weimer : dd_opendir() should keep a file handle (opened with O_DIRECTORY) and use openat() and similar functions to access files in it. ... The file system manipulation functions should guard against hard links (check that link count is <= 1, just as in the user coredump code in abrt-hook-ccpp), possibly after opening the file with O_PATH first to avoid side effects on open/close. Related: #1214745 Signed-off-by: Jakub Filak --- src/include/dump_dir.h | 7 + src/include/internal_libreport.h | 4 + src/lib/dump_dir.c | 417 +++++++++++++++++++++++---------------- src/lib/problem_data.c | 6 +- src/lib/xfuncs.c | 22 ++- 5 files changed, 275 insertions(+), 181 deletions(-) diff --git a/src/include/dump_dir.h b/src/include/dump_dir.h index 8f672d3..d675362 100644 --- a/src/include/dump_dir.h +++ b/src/include/dump_dir.h @@ -34,6 +34,12 @@ extern "C" { /* Utility function */ int create_symlink_lockfile(const char *filename, const char *pid_str); +int create_symlink_lockfile_at(int dir_fd, const char *filename, const char *pid_str); + +/* Opens filename for reading relatively to a directory represented by dir_fd. + * The function fails if the file is symbolic link, directory or hard link. + */ +int secure_openat_read(int dir_fd, const char *filename); enum { DD_FAIL_QUIETLY_ENOENT = (1 << 0), @@ -57,6 +63,7 @@ struct dump_dir { mode_t mode; time_t dd_time; char *dd_type; + int dd_fd; }; void dd_close(struct dump_dir *dd); diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h index 8d84fd4..d35d715 100644 --- a/src/include/internal_libreport.h +++ b/src/include/internal_libreport.h @@ -406,6 +406,8 @@ int xopen3(const char *pathname, int flags, int mode); int xopen(const char *pathname, int flags); #define xunlink libreport_xunlink void xunlink(const char *pathname); +#define xunlinkat libreport_xunlinkat +void xunlinkat(int dir_fd, const char *pathname, int flags); /* Just testing dent->d_type == DT_REG is wrong: some filesystems * do not report the type, they report DT_UNKNOWN for every dirent @@ -415,6 +417,8 @@ void xunlink(const char *pathname); */ #define is_regular_file libreport_is_regular_file int is_regular_file(struct dirent *dent, const char *dirname); +#define is_regular_file_at libreport_is_regular_file_at +int is_regular_file_at(struct dirent *dent, int dir_fd); #define dot_or_dotdot libreport_dot_or_dotdot bool dot_or_dotdot(const char *filename); diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index 0048faf..16cd987 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -85,6 +85,7 @@ static char *load_text_file(const char *path, unsigned flags); +static char *load_text_file_at(int dir_fd, const char *name, unsigned flags); static void copy_file_from_chroot(struct dump_dir* dd, const char *name, const char *chroot_dir, const char *file_path); @@ -98,10 +99,10 @@ static bool isdigit_str(const char *str) return true; } -static bool exist_file_dir(const char *path) +static bool exist_file_dir_at(int dir_fd, const char *name) { struct stat buf; - if (stat(path, &buf) == 0) + if (fstatat(dir_fd, name, &buf, AT_SYMLINK_NOFOLLOW) == 0) { if (S_ISDIR(buf.st_mode) || S_ISREG(buf.st_mode)) { @@ -111,15 +112,61 @@ static bool exist_file_dir(const char *path) return false; } +/* Opens the file in the three following steps: + * 1. open the file with O_PATH (get a file descriptor for operations with + * inode) and O_NOFOLLOW (do not dereference symbolick links) + * 2. stat the resulting file descriptor and fail if the opened file is not a + * regular file or if the number of links is greater than 1 (that means that + * the inode has more names (hard links)) + * 3. "re-open" the file descriptor retrieved in the first step with O_RDONLY + * by opening /proc/self/fd/$fd (then close the former file descriptor and + * return the new one). + */ +int secure_openat_read(int dir_fd, const char *pathname) +{ + static char reopen_buf[sizeof("/proc/self/fd/") + 3*sizeof(int) + 1]; + + int path_fd = openat(dir_fd, pathname, O_PATH | O_NOFOLLOW); + if (path_fd < 0) + return -1; + + struct stat path_sb; + int r = fstat(path_fd, &path_sb); + if (r < 0) + { + perror_msg("stat"); + close(path_fd); + return -1; + } + + if (!S_ISREG(path_sb.st_mode) || path_sb.st_nlink > 1) + { + log_notice("Path isn't a regular file or has more links (%lu)", path_sb.st_nlink); + errno = EINVAL; + close(path_fd); + return -1; + } + + if (snprintf(reopen_buf, sizeof(reopen_buf), "/proc/self/fd/%d", path_fd) >= sizeof(reopen_buf)) { + error_msg("BUG: too long path to a file descriptor"); + abort(); + } + + const int fd = open(reopen_buf, O_RDONLY); + close(path_fd); + + return fd; +} + /* Returns value less than 0 if the file is not readable or * if the file doesn't contain valid unixt time stamp. * * Any possible failure will be logged. */ -static time_t parse_time_file(const char *filename) +static time_t parse_time_file_at(int dir_fd, const char *filename) { /* Open input file, and parse it. */ - int fd = open(filename, O_RDONLY | O_NOFOLLOW); + int fd = secure_openat_read(dir_fd, filename); if (fd < 0) { VERB2 pwarn_msg("Can't open '%s'", filename); @@ -183,9 +230,9 @@ static time_t parse_time_file(const char *filename) * 0: failed to lock (someone else has it locked) * 1: success */ -int create_symlink_lockfile(const char* lock_file, const char* pid) +int create_symlink_lockfile_at(int dir_fd, const char* lock_file, const char* pid) { - while (symlink(pid, lock_file) != 0) + while (symlinkat(pid, dir_fd, lock_file) != 0) { if (errno != EEXIST) { @@ -198,7 +245,7 @@ int create_symlink_lockfile(const char* lock_file, const char* pid) } char pid_buf[sizeof(pid_t)*3 + 4]; - ssize_t r = readlink(lock_file, pid_buf, sizeof(pid_buf) - 1); + ssize_t r = readlinkat(dir_fd, lock_file, pid_buf, sizeof(pid_buf) - 1); if (r < 0) { if (errno == ENOENT) @@ -230,7 +277,7 @@ int create_symlink_lockfile(const char* lock_file, const char* pid) log("Lock file '%s' was locked by process %s, but it crashed?", lock_file, pid_buf); } /* The file may be deleted by now by other process. Ignore ENOENT */ - if (unlink(lock_file) != 0 && errno != ENOENT) + if (unlinkat(dir_fd, lock_file, /*only files*/0) != 0 && errno != ENOENT) { perror_msg("Can't remove stale lock file '%s'", lock_file); errno = 0; @@ -242,21 +289,21 @@ int create_symlink_lockfile(const char* lock_file, const char* pid) return 1; } +int create_symlink_lockfile(const char *filename, const char *pid_str) +{ + return create_symlink_lockfile_at(AT_FDCWD, filename, pid_str); +} + static const char *dd_check(struct dump_dir *dd) { - unsigned dirname_len = strlen(dd->dd_dirname); - char filename_buf[FILENAME_MAX+1]; - strcpy(filename_buf, dd->dd_dirname); - strcpy(filename_buf + dirname_len, "/"FILENAME_TIME); - dd->dd_time = parse_time_file(filename_buf); + dd->dd_time = parse_time_file_at(dd->dd_fd, FILENAME_TIME); if (dd->dd_time < 0) { log_warning("Missing file: "FILENAME_TIME); return FILENAME_TIME; } - strcpy(filename_buf + dirname_len, "/"FILENAME_TYPE); - dd->dd_type = load_text_file(filename_buf, DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE); + dd->dd_type = load_text_file_at(dd->dd_fd, FILENAME_TYPE, DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE); if (!dd->dd_type || (strlen(dd->dd_type) == 0)) { log_warning("Missing or empty file: "FILENAME_TYPE); @@ -274,17 +321,12 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) char pid_buf[sizeof(long)*3 + 2]; snprintf(pid_buf, sizeof(pid_buf), "%lu", (long)getpid()); - unsigned dirname_len = strlen(dd->dd_dirname); - char lock_buf[dirname_len + sizeof("/.lock")]; - strcpy(lock_buf, dd->dd_dirname); - strcpy(lock_buf + dirname_len, "/.lock"); - unsigned count = NO_TIME_FILE_COUNT; retry: while (1) { - int r = create_symlink_lockfile(lock_buf, pid_buf); + int r = create_symlink_lockfile_at(dd->dd_fd, ".lock", pid_buf); if (r < 0) return r; /* error */ if (r > 0) @@ -304,8 +346,8 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ if (missing_file) { - xunlink(lock_buf); - log_warning("Unlocked '%s' (no or corrupted '%s' file)", lock_buf, missing_file); + xunlinkat(dd->dd_fd, ".lock", /*only files*/0); + log_warning("Unlocked '%s' (no or corrupted '%s' file)", dd->dd_dirname, missing_file); if (--count == 0 || flags & DD_DONT_WAIT_FOR_LOCK) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */ @@ -326,13 +368,9 @@ static void dd_unlock(struct dump_dir *dd) { dd->locked = 0; - unsigned dirname_len = strlen(dd->dd_dirname); - char lock_buf[dirname_len + sizeof("/.lock")]; - strcpy(lock_buf, dd->dd_dirname); - strcpy(lock_buf + dirname_len, "/.lock"); - xunlink(lock_buf); + xunlinkat(dd->dd_fd, ".lock", /*only files*/0); - log_info("Unlocked '%s'", lock_buf); + log_info("Unlocked '%s/.lock'", dd->dd_dirname); } } @@ -340,17 +378,16 @@ static inline struct dump_dir *dd_init(void) { struct dump_dir* dd = (struct dump_dir*)xzalloc(sizeof(struct dump_dir)); dd->dd_time = -1; + dd->dd_fd = -1; return dd; } -int dd_exist(const struct dump_dir *dd, const char *path) +int dd_exist(const struct dump_dir *dd, const char *name) { - if (!str_is_correct_filename(path)) - error_msg_and_die("Cannot test existence. '%s' is not a valid file name", path); + if (!str_is_correct_filename(name)) + error_msg_and_die("Cannot test existence. '%s' is not a valid file name", name); - char *full_path = concat_path_file(dd->dd_dirname, path); - int ret = exist_file_dir(full_path); - free(full_path); + const int ret = exist_file_dir_at(dd->dd_fd, name); return ret; } @@ -360,6 +397,7 @@ void dd_close(struct dump_dir *dd) return; dd_unlock(dd); + close(dd->dd_fd); if (dd->next_dir) { closedir(dd->next_dir); @@ -384,10 +422,13 @@ struct dump_dir *dd_opendir(const char *dir, int flags) struct dump_dir *dd = dd_init(); dir = dd->dd_dirname = rm_trailing_slashes(dir); - + dd->dd_fd = open(dir, O_DIRECTORY | O_NOFOLLOW); struct stat stat_buf; - if (stat(dir, &stat_buf) != 0) + if (dd->dd_fd < 0) goto cant_access; + if (fstat(dd->dd_fd, &stat_buf) != 0) + goto cant_access; + /* & 0666 should remove the executable bit */ dd->mode = (stat_buf.st_mode & 0666); @@ -397,11 +438,12 @@ struct dump_dir *dd_opendir(const char *dir, int flags) if ((flags & DD_OPEN_READONLY) && errno == EACCES) { /* Directory is not writable. If it seems to be readable, - * return "read only" dd, not NULL */ - if (stat(dir, &stat_buf) == 0 - && S_ISDIR(stat_buf.st_mode) - && access(dir, R_OK) == 0 - ) { + * return "read only" dd, not NULL + * + * Does the directory have 'x' flag? + */ + if (faccessat(dd->dd_fd, ".", R_OK, AT_SYMLINK_NOFOLLOW) == 0) + { if(dd_check(dd) != NULL) { dd_close(dd); @@ -444,10 +486,9 @@ struct dump_dir *dd_opendir(const char *dir, int flags) if (geteuid() == 0) { /* In case caller would want to create more files, he'll need uid:gid */ - struct stat stat_buf; - if (stat(dir, &stat_buf) != 0 || !S_ISDIR(stat_buf.st_mode)) + if (fstat(dd->dd_fd, &stat_buf) != 0) { - error_msg("Can't stat '%s', or it is not a directory", dir); + error_msg("Can't stat '%s'", dir); dd_close(dd); return NULL; } @@ -542,8 +583,7 @@ struct dump_dir *dd_create_skeleton(const char *dir, uid_t uid, mode_t mode, int * dd_create("dir/..") and similar are madness, refuse them. */ error_msg("Bad dir name '%s'", dir); - dd_close(dd); - return NULL; + goto fail; } /* Was creating it with mode 0700 and user as the owner, but this allows @@ -559,22 +599,31 @@ struct dump_dir *dd_create_skeleton(const char *dir, uid_t uid, mode_t mode, int if (r != 0) { perror_msg("Can't create directory '%s'", dir); - dd_close(dd); - return NULL; + goto fail; } - if (dd_lock(dd, CREATE_LOCK_USLEEP, /*flags:*/ 0) < 0) + dd->dd_fd = open(dd->dd_dirname, O_DIRECTORY | O_NOFOLLOW); + if (dd->dd_fd < 0) { - dd_close(dd); - return NULL; + perror_msg("Can't open newly created directory '%s'", dir); + goto fail; } + struct stat stat_sb; + if (fstat(dd->dd_fd, &stat_sb) < 0) + { + perror_msg("stat(%s)", dd->dd_dirname); + goto fail; + } + + if (dd_lock(dd, CREATE_LOCK_USLEEP, /*flags:*/ 0) < 0) + goto fail; + /* mkdir's mode (above) can be affected by umask, fix it */ - if (chmod(dir, dir_mode) == -1) + if (fchmod(dd->dd_fd, dir_mode) == -1) { perror_msg("Can't change mode of '%s'", dir); - dd_close(dd); - return NULL; + goto fail; } dd->dd_uid = (uid_t)-1L; @@ -616,6 +665,10 @@ struct dump_dir *dd_create_skeleton(const char *dir, uid_t uid, mode_t mode, int } return dd; + +fail: + dd_close(dd); + return NULL; } /* Resets ownership of the given directory to UID and GID according to values @@ -623,7 +676,7 @@ struct dump_dir *dd_create_skeleton(const char *dir, uid_t uid, mode_t mode, int */ int dd_reset_ownership(struct dump_dir *dd) { - const int r =lchown(dd->dd_dirname, dd->dd_uid, dd->dd_gid); + const int r = fchown(dd->dd_fd, dd->dd_uid, dd->dd_gid); if (r < 0) { perror_msg("Can't change '%s' ownership to %lu:%lu", dd->dd_dirname, @@ -740,59 +793,39 @@ void dd_sanitize_mode_and_owner(struct dump_dir *dd) if (!dd->locked) error_msg_and_die("dump_dir is not opened"); /* bug */ - DIR *d = opendir(dd->dd_dirname); - if (!d) - return; - - struct dirent *dent; - while ((dent = readdir(d)) != NULL) + dd_init_next_file(dd); + char *short_name; + while (dd_get_next_file(dd, &short_name, /*full_name*/ NULL)) { - if (dent->d_name[0] == '.') /* ".lock", ".", ".."? skip */ - continue; - char *full_path = concat_path_file(dd->dd_dirname, dent->d_name); - struct stat statbuf; - if (lstat(full_path, &statbuf) == 0 && S_ISREG(statbuf.st_mode)) - { - if ((statbuf.st_mode & 0777) != dd->mode) - { - /* We open the file only for fchmod() - * - * We use fchmod() because chmod() changes the permissions of - * the file specified whose pathname is given in path, which - * is dereferenced if it is a symbolic link. - */ - int fd = open(full_path, O_RDONLY | O_NOFOLLOW, dd->mode); - if (fd >= 0) - { - if (fchmod(fd, dd->mode) != 0) - { - perror_msg("Can't change '%s' mode to 0%o", full_path, - (unsigned)dd->mode); - } - close(fd); - } - else - { - perror_msg("Can't open regular file '%s'", full_path); - } - } - if (statbuf.st_uid != dd->dd_uid || statbuf.st_gid != dd->dd_gid) - { - if (lchown(full_path, dd->dd_uid, dd->dd_gid) != 0) - { - perror_msg("Can't change '%s' ownership to %lu:%lu", full_path, - (long)dd->dd_uid, (long)dd->dd_gid); - } - } - } - free(full_path); + /* The current process has to have read access at least */ + int fd = secure_openat_read(dd->dd_fd, short_name); + if (fd < 0) + goto next; + + if (fchmod(fd, dd->mode) != 0) + perror_msg("Can't change '%s/%s' mode to 0%o", dd->dd_dirname, short_name, + (unsigned)dd->mode); + + if (fchown(fd, dd->dd_uid, dd->dd_gid) != 0) + perror_msg("Can't change '%s/%s' ownership to %lu:%lu", dd->dd_dirname, short_name, + (long)dd->dd_uid, (long)dd->dd_gid); + + close(fd); +next: + free(short_name); } - closedir(d); } -static int delete_file_dir(const char *dir, bool skip_lock_file) +static int delete_file_dir(int dir_fd, bool skip_lock_file) { - DIR *d = opendir(dir); + int opendir_fd = dup(dir_fd); + if (opendir_fd < 0) + { + perror_msg("delete_file_dir: dup(dir_fd)"); + return -1; + } + + DIR *d = fdopendir(opendir_fd); if (!d) { /* The caller expects us to error out only if the directory @@ -818,26 +851,35 @@ static int delete_file_dir(const char *dir, bool skip_lock_file) unlink_lock_file = true; continue; } - char *full_path = concat_path_file(dir, dent->d_name); - if (unlink(full_path) == -1 && errno != ENOENT) + if (unlinkat(dir_fd, dent->d_name, /*only files*/0) == -1 && errno != ENOENT) { int err = 0; if (errno == EISDIR) { errno = 0; - err = delete_file_dir(full_path, /*skip_lock_file:*/ false); + int subdir_fd = openat(dir_fd, dent->d_name, O_DIRECTORY); + if (subdir_fd < 0) + { + perror_msg("Can't open sub-dir'%s'", dent->d_name); + closedir(d); + return -1; + } + else + { + err = delete_file_dir(subdir_fd, /*skip_lock_file:*/ false); + close(subdir_fd); + if (err == 0) + unlinkat(dir_fd, dent->d_name, AT_REMOVEDIR); + } } if (errno || err) { - perror_msg("Can't remove '%s'", full_path); - free(full_path); + perror_msg("Can't remove '%s'", dent->d_name); closedir(d); return -1; } } - free(full_path); } - closedir(d); /* Here we know for sure that all files/subdirs we found via readdir * were deleted successfully. If rmdir below fails, we assume someone @@ -845,29 +887,9 @@ static int delete_file_dir(const char *dir, bool skip_lock_file) */ if (unlink_lock_file) - { - char *full_path = concat_path_file(dir, ".lock"); - xunlink(full_path); - free(full_path); - - unsigned cnt = RMDIR_FAIL_COUNT; - do { - if (rmdir(dir) == 0) - return 0; - /* Someone locked the dir after unlink, but before rmdir. - * This "someone" must be dd_lock(). - * It detects this (by seeing that there is no time file) - * and backs off at once. So we need to just retry rmdir, - * with minimal sleep. - */ - usleep(RMDIR_FAIL_USLEEP); - } while (--cnt != 0); - } + xunlinkat(dir_fd, ".lock", /*only files*/0); - int r = rmdir(dir); - if (r) - perror_msg("Can't remove directory '%s'", dir); - return r; + return 0; } int dd_delete(struct dump_dir *dd) @@ -878,10 +900,34 @@ int dd_delete(struct dump_dir *dd) return -1; } - int r = delete_file_dir(dd->dd_dirname, /*skip_lock_file:*/ true); + if (delete_file_dir(dd->dd_fd, /*skip_lock_file:*/ true) != 0) + { + perror_msg("Can't remove contents of directory '%s'", dd->dd_dirname); + return -2; + } + + unsigned cnt = RMDIR_FAIL_COUNT; + do { + if (rmdir(dd->dd_dirname) == 0) + break; + /* Someone locked the dir after unlink, but before rmdir. + * This "someone" must be dd_lock(). + * It detects this (by seeing that there is no time file) + * and backs off at once. So we need to just retry rmdir, + * with minimal sleep. + */ + usleep(RMDIR_FAIL_USLEEP); + } while (--cnt != 0); + + if (cnt == 0) + { + perror_msg("Can't remove directory '%s'", dd->dd_dirname); + return -3; + } + dd->locked = 0; /* delete_file_dir already removed .lock */ dd_close(dd); - return r; + return 0; } int dd_chown(struct dump_dir *dd, uid_t new_uid) @@ -911,29 +957,37 @@ int dd_chown(struct dump_dir *dd, uid_t new_uid) gid_t groups_gid = pw->pw_gid; #endif - int chown_res = lchown(dd->dd_dirname, owners_uid, groups_gid); + int chown_res = fchown(dd->dd_fd, owners_uid, groups_gid); if (chown_res) - perror_msg("lchown('%s')", dd->dd_dirname); + perror_msg("fchown('%s')", dd->dd_dirname); else { dd_init_next_file(dd); - char *full_name; - while (chown_res == 0 && dd_get_next_file(dd, /*short_name*/ NULL, &full_name)) + char *short_name; + while (chown_res == 0 && dd_get_next_file(dd, &short_name, /*full_name*/ NULL)) { - log_debug("chowning %s", full_name); - chown_res = lchown(full_name, owners_uid, groups_gid); + /* The current process has to have read access at least */ + int fd = secure_openat_read(dd->dd_fd, short_name); + if (fd < 0) + goto next; + + log_debug("chowning %s", short_name); + + chown_res = fchown(fd, owners_uid, groups_gid); if (chown_res) - perror_msg("lchown('%s')", full_name); - free(full_name); + perror_msg("fchownat('%s')", short_name); + + close(fd); +next: + free(short_name); } } return chown_res; } -static char *load_text_file(const char *path, unsigned flags) +static char *load_text_from_file_descriptor(int fd, const char *path, int flags) { - int fd = open(path, O_RDONLY | ((flags & DD_OPEN_FOLLOW) ? 0 : O_NOFOLLOW)); if (fd == -1) { if (!(flags & DD_FAIL_QUIETLY_ENOENT)) @@ -988,6 +1042,20 @@ static char *load_text_file(const char *path, unsigned flags) return strbuf_free_nobuf(buf_content); } +static char *load_text_file_at(int dir_fd, const char *name, unsigned flags) +{ + assert(name[0] != '/'); + + const int fd = openat(dir_fd, name, O_RDONLY | ((flags & DD_OPEN_FOLLOW) ? 0 : O_NOFOLLOW)); + return load_text_from_file_descriptor(fd, name, flags); +} + +static char *load_text_file(const char *path, unsigned flags) +{ + const int fd = open(path, O_RDONLY | ((flags & DD_OPEN_FOLLOW) ? 0 : O_NOFOLLOW)); + return load_text_from_file_descriptor(fd, path, flags); +} + static void copy_file_from_chroot(struct dump_dir* dd, const char *name, const char *chroot_dir, const char *file_path) { char *chrooted_name = concat_path_file(chroot_dir, file_path); @@ -1001,14 +1069,16 @@ static void copy_file_from_chroot(struct dump_dir* dd, const char *name, const c } } -static bool save_binary_file(const char *path, const char* data, unsigned size, uid_t uid, gid_t gid, mode_t mode) +static bool save_binary_file_at(int dir_fd, const char *name, const char* data, unsigned size, uid_t uid, gid_t gid, mode_t mode) { + assert(name[0] != '/'); + /* the mode is set by the caller, see dd_create() for security analysis */ - unlink(path); - int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT | O_NOFOLLOW, mode); + unlinkat(dir_fd, name, /*remove only files*/0); + int fd = openat(dir_fd, name, O_WRONLY | O_EXCL | O_CREAT | O_NOFOLLOW, mode); if (fd < 0) { - perror_msg("Can't open file '%s'", path); + perror_msg("Can't open file '%s'", name); return false; } @@ -1016,7 +1086,9 @@ static bool save_binary_file(const char *path, const char* data, unsigned size, { if (fchown(fd, uid, gid) == -1) { - perror_msg("Can't change '%s' ownership to %lu:%lu", path, (long)uid, (long)gid); + perror_msg("Can't change '%s' ownership to %lu:%lu", name, (long)uid, (long)gid); + close(fd); + return false; } } @@ -1028,14 +1100,16 @@ static bool save_binary_file(const char *path, const char* data, unsigned size, */ if (fchmod(fd, mode) == -1) { - perror_msg("Can't change mode of '%s'", path); + perror_msg("Can't change mode of '%s'", name); + close(fd); + return false; } unsigned r = full_write(fd, data, size); close(fd); if (r != size) { - error_msg("Can't save file '%s'", path); + error_msg("Can't save file '%s'", name); return false; } @@ -1058,11 +1132,7 @@ char* dd_load_text_ext(const struct dump_dir *dd, const char *name, unsigned fla if (strcmp(name, "release") == 0) name = FILENAME_OS_RELEASE; - char *full_path = concat_path_file(dd->dd_dirname, name); - char *ret = load_text_file(full_path, flags); - free(full_path); - - return ret; + return load_text_file_at(dd->dd_fd, name, flags); } char* dd_load_text(const struct dump_dir *dd, const char *name) @@ -1078,9 +1148,7 @@ void dd_save_text(struct dump_dir *dd, const char *name, const char *data) if (!str_is_correct_filename(name)) error_msg_and_die("Cannot save text. '%s' is not a valid file name", name); - char *full_path = concat_path_file(dd->dd_dirname, name); - save_binary_file(full_path, data, strlen(data), dd->dd_uid, dd->dd_gid, dd->mode); - free(full_path); + save_binary_file_at(dd->dd_fd, name, data, strlen(data), dd->dd_uid, dd->dd_gid, dd->mode); } void dd_save_binary(struct dump_dir* dd, const char* name, const char* data, unsigned size) @@ -1091,9 +1159,7 @@ void dd_save_binary(struct dump_dir* dd, const char* name, const char* data, uns if (!str_is_correct_filename(name)) error_msg_and_die("Cannot save binary. '%s' is not a valid file name", name); - char *full_path = concat_path_file(dd->dd_dirname, name); - save_binary_file(full_path, data, size, dd->dd_uid, dd->dd_gid, dd->mode); - free(full_path); + save_binary_file_at(dd->dd_fd, name, data, size, dd->dd_uid, dd->dd_gid, dd->mode); } long dd_get_item_size(struct dump_dir *dd, const char *name) @@ -1102,21 +1168,19 @@ long dd_get_item_size(struct dump_dir *dd, const char *name) error_msg_and_die("Cannot get item size. '%s' is not a valid file name", name); long size = -1; - char *iname = concat_path_file(dd->dd_dirname, name); struct stat statbuf; + int r = fstatat(dd->dd_fd, name, &statbuf, AT_SYMLINK_NOFOLLOW); - if (lstat(iname, &statbuf) == 0 && S_ISREG(statbuf.st_mode)) + if (r == 0 && S_ISREG(statbuf.st_mode)) size = statbuf.st_size; else { if (errno == ENOENT) size = 0; else - perror_msg("Can't get size of file '%s'", iname); + perror_msg("Can't get size of file '%s'", name); } - free(iname); - return size; } @@ -1128,18 +1192,16 @@ int dd_delete_item(struct dump_dir *dd, const char *name) if (!str_is_correct_filename(name)) error_msg_and_die("Cannot delete item. '%s' is not a valid file name", name); - char *path = concat_path_file(dd->dd_dirname, name); - int res = unlink(path); + int res = unlinkat(dd->dd_fd, name, /*only files*/0); if (res < 0) { if (errno == ENOENT) errno = res = 0; else - perror_msg("Can't delete file '%s'", path); + perror_msg("Can't delete file '%s'", name); } - free(path); return res; } @@ -1147,11 +1209,17 @@ DIR *dd_init_next_file(struct dump_dir *dd) { // if (!dd->locked) // error_msg_and_die("dump_dir is not opened"); /* bug */ + int opendir_fd = dup(dd->dd_fd); + if (opendir_fd < 0) + { + perror_msg("dd_init_next_file: dup(dd_fd)"); + return NULL; + } if (dd->next_dir) closedir(dd->next_dir); - dd->next_dir = opendir(dd->dd_dirname); + dd->next_dir = fdopendir(opendir_fd); if (!dd->next_dir) { error_msg("Can't open directory '%s'", dd->dd_dirname); @@ -1168,7 +1236,7 @@ int dd_get_next_file(struct dump_dir *dd, char **short_name, char **full_name) struct dirent *dent; while ((dent = readdir(dd->next_dir)) != NULL) { - if (is_regular_file(dent, dd->dd_dirname)) + if (is_regular_file_at(dent, dd->dd_fd)) { if (short_name) *short_name = xstrdup(dent->d_name); @@ -1233,6 +1301,7 @@ int dd_rename(struct dump_dir *dd, const char *new_path) return -1; } + /* Keeps the opened file descriptor valid */ int res = rename(dd->dd_dirname, new_path); if (res == 0) { diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c index fc07288..ebddd3c 100644 --- a/src/lib/problem_data.c +++ b/src/lib/problem_data.c @@ -279,14 +279,14 @@ static const char *const always_text_files[] = { FILENAME_OS_RELEASE, NULL }; -static char* is_text_file(const char *name, ssize_t *sz) +static char* is_text_file_at(int dir_fd, const char *name, ssize_t *sz) { /* We were using magic.h API to check for file being text, but it thinks * that file containing just "0" is not text (!!) * So, we do it ourself. */ - int fd = open(name, O_RDONLY); + int fd = secure_openat_read(dir_fd, name); if (fd < 0) return NULL; /* it's not text (because it does not exist! :) */ @@ -399,7 +399,7 @@ void problem_data_load_from_dump_dir(problem_data_t *problem_data, struct dump_d } ssize_t sz = 4*1024; - char *text = is_text_file(full_name, &sz); + char *text = is_text_file_at(dd->dd_fd, short_name, &sz); if (!text || text == HUGE_TEXT) { int flag = !text ? CD_FLAG_BIN : (CD_FLAG_BIN+CD_FLAG_BIGTXT); diff --git a/src/lib/xfuncs.c b/src/lib/xfuncs.c index 1ce44aa..979c7b8 100644 --- a/src/lib/xfuncs.c +++ b/src/lib/xfuncs.c @@ -331,6 +331,12 @@ int xopen(const char *pathname, int flags) return xopen3(pathname, flags, 0666); } +void xunlinkat(int dir_fd, const char *pathname, int flags) +{ + if (unlinkat(dir_fd, pathname, flags)) + perror_msg_and_die("Can't remove file '%s'", pathname); +} + void xunlink(const char *pathname) { if (unlink(pathname)) @@ -359,21 +365,29 @@ int open_or_warn(const char *pathname, int flags) * do not report the type, they report DT_UNKNOWN for every dirent * (and this is not a bug in filesystem, this is allowed by standards). */ -int is_regular_file(struct dirent *dent, const char *dirname) +int is_regular_file_at(struct dirent *dent, int dir_fd) { if (dent->d_type == DT_REG) return 1; if (dent->d_type != DT_UNKNOWN) return 0; - char *fullname = xasprintf("%s/%s", dirname, dent->d_name); struct stat statbuf; - int r = lstat(fullname, &statbuf); - free(fullname); + int r = fstatat(dir_fd, dent->d_name, &statbuf, AT_SYMLINK_NOFOLLOW); return r == 0 && S_ISREG(statbuf.st_mode); } +int is_regular_file(struct dirent *dent, const char *dirname) +{ + int dir_fd = open(dirname, O_DIRECTORY); + if (dir_fd < 0) + return 0; + int r = is_regular_file_at(dent, dir_fd); + close(dir_fd); + return r; +} + /* Is it "." or ".."? */ /* abrtlib candidate */ bool dot_or_dotdot(const char *filename) -- 1.8.3.1