[ovs-dev] [PATCH v4 2/3] chutil: Add hardness amplification versions of chmod/chown
Aaron Conole
aconole at redhat.com
Fri Aug 19 23:48:13 UTC 2016
For certain types of files (in particular Unix Domain Sockets), the standard
fchmod/fchown calls have strange side effects. While Unix Domain Sockets
have their own particular quirks depending on the system, there may be other
files where Open vSwitch would operate on the file without a concrete file
handle. To detect, and possibly prevent, file system races when operating on
these files, two new functions are added (ovs_kchmod and ovs_kchown), which
perform hardness amplification and column-wise traversal as described in
https://www.usenix.org/legacy/event/fast08/tech/full_papers/tsafrir/tsafrir_html/index.html
Fallbacks are provided which return ENOTSUP on systems where the POSIX1.2008
function calls for {fchmod,fchown,fstat,readlink,open}at are not available.
Signed-off-by: Aaron Conole <aconole at redhat.com>
---
v3->v4:
* Complete rewrite using both directory traversal and hardness amplification.
The check for the *at were added for the travis build - AIUI FreeBSD does
provide those functions, but I did not test there.
configure.ac | 2 +-
lib/chutil-unix.c | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++
lib/chutil.h | 4 +
tests/test-chutil.c | 92 ++++++++++++----
4 files changed, 382 insertions(+), 20 deletions(-)
diff --git a/configure.ac b/configure.ac
index 2f854dd..e7cce37 100644
--- a/configure.ac
+++ b/configure.ac
@@ -105,7 +105,7 @@ AC_CHECK_DECLS([sys_siglist], [], [], [[#include <signal.h>]])
AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec],
[], [], [[#include <sys/stat.h>]])
AC_CHECK_MEMBERS([struct ifreq.ifr_flagshigh], [], [], [[#include <net/if.h>]])
-AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r])
+AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r fchmodat fchownat fstatat openat readlinkat])
AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h stdatomic.h])
AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include <sys/types.h>
#include <net/if.h>]])
diff --git a/lib/chutil-unix.c b/lib/chutil-unix.c
index f0fd3c9..4920b9a 100644
--- a/lib/chutil-unix.c
+++ b/lib/chutil-unix.c
@@ -19,12 +19,14 @@
#include "chutil.h"
#include <errno.h>
+#include <fcntl.h>
#include <grp.h>
#include <inttypes.h>
#include <pwd.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
+#include <sys/types.h>
#include <unistd.h>
#include "daemon.h"
@@ -48,6 +50,75 @@ VLOG_DEFINE_THIS_MODULE(chutil_unix);
#define SUID_MODES (S_ISUID | S_ISGID)
+/* The following functions are being defined to enable builds on systems
+ * where the POSIX1.2008 *at functions are not available. They are built
+ * to signal error to the application. Future work might include auditing
+ * all chdir calls and building an appropriate set of interlocked functions
+ * to emulate this behavior. */
+
+#ifndef HAVE_FCHMODAT
+static int
+fchmodat(int a OVS_UNUSED, const char *b OVS_UNUSED, mode_t c OVS_UNUSED,
+ int d OVS_UNUSED)
+{
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
+#ifndef HAVE_FCHOWNAT
+static int
+fchownat(int a OVS_UNUSED, const char *b OVS_UNUSED, uid_t c OVS_UNUSED,
+ gid_t d OVS_UNUSED, int e OVS_UNUSED)
+{
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
+#ifndef HAVE_FSTATAT
+static int
+fstatat(int a OVS_UNUSED, const char *b OVS_UNUSED, struct stat *c OVS_UNUSED,
+ int d OVS_UNUSED)
+{
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
+#ifndef HAVE_OPENAT
+static int
+openat(int a OVS_UNUSED, const char *b OVS_UNUSED, int c OVS_UNUSED, ...)
+{
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
+#ifndef HAVE_READLINKAT
+static int
+readlinkat(int a OVS_UNUSED, const char *b OVS_UNUSED, char *c OVS_UNUSED,
+ size_t d OVS_UNUSED)
+{
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
+#ifndef O_PATH
+#define O_PATH 0
+#endif
+
+#ifndef EBADFD
+/* On platforms without EBADFD support, fallback to overloading EBADF. */
+#define EBADFD EBADF
+#endif
+
+#ifndef AT_SYMLINK_NOFOLLOW
+#define AT_SYMLINK_NOFOLLOW 0
+#endif
+
+
/* Convert a chown-style string to uid/gid; supports numeric arguments
* as well as usernames. */
int
@@ -266,6 +337,239 @@ actions:
}
+#define K_ROUNDS 9 /* Lifted from "Portably Solving File TOCTTOU Races with
+ Hardness Amplification" by Tsafrir, et. al. */
+
+static int cmp_stat(struct stat *s1, struct stat *s2)
+{
+ return s1->st_ino == s2->st_ino && s1->st_dev == s2->st_dev &&
+ s1->st_mode == s2->st_mode;
+}
+
+
+/* Checks whether the relative path element is a symlink. If an error
+ * occurs, returns -1. If the path is a symlink, returns 1. Otherwise,
+ * returns 0. The stat struct, and target, are output variables and are
+ * considered valid unless the return value is -1. In the case that the
+ * hardness Amplification fails, errno will be set to EBADFD. */
+static int is_symlink(int dirfd, const char *path_elem, char target[],
+ size_t target_len, struct stat *s)
+{
+ struct stat s_cmp;
+ int result;
+ for (int k = 0; k < K_ROUNDS; ++k) {
+ result = fstatat(dirfd, path_elem, s, 0);
+ if (!k) {
+ s_cmp = *s;
+ } else if (!cmp_stat(&s_cmp, s)){
+ return -1;
+ }
+ }
+
+ if (!result && S_ISLNK(s->st_mode)) {
+ result = readlinkat(dirfd, path_elem, target, target_len);
+ if (result != -1) {
+ target[result] = '\0';
+ result = 1;
+ }
+ }
+ return result;
+}
+
+
+static int directory_traverse(const char *path, int top_fd)
+{
+ char *dir_path = NULL, *full_path = NULL;
+ int dirfd = -1;
+ struct stat st;
+
+ if (path[0] != '/' && top_fd == -1) {
+ char cwd_buf[PATH_MAX] = {0};
+ if (getcwd(cwd_buf, PATH_MAX)) {
+ top_fd = directory_traverse(cwd_buf, -1);
+ }
+ } else {
+ top_fd = open("/", O_PATH);
+ }
+
+ if (top_fd == -1 || !strcmp(path, "/")) {
+ return top_fd;
+ }
+
+ dir_path = full_path = xstrdup(path);
+ dir_path += strspn(dir_path, "/");
+ do {
+ char symlink_target[PATH_MAX] = {0};
+ if (strspn(dir_path, "/")) {
+ *(dir_path + strspn(dir_path, "/")) = '\0';
+ }
+ switch (is_symlink(top_fd, dir_path, symlink_target, PATH_MAX, &st)) {
+ case 0:
+ dirfd = openat(top_fd, dir_path, O_PATH|O_NOFOLLOW|O_DIRECTORY);
+ break;
+ case 1:
+ if (symlink_target[0] != '/') {
+ dirfd = directory_traverse(symlink_target, top_fd);
+ } else {
+ dirfd = directory_traverse(symlink_target, -1);
+ }
+ break;
+ default:
+ dirfd = -1;
+ break;
+ }
+ close(top_fd);
+ if (dirfd != -1) {
+ struct stat s;
+ if (!fstat(dirfd, &s) && cmp_stat(&s, &st)) {
+ size_t path_elem_len = strlen(dir_path);
+ dir_path += path_elem_len;
+ } else {
+ close(dirfd);
+ dirfd = -1;
+ }
+ }
+ top_fd = dirfd;
+ } while (dirfd >= 0 && strlen(dir_path));
+ free(full_path);
+ return top_fd;
+}
+
+
+/* Changes the mode of a file to the mode specified. Accepts chmod style
+ * comma-separated strings. Returns 0 on success, otherwise a positive errno
+ * value. This version partially implements the amplification hardness
+ * technique to detect (and occasionally prevent) some forms of TOCTTOU
+ * errors. In the case that the named file is a unix-domain socket, the
+ * containing directory for the socket should have restrictive permissions.
+ * EINVAL will be populated in errno if the final path atom is a symbolic link.
+ * It is recommended to use ovs_fchmod if the path is already opened. */
+int
+ovs_kchmod(const char *path, const char *mode)
+{
+ char *tmpdir = xstrdup(path);
+ char *tmppath = strrchr(tmpdir, '/');
+ int fd = -1, result = 0;
+ int dirfd, k;
+ char target_path[PATH_MAX];
+ struct stat st, s;
+ if (!tmppath) {
+ dirfd = directory_traverse(".", -1);
+ } else {
+ *tmppath++ = '\0';
+ dirfd = directory_traverse(tmpdir, -1);
+ }
+
+ if (dirfd == -1 || is_symlink(dirfd, tmppath, target_path, PATH_MAX, &s)) {
+ goto end;
+ }
+
+ mode_t new_mode;
+ errno = 0;
+ new_mode = chmod_getmode(mode, s.st_mode);
+ result = fchmodat(dirfd, tmppath, new_mode, 0);
+ /* need to reset 's' copy of st_mode, because it will have changed. */
+ s.st_mode = new_mode | (s.st_mode & ~(ALL_MODES));
+ for (k = 0; !errno && !result && k < K_ROUNDS; ++k) {
+ result = fstatat(dirfd, tmppath, &st, AT_SYMLINK_NOFOLLOW);
+ if (result) {
+ goto end;
+ }
+
+ if (!cmp_stat(&s, &st)) {
+ /* WARNING: In this case, a race means we modified an inode,
+ * and it may have been the wrong one. */
+ errno = EBADFD;
+ goto end;
+ }
+ }
+
+end:
+ if (errno) {
+ result = errno;
+ VLOG_ERR("ovs_kchown: (%s)", ovs_strerror(errno));
+ }
+ if (fd != -1) {
+ close(fd);
+ }
+ if (dirfd != -1) {
+ close(dirfd);
+ }
+ free(tmpdir);
+ return result;
+}
+
+
+/* Changes the mode of a file to the mode specified. Accepts chmod style
+ * comma-separated strings. Returns 0 on success, otherwise a positive errno
+ * value. This version partially implements the amplification hardness
+ * technique to detect (and occasionally prevent) some forms of TOCTTOU
+ * errors. In the case that the named file is a unix-domain socket, the
+ * containing directory for the socket should have restrictive permissions.
+ * EINVAL will be populated in errno if the final path atom is a symbolic
+ * link. It is recommended to use ovs_fchown if the file handle is already
+ * opened. */
+int
+ovs_kchown(const char *path, const char *usrstr)
+{
+ char *tmpdir = xstrdup(path);
+ char *tmppath = strrchr(tmpdir, '/');
+ int fd = -1, result = 0;
+ int dirfd, k;
+ char target_path[PATH_MAX];
+ struct stat st, s;
+ if (!tmppath) {
+ dirfd = directory_traverse(".", -1);
+ } else {
+ *tmppath++ = '\0';
+ dirfd = directory_traverse(tmpdir, -1);
+ }
+
+ if (dirfd == -1 || is_symlink(dirfd, tmppath, target_path, PATH_MAX, &s)) {
+ goto end;
+ }
+
+ uid_t user;
+ gid_t group;
+
+ if (ovs_strtousr(usrstr, &user, NULL, &group, true)) {
+ errno = EINVAL;
+ goto end;
+ }
+
+ errno = 0; /* clearing errno here is for the result assignment later. */
+ result = fchownat(dirfd, tmppath, user, group, 0);
+ for (k = 0; !errno && !result && k < K_ROUNDS; ++k) {
+ result = fstatat(dirfd, tmppath, &st, AT_SYMLINK_NOFOLLOW);
+ if (result) {
+ goto end;
+ }
+
+ if (!cmp_stat(&s, &st)) {
+ /* WARNING: In this case, a race means we modified an inode,
+ * and it may have been the wrong one. */
+ errno = EBADFD;
+ goto end;
+ }
+ }
+
+end:
+ if (errno) {
+ result = errno;
+ VLOG_ERR("ovs_kchown: (%s)", ovs_strerror(errno));
+ }
+
+ if (fd != -1) {
+ close(fd);
+ }
+ if (dirfd != -1) {
+ close(dirfd);
+ }
+ free(tmpdir);
+ return result;
+}
+
+
/* Changes the mode of a file to the mode specified. Accepts chmod style
* comma-separated strings. Returns 0 on success, otherwise a positive errno
* value. */
diff --git a/lib/chutil.h b/lib/chutil.h
index cdd4d52..100c8e6 100644
--- a/lib/chutil.h
+++ b/lib/chutil.h
@@ -25,6 +25,10 @@
#ifndef WIN32
int ovs_fchmod(int fd, const char *mode) OVS_WARN_UNUSED_RESULT;
int ovs_fchown(int fd, const char *usrstr) OVS_WARN_UNUSED_RESULT;
+
+int ovs_kchmod(const char *path, const char *mode) OVS_WARN_UNUSED_RESULT;
+int ovs_kchown(const char *path, const char *usrstr) OVS_WARN_UNUSED_RESULT;
+
int ovs_strtousr(const char *user_spec, uid_t *uid, char **user,
gid_t *gid, bool validate_user_group) OVS_WARN_UNUSED_RESULT;
#endif
diff --git a/tests/test-chutil.c b/tests/test-chutil.c
index dce07b4..9ba6b6b 100644
--- a/tests/test-chutil.c
+++ b/tests/test-chutil.c
@@ -26,12 +26,15 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
-#include <sys/types.h>
+#include <sys/socket.h>
#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/un.h>
#include <unistd.h>
#include "chutil.h"
#include "ovstest.h"
+#include "random.h"
#include "util.h"
static int
@@ -46,21 +49,54 @@ get_mode(const char *pathname, mode_t *mode)
}
static int
-with_temp_file(int (*fn)(const char *pathname, int fd))
+with_temp_file(int (*fn)(const char *pathname, int fd, bool usepath),
+ bool usepath)
{
char filepath[PATH_MAX] = "/tmp/test_chutil_wtfXXXXXX";
mode_t old_mask = umask(0777);
int fd = mkstemp(filepath);
umask(old_mask);
assert(fd >= 0);
- int result = fn(filepath, fd);
+ int result = fn(filepath, fd, usepath);
close(fd);
unlink(filepath);
return result;
}
static int
-run_chmod_bad_parsing(const char *pathname, int fd)
+with_temp_socket(int (*fn)(const char *pathname, int fd, bool usepath),
+ bool usepath)
+{
+ int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ mode_t old_mask = umask(0777);
+ char fname[PATH_MAX];
+ int result = -1;
+
+ assert(fd >= 0);
+
+ /* keep attempting to open a socket until success */
+ for (int kTimes = 0; kTimes < 10; ++kTimes) {
+ struct sockaddr_un addr;
+ snprintf(fname, PATH_MAX, "/tmp/test_chutil_socket%08X",
+ random_range(~0));
+ memset(&addr, 0, sizeof addr);
+ addr.sun_family = AF_UNIX;
+ snprintf((char *)&addr.sun_path, sizeof addr.sun_path, "%s", fname);
+ if (!bind(fd, (struct sockaddr *)&addr, sizeof addr)) {
+ result = fn(fname, fd, usepath);
+ goto done;
+ }
+ }
+ printf("E: Unable to open a socket after 10 attempts.\n");
+done:
+ umask(old_mask);
+ unlink(fname);
+ close(fd);
+ return result;
+}
+
+static int
+run_chmod_bad_parsing(const char *pathname, int fd, bool usepath)
{
static char users[] = "bcdefhijklmnpqrstvwxyz";
static char perms[] = "abcdefghijklmnopqtuvyz";
@@ -77,7 +113,9 @@ run_chmod_bad_parsing(const char *pathname, int fd)
char buf[256] = {0};
mode_t testmode;
snprintf(buf, sizeof(buf), "%c+rwx", *itest);
- if (!ovs_fchmod(fd, buf) || get_mode(pathname, &testmode)
+ int chmodresult = usepath ? ovs_kchmod(pathname, buf) :
+ ovs_fchmod(fd, buf);
+ if (!chmodresult || get_mode(pathname, &testmode)
|| testmode != pathmode) {
printf("F(%s)", buf);
return -1;
@@ -88,7 +126,9 @@ run_chmod_bad_parsing(const char *pathname, int fd)
char buf[256] = {0};
mode_t testmode;
snprintf(buf, sizeof(buf), "u+%c", *itest);
- if (!ovs_fchmod(fd, buf) || get_mode(pathname, &testmode)
+ int chmodresult = usepath ? ovs_kchmod(pathname, buf) :
+ ovs_fchmod(fd, buf);
+ if (!chmodresult || get_mode(pathname, &testmode)
|| testmode != pathmode) {
printf("F(%s)", buf);
return -1;
@@ -99,7 +139,9 @@ run_chmod_bad_parsing(const char *pathname, int fd)
char buf[256] = {0};
mode_t testmode;
snprintf(buf, sizeof(buf), "u%crw", *itest);
- if (!ovs_fchmod(fd, buf) || get_mode(pathname, &testmode)
+ int chmodresult = usepath ? ovs_kchmod(pathname, buf) :
+ ovs_fchmod(fd, buf);
+ if (!chmodresult || get_mode(pathname, &testmode)
|| testmode != pathmode) {
printf("F(%s)", buf);
return -1;
@@ -111,7 +153,7 @@ run_chmod_bad_parsing(const char *pathname, int fd)
/* Skip suid and sgid for now. */
static int
-run_chmod_str_successes(const char *pathname, int fd)
+run_chmod_str_successes(const char *pathname, int fd, bool usepath)
{
const char *users[] = { "u", "g", "o", "a", "ug", "uo", "go" };
const char *perms[] = { "r", "w", "x", "rw", "rx", "wx" };
@@ -127,13 +169,17 @@ run_chmod_str_successes(const char *pathname, int fd)
mode_t pathmode;
char buf[256] = {0};
snprintf(buf, sizeof(buf), "%s+%s", users[iusers], perms[iperms]);
- if (ovs_fchmod(fd, buf) || get_mode(pathname, &pathmode)) {
+ int chmodresult = usepath ? ovs_kchmod(pathname, buf) :
+ ovs_fchmod(fd, buf);
+ if (chmodresult || get_mode(pathname, &pathmode)) {
printf("run_chmod_successes:E(%s)\n", buf);
return -1;
}
/* XXX: Check the actual mode here */
snprintf(buf, sizeof(buf), "%s-%s", users[iusers], perms[iperms]);
- if (ovs_fchmod(fd, buf) || get_mode(pathname, &pathmode)
+ chmodresult = usepath ? ovs_kchmod(pathname, buf) :
+ ovs_fchmod(fd, buf);
+ if (chmodresult || get_mode(pathname, &pathmode)
|| pathmode != chkmode) {
printf("run_chmod_successes:F(%s:%x:%x)\n", buf, pathmode,
chkmode);
@@ -143,13 +189,16 @@ run_chmod_str_successes(const char *pathname, int fd)
}
mode_t pmode;
- if (ovs_fchmod(fd, "u-rwx,g-rwx,o-rwx")
- || get_mode(pathname, &pmode) || pmode != 0) {
+ int chmodchange = usepath ? ovs_kchmod(pathname, "u-rwx,g-rwx,o-rwx") :
+ ovs_fchmod(fd, "u-rwx,g-rwx,o-rwx");
+ if (chmodchange || get_mode(pathname, &pmode) || pmode != 0) {
printf("run_chmod_successes:csvF\n");
return -1;
}
- if (ovs_fchmod(fd, "u=rx,g=w") || get_mode(pathname, &pmode)
+ chmodchange = usepath ? ovs_kchmod(pathname, "u=rx,g=w") :
+ ovs_fchmod(fd, "u=rx,g=w");
+ if (chmodchange || get_mode(pathname, &pmode)
|| pmode != (S_IRUSR | S_IXUSR | S_IWGRP)) {
printf("run_chmod_successes:assignF\n");
return -1;
@@ -158,7 +207,7 @@ run_chmod_str_successes(const char *pathname, int fd)
}
static int
-run_chmod_numeric_successes(const char *pathname, int fd)
+run_chmod_numeric_successes(const char *pathname, int fd, bool usepath)
{
const char *modestrs[] = {"0755", "0644", "0600", "11", "20", "755",
"640"};
@@ -174,8 +223,9 @@ run_chmod_numeric_successes(const char *pathname, int fd)
size_t imodes;
for (imodes = 0; imodes < ARRAY_SIZE(modestrs); ++imodes) {
mode_t newmode;
- if (ovs_fchmod(fd, modestrs[imodes]) ||
- get_mode(pathname, &newmode)) {
+ int chmodresult = usepath ? ovs_kchmod(pathname, modestrs[imodes]) :
+ ovs_fchmod(fd, modestrs[imodes]);
+ if (chmodresult || get_mode(pathname, &newmode)) {
printf("run_chmod_numeric_successes:F(%s)\n", modestrs[imodes]);
return -1;
}
@@ -232,9 +282,13 @@ run_ovs_strtouser_failures(void)
static void
test_chutil_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
{
- assert(!with_temp_file(run_chmod_bad_parsing));
- assert(!with_temp_file(run_chmod_str_successes));
- assert(!with_temp_file(run_chmod_numeric_successes));
+ assert(!with_temp_file(run_chmod_bad_parsing, false));
+ assert(!with_temp_file(run_chmod_str_successes, false));
+ assert(!with_temp_file(run_chmod_numeric_successes, false));
+ assert(!with_temp_file(run_chmod_str_successes, true));
+ assert(!with_temp_file(run_chmod_numeric_successes, true));
+ assert(!with_temp_socket(run_chmod_str_successes, true));
+ assert(!with_temp_socket(run_chmod_numeric_successes, true));
assert(!run_ovs_strtouser_successes());
assert(!run_ovs_strtouser_failures());
printf("\n");
--
2.5.5
More information about the dev
mailing list