YARN-4731. container-executor should not follow symlinks in recursive_unlink_children. Contributed by Colin Patrick McCabe

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

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);

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
@@ -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();