From: Michael Orlitzky <michael@orlitzky.com>
Date: Thu, 22 Feb 2018 23:00:11 +0000 (-0500)
Subject: Naively ignore hard links to avoid security mishaps.
X-Git-Tag: v0.1.0~39
X-Git-Url: https://gitweb.michael.orlitzky.com/?a=commitdiff_plain;h=e5d36040a86457ed66782496d79136d6bdc4d9a3;p=apply-default-acl.git

Naively ignore hard links to avoid security mishaps.

If an attacker can introduce a hard link into a directory with a
default ACL, then he may be able to trick the user into applying that
default ACL to the target of the hard link which lives somewhere else
entirely. That can be exploited to gain access to the user's files,
and is hard to detect.

To avoid that problem entirely, great care must be taken. For now, a
naive check of the target path is implemented to ensure that (at the
start of the routine) it has only one name on the filesystem. This
still admits a race condition, but is an improvement.

The new behavior is now documented in the man page, and a test has
been added to ensure that pre-existing hard links are ignored.
---

diff --git a/doc/man/apply-default-acl.1 b/doc/man/apply-default-acl.1
index 75c7a75..bfbb724 100644
--- a/doc/man/apply-default-acl.1
+++ b/doc/man/apply-default-acl.1
@@ -11,8 +11,8 @@ apply-default-acl \- Apply default POSIX ACLs to files and directories.
 
 .P
 If the directory containing \fIpath\fR has a default ACL, the ACL on
-\fIpath\fR is replaced with that default. Symbolic links are not
-followed.
+\fIpath\fR is replaced with that default. Neither symbolic nor hard
+links are followed.
 
 .P
 By default, a heuristic is used to determine whether or not the
diff --git a/run-tests.sh b/run-tests.sh
index d055560..7f7ba8f 100755
--- a/run-tests.sh
+++ b/run-tests.sh
@@ -730,3 +730,22 @@ other::r--
 EOF
 )
 compare
+
+
+# Ensure that hard links are ignored.
+TESTNUM=30
+TARGET="${TESTDIR}/foo"
+LINK2TARGET="${TESTDIR}/bar"
+touch "${TARGET}"
+ln "${TARGET}" "${LINK2TARGET}"
+setfacl --default --modify user:${USERS[0]}:rwx "${TESTDIR}"
+"${BIN}" "${LINK2TARGET}"
+ACTUAL=$( getfacl --omit-header "${TARGET}" )
+EXPECTED=$(cat <<EOF
+user::rw-
+group::r--
+other::r--
+
+EOF
+)
+compare
diff --git a/src/apply-default-acl.c b/src/apply-default-acl.c
index 1950528..b89a732 100644
--- a/src/apply-default-acl.c
+++ b/src/apply-default-acl.c
@@ -64,6 +64,31 @@ mode_t get_mode(const char* path) {
 
 
 
+/**
+ * @brief Determine if the given path might refer to an (unsafe) hard link.
+ *
+ * @param path
+ *   The path to test.
+ *
+ * @return true if we are certain that @c path does not refer to 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.
+ */
+bool is_hardlink_safe(const char* path) {
+  if (path == NULL) {
+    return false;
+  }
+  struct stat s;
+  int result = lstat(path, &s);
+  if (result == 0) {
+    return (s.st_nlink == 1 || S_ISDIR(s.st_mode));
+  }
+  else {
+    return false;
+  }
+}
+
+
 /**
  * @brief Determine whether or not the given path is a regular file.
  *
@@ -678,6 +703,32 @@ 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;
+  }
+
   if (!is_regular_file(path) && !is_directory(path)) {
     return ACL_FAILURE;
   }