You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-commits@hadoop.apache.org by ar...@apache.org on 2016/03/04 02:01:54 UTC
[15/50] [abbrv] hadoop git commit: YARN-4731. container-executor
should not follow symlinks in recursive_unlink_children. Contributed by Colin
Patrick McCabe
YARN-4731. container-executor should not follow symlinks in recursive_unlink_children. Contributed by Colin Patrick McCabe
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo
Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/c58a6d53
Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/c58a6d53
Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/c58a6d53
Branch: refs/heads/HDFS-1312
Commit: c58a6d53c58209a8f78ff64e04e9112933489fb5
Parents: 8bc023b
Author: Jason Lowe <jl...@apache.org>
Authored: Mon Feb 29 15:24:35 2016 +0000
Committer: Jason Lowe <jl...@apache.org>
Committed: Mon Feb 29 15:24:35 2016 +0000
----------------------------------------------------------------------
hadoop-yarn-project/CHANGES.txt | 3 +
.../impl/container-executor.c | 54 ++++++++++-
.../test/test-container-executor.c | 99 +++++++++++++++++++-
3 files changed, 153 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hadoop/blob/c58a6d53/hadoop-yarn-project/CHANGES.txt
----------------------------------------------------------------------
diff --git a/hadoop-yarn-project/CHANGES.txt b/hadoop-yarn-project/CHANGES.txt
index 3c91fbd..27eff2d 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
http://git-wip-us.apache.org/repos/asf/hadoop/blob/c58a6d53/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c
----------------------------------------------------------------------
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 4bc8c78..44de2bb 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);
http://git-wip-us.apache.org/repos/asf/hadoop/blob/c58a6d53/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c
----------------------------------------------------------------------
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 b16c6bc..ab29543 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();