From 1951e7282043dfe1268d492aea056b554baedb75 Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
Date: Fri, 24 Apr 2015 10:57:23 +0200
Subject: [LIBREPORT PATCH] lib: fix races in dump directory handling code
Florian Weimer <fweimer@redhat.com>:
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 <jfilak@redhat.com>
---
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