]> gitweb.michael.orlitzky.com - apply-default-acl.git/commitdiff
Replace most path usage with file descriptors.
authorMichael Orlitzky <michael@orlitzky.com>
Fri, 23 Feb 2018 21:11:08 +0000 (16:11 -0500)
committerMichael Orlitzky <michael@orlitzky.com>
Mon, 26 Feb 2018 19:10:14 +0000 (14:10 -0500)
Before this commit, we were passing around paths everywhere to specify
the targets of operations. This is not optimal from a security
standpoint: the best we can do to avoid following hard links is to
check whether or not a given file has more than one name. There is a
race condition inherent in that approach -- between when you stat the
file and when you use it, the number of names may change -- but using
paths makes that window larger than it has to be. There's no guarantee
that a path used at the bottom of apply_default_acl() will refer to
the same file that we called stat() on at the top of the function.

To work around that, most of the path handling functions have been
replaced with versions that use file descriptors. Now we are able to
stat() our file descriptor immediately after opening it, and the
descriptor itself will always refer to the same file. There's still
the smallest of windows for an exploit, but this makes it much safer
to call apply_default_acl() when there may be hard links present.

src/apply-default-acl.c

index 4a6ef765e10379d213661dacf39d8a08636cb20c..d97d67d25fed90d5b053652f2dcf2cb39867ea10 100644 (file)
@@ -13,8 +13,7 @@
 #include <fcntl.h>  /* AT_FOO constants */
 #include <ftw.h>    /* nftw() et al. */
 #include <getopt.h>
-#include <libgen.h> /* dirname()     */
-#include <limits.h> /* PATH_MAX      */
+#include <libgen.h> /* basename(), dirname() */
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 
 /**
- * @brief Get the mode bits from the given path.
+ * @brief Get the mode bits from the given file descriptor.
  *
- * @param path
- *   The path (file or directory) whose mode we want.
+ * @param fd
+ *   The file descriptor (which may reference a directory) whose
+ *   mode we want.
  *
  * @return A mode_t (st_mode) structure containing the mode bits.
  *   See sys/stat.h for details.
  */
-mode_t get_mode(const char* path) {
-  if (path == NULL) {
+mode_t get_mode(int fd) {
+  if (fd <= 0) {
     errno = ENOENT;
-    return -1;
+    return ACL_ERROR;
   }
 
   struct stat s;
-  int result = lstat(path, &s);
+  int result = fstat(fd, &s);
 
   if (result == 0) {
     return s.st_mode;
@@ -65,21 +65,22 @@ mode_t get_mode(const char* path) {
 
 
 /**
- * @brief Determine if the given path might refer to an (unsafe) hard link.
+ * @brief Determine if the given file descriptor might refer to an
+ *   (unsafe) hard link.
  *
- * @param path
- *   The path to test.
+ * @param fd
+ *   The file descriptor whose link count we want to investigate.
  *
- * @return true if we are certain that @c path does not refer to a hard
+ * @return true if we are certain that @c fd does not describe a hard
  *   link, and false otherwise. In case of error, false is returned,
- *   because we are not sure that @c path is not a hard link.
+ *   because we are not sure that @c fd is not a hard link.
  */
-bool is_hardlink_safe(const char* path) {
-  if (path == NULL) {
+bool is_hardlink_safe(int fd) {
+  if (fd <= 0) {
     return false;
   }
   struct stat s;
-  int result = lstat(path, &s);
+  int result = fstat(fd, &s);
   if (result == 0) {
     return (s.st_nlink == 1 || S_ISDIR(s.st_mode));
   }
@@ -90,20 +91,21 @@ bool is_hardlink_safe(const char* path) {
 
 
 /**
- * @brief Determine whether or not the given path is a regular file.
+ * @brief Determine whether or not the given file descriptor is for
+ *   a regular file.
  *
- * @param path
- *   The path to test.
+ * @param fd
+ *   The file descriptor to test for regular-fileness.
  *
- * @return true if @c path is a regular file, false otherwise.
+ * @return true if @c fd describes a regular file, and false otherwise.
  */
-bool is_regular_file(const char* path) {
-  if (path == NULL) {
+bool is_regular_file(int fd) {
+  if (fd <= 0) {
     return false;
   }
 
   struct stat s;
-  int result = lstat(path, &s);
+  int result = fstat(fd, &s);
   if (result == 0) {
     return S_ISREG(s.st_mode);
   }
@@ -174,6 +176,31 @@ bool is_path_directory(const char* path) {
 }
 
 
+/**
+ * @brief Determine whether or not the given file descriptor is for
+ *   a directory.
+ *
+ * @param fd
+ *   The file descriptor whose directoryness is in question.
+ *
+ * @return true if @c fd describes a directory, and false otherwise.
+ */
+bool is_directory(int fd) {
+  if (fd <= 0) {
+    return false;
+  }
+
+  struct stat s;
+  int result = fstat(fd, &s);
+  if (result == 0) {
+    return S_ISDIR(s.st_mode);
+  }
+  else {
+    return false;
+  }
+}
+
+
 
 /**
  * @brief Update (or create) an entry in an @b minimal ACL.
@@ -433,35 +460,35 @@ int acl_execute_masked(acl_t acl) {
 
 
 /**
- * @brief Determine whether @c path is executable (by anyone) or a
+ * @brief Determine whether @c fd is executable (by anyone) or a
  *   directory.
  *
  * This is used as part of the heuristic to determine whether or not
- * we should mask the execute bit when inheriting an ACL. If @c path
- * is a directory, the answer is a clear-cut yes. This behavior is
- * modeled after the capital 'X' perms of setfacl.
+ * we should mask the execute bit when inheriting an ACL. If @c fd
+ * describes a directory, the answer is a clear-cut yes. This behavior
+ * is modeled after the capital 'X' perms of setfacl.
  *
- * If @c path is a file, we check the @a effective permissions,
+ * If @c fd describes a file, we check the @a effective permissions,
  * contrary to what setfacl does.
  *
- * @param path
- *   The path to check.
+ * @param fd
+ *   The file descriptor to check.
  *
  * @return
- *   - @c ACL_SUCCESS - @c path is a directory, or someone has effective
+ *   - @c ACL_SUCCESS - @c fd describes a directory, or someone has effective
        execute permissions.
- *   - @c ACL_FAILURE - @c path is a regular file and nobody can execute
+ *   - @c ACL_FAILURE - @c fd describes a regular file and nobody can execute
        it.
  *   - @c ACL_ERROR - Unexpected library error.
  */
-int any_can_execute_or_dir(const char* path) {
+int any_can_execute_or_dir(int fd) {
 
-  if (is_path_directory(path)) {
+  if (is_directory(fd)) {
     /* That was easy... */
     return ACL_SUCCESS;
   }
 
-  acl_t acl = acl_get_file(path, ACL_TYPE_ACCESS);
+  acl_t acl = acl_get_fd(fd);
 
   if (acl == (acl_t)NULL) {
     perror("any_can_execute_or_dir (acl_get_file)");
@@ -472,7 +499,7 @@ int any_can_execute_or_dir(const char* path) {
   int result = ACL_FAILURE;
 
   if (acl_is_minimal(acl)) {
-    mode_t mode = get_mode(path);
+    mode_t mode = get_mode(fd);
     if (mode & (S_IXUSR | S_IXOTH | S_IXGRP)) {
       result = ACL_SUCCESS;
       goto cleanup;
@@ -578,13 +605,13 @@ int assign_default_acl(const char* path, acl_t acl) {
   acl_t path_acl = acl_dup(acl);
 
   if (path_acl == (acl_t)NULL) {
-    perror("inherit_default_acl (acl_dup)");
+    perror("assign_default_acl (acl_dup)");
     return ACL_ERROR; /* Nothing to clean up in this case. */
   }
 
   int sf_result = acl_set_file(path, ACL_TYPE_DEFAULT, path_acl);
-  if (sf_result == -1) {
-    perror("inherit_default_acl (acl_set_file)");
+  if (sf_result == ACL_ERROR) {
+    perror("assign_default_acl (acl_set_file)");
     result = ACL_ERROR;
   }
 
@@ -596,26 +623,26 @@ int assign_default_acl(const char* path, acl_t acl) {
 
 /**
  * @brief Remove @c ACL_USER, @c ACL_GROUP, and @c ACL_MASK entries
- *   from the given path.
+ *   from the given file descriptor.
  *
- * @param path
- *   The path whose ACLs we want to wipe.
+ * @param fd
+ *   The file descriptor whose ACLs we want to wipe.
  *
  * @return
  *   - @c ACL_SUCCESS - The ACLs were wiped successfully, or none
  *     existed in the first place.
  *   - @c ACL_ERROR - Unexpected library error.
  */
-int wipe_acls(const char* path) {
+int wipe_acls(int fd) {
 
-  if (path == NULL) {
+  if (fd <= 0) {
     errno = ENOENT;
     return ACL_ERROR;
   }
 
-  acl_t acl = acl_get_file(path, ACL_TYPE_ACCESS);
+  acl_t acl = acl_get_fd(fd);
   if (acl == (acl_t)NULL) {
-    perror("wipe_acls (acl_get_file)");
+    perror("wipe_acls (acl_get_fd)");
     return ACL_ERROR;
   }
 
@@ -644,9 +671,9 @@ int wipe_acls(const char* path) {
     goto cleanup;
   }
 
-  int sf_result = acl_set_file(path, ACL_TYPE_ACCESS, acl);
+  int sf_result = acl_set_fd(fd, acl);
   if (sf_result == ACL_ERROR) {
-    perror("wipe_acls (acl_set_file)");
+    perror("wipe_acls (acl_set_fd)");
     result = ACL_ERROR;
     goto cleanup;
   }
@@ -682,45 +709,59 @@ int apply_default_acl(const char* path, bool no_exec_mask) {
     return ACL_ERROR;
   }
 
-  /* Refuse to operate on hard links, which can be abused by an
-   * attacker to trick us into changing the ACL on a file we didn't
-   * intend to; namely the "target" of the hard link. To truly prevent
-   * that sort of mischief, we should be using file descriptors for
-   * the target and its parent directory. Then modulo a tiny race
-   * condition, we would be sure that "path" and "parent" don't change
-   * their nature between the time that we test them and when we
-   * utilize them. For contrast, the same attacker is free to replace
-   * "path" with a hard link after is_hardlink_safe() has returned
-   * "true" below.
-   *
-   * Unfortunately, our API is lacking in this area. For example,
-   * acl_set_fd() is only capable of setting the ACL_TYPE_ACCESS list,
-   * and not the ACL_TYPE_DEFAULT. Apparently the only way to operate
-   * on default ACLs is through the path name, which is inherently
-   * unreliable since the acl_*_file() calls themselves might follow
-   * links (both hard and symbolic).
-   *
-   * Some improvement could still be made by using descriptors where
-   * possible -- this would shrink the exploit window -- but for now
-   * we use a naive implementation that only keeps honest men honest.
-  */
-  if (!is_hardlink_safe(path)) {
-    return ACL_FAILURE;
+  /* Define these next three variables here because we may have to
+   * jump to the cleanup routine which expects them to exist.
+   */
+
+  /* Our return value. */
+  int result = ACL_SUCCESS;
+
+  /* The default ACL on path's parent directory */
+  acl_t defacl = (acl_t)NULL;
+
+  /* The file descriptor corresponding to "path" */
+  int fd = 0;
+
+  /* Split "path" into base/dirname parts to be used with openat().
+   * We duplicate the strings involved because dirname/basename mangle
+   * their arguments.
+   */
+  char* path_copy = strdup(path);
+  if (path_copy == NULL) {
+    perror("apply_default_acl (strdup)");
+    return ACL_ERROR;
   }
+  char* parent = dirname(path_copy);
 
-  if (!is_regular_file(path) && !is_path_directory(path)) {
-    return ACL_FAILURE;
+  fd = open(path, O_NOFOLLOW);
+  if (fd == -1) {
+    if (errno == ELOOP) {
+      result = ACL_FAILURE; /* hit a symlink */
+      goto cleanup;
+    }
+    else {
+      perror("apply_default_acl (open fd)");
+      result = ACL_ERROR;
+      goto cleanup;
+    }
   }
 
-  /* dirname mangles its argument */
-  char path_copy[PATH_MAX];
-  strncpy(path_copy, path, PATH_MAX-1);
-  path_copy[PATH_MAX-1] = 0;
 
-  char* parent = dirname(path_copy);
-  if (!is_path_directory(parent)) {
-    /* Make sure dirname() did what we think it did. */
-    return ACL_FAILURE;
+  /* Refuse to operate on hard links, which can be abused by an
+   * attacker to trick us into changing the ACL on a file we didn't
+   * intend to; namely the "target" of the hard link. There is TOCTOU
+   * race condition here, but the window is as small as possible
+   * between when we open the file descriptor (look above) and when we
+   * fstat it.
+  */
+  if (!is_hardlink_safe(fd)) {
+    result = ACL_FAILURE;
+    goto cleanup;
+  }
+
+  if (!is_regular_file(fd) && !is_directory(fd)) {
+    result = ACL_FAILURE;
+    goto cleanup;
   }
 
   /* Default to not masking the exec bit; i.e. applying the default
@@ -729,27 +770,26 @@ int apply_default_acl(const char* path, bool no_exec_mask) {
   bool allow_exec = true;
 
   if (!no_exec_mask) {
-    int ace_result = any_can_execute_or_dir(path);
+    int ace_result = any_can_execute_or_dir(fd);
 
     if (ace_result == ACL_ERROR) {
       perror("apply_default_acl (any_can_execute_or_dir)");
-      return ACL_ERROR;
+      result = ACL_ERROR;
+      goto cleanup;
     }
 
     allow_exec = (bool)ace_result;
   }
 
-  acl_t defacl = acl_get_file(parent, ACL_TYPE_DEFAULT);
+  defacl = acl_get_file(parent, ACL_TYPE_DEFAULT);
 
   if (defacl == (acl_t)NULL) {
     perror("apply_default_acl (acl_get_file)");
-    return ACL_ERROR;
+    result = ACL_ERROR;
+    goto cleanup;
   }
 
-  /* Our return value. */
-  int result = ACL_SUCCESS;
-
-  int wipe_result = wipe_acls(path);
+  int wipe_result = wipe_acls(fd);
   if (wipe_result == ACL_ERROR) {
     perror("apply_default_acl (wipe_acls)");
     result = ACL_ERROR;
@@ -758,9 +798,9 @@ int apply_default_acl(const char* path, bool no_exec_mask) {
 
   /* Do this after wipe_acls(), otherwise we'll overwrite the wiped
      ACL with this one. */
-  acl_t acl = acl_get_file(path, ACL_TYPE_ACCESS);
+  acl_t acl = acl_get_fd(fd);
   if (acl == (acl_t)NULL) {
-    perror("apply_default_acl (acl_get_file)");
+    perror("apply_default_acl (acl_get_fd)");
     result = ACL_ERROR;
     goto cleanup;
   }
@@ -768,7 +808,7 @@ int apply_default_acl(const char* path, bool no_exec_mask) {
   /* If it's a directory, inherit the parent's default. */
   int inherit_result = assign_default_acl(path, defacl);
   if (inherit_result == ACL_ERROR) {
-    perror("apply_default_acl (inherit_acls)");
+    perror("apply_default_acl (assign_default_acl)");
     result = ACL_ERROR;
     goto cleanup;
   }
@@ -856,15 +896,22 @@ int apply_default_acl(const char* path, bool no_exec_mask) {
     goto cleanup;
   }
 
-  int sf_result = acl_set_file(path, ACL_TYPE_ACCESS, acl);
+  int sf_result = acl_set_fd(fd, acl);
   if (sf_result == ACL_ERROR) {
-    perror("apply_default_acl (acl_set_file)");
+    perror("apply_default_acl (acl_set_fd)");
     result = ACL_ERROR;
     goto cleanup;
   }
 
  cleanup:
-  acl_free(defacl);
+  free(path_copy);
+  if (defacl != (acl_t)NULL) {
+    acl_free(defacl);
+  }
+  if (fd >= 0 && close(fd) == -1) {
+    perror("apply_default_acl (close)");
+    result = ACL_ERROR;
+  }
   return result;
 }