From c58a6d53c58209a8f78ff64e04e9112933489fb5 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Mon, 29 Feb 2016 15:24:35 +0000 Subject: [PATCH] YARN-4731. container-executor should not follow symlinks in recursive_unlink_children. Contributed by Colin Patrick McCabe --- hadoop-yarn-project/CHANGES.txt | 3 + .../impl/container-executor.c | 54 +++++++++- .../test/test-container-executor.c | 99 ++++++++++++++++++- 3 files changed, 153 insertions(+), 3 deletions(-) diff --git a/hadoop-yarn-project/CHANGES.txt b/hadoop-yarn-project/CHANGES.txt index 3c91fbd7639..27eff2d4828 100644 --- a/hadoop-yarn-project/CHANGES.txt +++ b/hadoop-yarn-project/CHANGES.txt @@ -239,6 +239,9 @@ Release 2.9.0 - UNRELEASED YARN-4566. Fix test failure in TestMiniYarnClusterNodeUtilization. (Takashi Ohnishi via rohithsharmaks) + YARN-4731. container-executor should not follow symlinks in + recursive_unlink_children (Colin Patrick McCabe via jlowe) + Release 2.8.0 - UNRELEASED INCOMPATIBLE CHANGES diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c index 4bc8c787bf1..44de2bb335b 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c @@ -1579,9 +1579,9 @@ static int rmdir_as_nm(const char* path) { static int open_helper(int dirfd, const char *name) { int fd; if (dirfd >= 0) { - fd = openat(dirfd, name, O_RDONLY); + fd = openat(dirfd, name, O_RDONLY | O_NOFOLLOW); } else { - fd = open(name, O_RDONLY); + fd = open(name, O_RDONLY | O_NOFOLLOW); } if (fd >= 0) { return fd; @@ -1615,6 +1615,34 @@ static int unlink_helper(int dirfd, const char *name, int flags) { return errno; } +/** + * Determine if an entry in a directory is a symlink. + * + * @param dirfd The directory file descriptor, or -1 if there is none. + * @param name If dirfd is -1, this is the path to examine. + * Otherwise, this is the file name in the directory to + * examine. + * + * @return 0 if the entry is not a symlink + * 1 if the entry is a symlink + * A negative errno code if we couldn't access the entry. + */ +static int is_symlink_helper(int dirfd, const char *name) +{ + struct stat stat; + + if (dirfd < 0) { + if (lstat(name, &stat) < 0) { + return -errno; + } + } else { + if (fstatat(dirfd, name, &stat, AT_SYMLINK_NOFOLLOW) < 0) { + return -errno; + } + } + return !!S_ISLNK(stat.st_mode); +} + static int recursive_unlink_helper(int dirfd, const char *name, const char* fullpath) { @@ -1622,6 +1650,28 @@ static int recursive_unlink_helper(int dirfd, const char *name, DIR *dfd = NULL; struct stat stat; + // Check to see if the file is a symlink. If so, delete the symlink rather + // than what it points to. + ret = is_symlink_helper(dirfd, name); + if (ret < 0) { + // is_symlink_helper failed. + ret = -ret; + fprintf(LOGFILE, "is_symlink_helper(%s) failed: %s\n", + fullpath, strerror(ret)); + goto done; + } else if (ret == 1) { + // is_symlink_helper determined that the path is a symlink. + ret = unlink_helper(dirfd, name, 0); + if (ret) { + fprintf(LOGFILE, "failed to unlink symlink %s: %s\n", + fullpath, strerror(ret)); + } + goto done; + } + + // Open the file. We use O_NOFOLLOW here to ensure that we if a symlink was + // swapped in by an attacker, we will fail to follow it rather than deleting + // something we potentially should not. fd = open_helper(dirfd, name); if (fd == -EACCES) { ret = chmod_helper(dirfd, name, 0700); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c index b16c6bcf6ed..ab29543ee81 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c @@ -750,6 +750,100 @@ void test_run_container() { check_pid_file(cgroups_pids[1], child); } +static void mkdir_or_die(const char *path) { + if (mkdir(path, 0777) < 0) { + int err = errno; + printf("mkdir(%s) failed: %s\n", path, strerror(err)); + exit(1); + } +} + +static void touch_or_die(const char *path) { + FILE* f = fopen(path, "w"); + if (!f) { + int err = errno; + printf("fopen(%s, w) failed: %s\n", path, strerror(err)); + exit(1); + } + if (fclose(f) < 0) { + int err = errno; + printf("fclose(%s) failed: %s\n", path, strerror(err)); + exit(1); + } +} + +static void symlink_or_die(const char *old, const char *new) { + if (symlink(old, new) < 0) { + int err = errno; + printf("symlink(%s, %s) failed: %s\n", old, new, strerror(err)); + exit(1); + } +} + +static void expect_type(const char *path, int mode) { + struct stat st_buf; + + if (stat(path, &st_buf) < 0) { + int err = errno; + if (err == ENOENT) { + if (mode == 0) { + return; + } + printf("expect_type(%s): stat failed unexpectedly: %s\n", + path, strerror(err)); + exit(1); + } + } + if (mode == 0) { + printf("expect_type(%s): we expected the file to be gone, but it " + "existed.\n", path); + exit(1); + } + if (!(st_buf.st_mode & mode)) { + printf("expect_type(%s): the file existed, but it had mode 0%4o, " + "which didn't have bit 0%4o\n", path, st_buf.st_mode, mode); + exit(1); + } +} + +int recursive_unlink_children(const char *name); + +void test_recursive_unlink_children() { + int ret; + + mkdir_or_die(TEST_ROOT "/unlinkRoot"); + mkdir_or_die(TEST_ROOT "/unlinkRoot/a"); + touch_or_die(TEST_ROOT "/unlinkRoot/b"); + mkdir_or_die(TEST_ROOT "/unlinkRoot/c"); + touch_or_die(TEST_ROOT "/unlinkRoot/c/d"); + touch_or_die(TEST_ROOT "/external"); + symlink_or_die(TEST_ROOT "/external", + TEST_ROOT "/unlinkRoot/c/external"); + ret = recursive_unlink_children(TEST_ROOT "/unlinkRoot"); + if (ret != 0) { + printf("recursive_unlink_children(%s) failed: error %d\n", + TEST_ROOT "/unlinkRoot", ret); + exit(1); + } + // unlinkRoot should still exist. + expect_type(TEST_ROOT "/unlinkRoot", S_IFDIR); + // Other files under unlinkRoot should have been deleted. + expect_type(TEST_ROOT "/unlinkRoot/a", 0); + expect_type(TEST_ROOT "/unlinkRoot/b", 0); + expect_type(TEST_ROOT "/unlinkRoot/c", 0); + // We shouldn't have followed the symlink. + expect_type(TEST_ROOT "/external", S_IFREG); + // Clean up. + if (rmdir(TEST_ROOT "/unlinkRoot") < 0) { + int err = errno; + printf("failed to rmdir " TEST_ROOT "/unlinkRoot: %s\n", strerror(err)); + } + if (unlink(TEST_ROOT "/external") < 0) { + int err = errno; + printf("failed to unlink " TEST_ROOT "/external: %s\n", strerror(err)); + } +} + // This test is expected to be executed either by a regular // user or by root. If executed by a regular user it doesn't // test all the functions that would depend on changing the @@ -775,7 +869,7 @@ int main(int argc, char **argv) { if (mkdirs(TEST_ROOT "/logs/userlogs", 0755) != 0) { exit(1); } - + if (write_config_file(TEST_ROOT "/test.cfg", 1) != 0) { exit(1); } @@ -804,6 +898,9 @@ int main(int argc, char **argv) { printf("\nStarting tests\n"); + printf("\nTesting recursive_unlink_children()\n"); + test_recursive_unlink_children(); + printf("\nTesting resolve_config_path()\n"); test_resolve_config_path();