Re: [PATCH v2] fs: hardlink creation restrictions

On Tue, Feb 21, 2012 at 3:10 PM, Andrew Morton
[off-list ref] wrote:

On Tue, 21 Feb 2012 13:58:00 -0800
Kees Cook [off-list ref] wrote:

quoted

On systems that have user-writable directories on the same partition
as system files, a long-standing class of security issues is the
hardlink-based time-of-check-time-of-use race, most commonly seen in
world-writable directories like /tmp. The common method of exploitation
of this flaw is to cross privilege boundaries when following a given
hardlink (i.e. a root process follows a hardlink created by another
user). Additionally, an issue exists where users can "pin" a potentially
vulnerable setuid/setgid file so that an administrator will not actually
upgrade a system fully.

The solution is to permit hardlinks to only be created when the user is
already the existing file's owner, or if they already have read/write
access to the existing file.

Many Linux users are surprised when they learn they can link to files
they have no access to, so this change appears to follow the doctrine
of "least surprise". Additionally, this change does not violate POSIX,
which states "the implementation may require that the calling process
has permission to access the existing file"[1].

This change is known to break some implementations of the "at" daemon,
though the version used by Fedora and Ubuntu has been fixed[2] for
a while. Otherwise, the change has been undisruptive while in use in
Ubuntu for the last 1.5 years.

This patch is based on the patch in Openwall and grsecurity.  I have
added a sysctl to enable the protected behavior, documentation, and an
audit notification.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html
[2] http://anonscm.debian.org/gitweb/?p=collab-maint/at.git;a=commitdiff;h=f4114656c3a6c6f6070e315ffdf940a49eda3279

Looks OKish to me.

Thanks!

quoted

--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt

@@ -32,7 +32,8 @@ Currently, these files are in /proc/sys/fs:

 - nr_open
 - overflowuid
 - overflowgid
-- protected_sticky_symlinks
+- protected_hardlinks
+- protected_symlinks

It's silly to add protected_sticky_symlinks and to then rename it in
the very next patch.  I have reworked my copy of the earlier
fs-symlink-restrictions-on-sticky-directories.patch so that it adds
"protected_symlinks".  I then reworked this patch
(fs-hardlink-creation-restrictions.patch) to add protected_hardlinks.
Nice and simple.

Okay, sounds good. Thanks for the clean-ups!

quoted

...

+static inline void
+audit_log_link_denied(const char *operation, struct path *link)
+{
+     struct audit_buffer *ab;
+
+     ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_AVC);
+     audit_log_format(ab, "op=%s action=denied", operation);
+     audit_log_format(ab, " pid=%d comm=", current->pid);
+     audit_log_untrustedstring(ab, current->comm);
+     audit_log_d_path(ab, " path=", link);
+     audit_log_format(ab, " dev=");
+     audit_log_untrustedstring(ab, link->dentry->d_inode->i_sb->s_id);
+     audit_log_format(ab, " ino=%lu", link->dentry->d_inode->i_ino);
+     audit_log_end(ab);
+}

This is far too large to inline.  It has two callsites and gcc is
probably smart anough to uninline it for us.  I queued a patch to
remove the `inline'.

Okay, seems fine to me.

quoted

 /**
  * may_follow_link - Check symlink following for unsafe situations
- * @dentry: The inode/dentry of the symlink
- * @nameidata: The path data of the symlink
+ * @link: The path of the symlink
  *
- * In the case of the protected_sticky_symlinks sysctl being enabled,
+ * In the case of the sysctl_protected_symlinks sysctl being enabled,
  * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
  * in a sticky world-writable directory. This is to protect privileged
  * processes from failing races against path names that may change out

@@ -643,19 +660,20 @@ int sysctl_protected_sticky_symlinks __read_mostly =

  *
  * Returns 0 if following the symlink is allowed, -ve on error.
  */
-static inline int
-may_follow_link(struct dentry *dentry, struct nameidata *nameidata)
+static inline int may_follow_link(struct path *link)
 {
      int error = 0;
      const struct inode *parent;
      const struct inode *inode;
      const struct cred *cred;
+     struct dentry *dentry;

-     if (!sysctl_protected_sticky_symlinks)
+     if (!sysctl_protected_symlinks)
              return 0;

This is also too large to inline.  It doesn't matter a lot - it's not a
hot path.  I removed this `inline' as well.  gcc will probably inline
it anyway.

If we were really concerned about speed here, we might decide to inline
the test of sysctl_protected_symlinks and to uninline the remainder of
the function.

quoted

      /* Allowed if owner and follower match. */
      cred = current_cred();
+     dentry = link->dentry;
      inode = dentry->d_inode;
      if (cred->fsuid == inode->i_uid)
              return 0;

...

+static inline int may_linkat(struct path *link)
+{
+     int error = 0;
+     const struct cred *cred;
+     struct inode *inode;
+     int mode;
+
+     if (!sysctl_protected_hardlinks)
+             return 0;
+
+     cred = current_cred();
+     inode = link->dentry->d_inode;
+     mode = inode->i_mode;
+
+     if (cred->fsuid != inode->i_uid &&
+         (!S_ISREG(mode) || (mode & S_ISUID) ||
+          ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) ||
+          (inode_permission(inode, MAY_READ | MAY_WRITE))) &&
+         !capable(CAP_FOWNER))
+             error = -EPERM;

omigod.  Geeze man, whose side are you on?

Heh. Yeah, this was not my favorite if statement.

Sounds good -- I'll get it written and tested shortly.

Also please take a look at local variable "mode" and decide if there
was a good reason for it being `int' instead of mode_t?

Ah, sorry. That was an oversight on my part -- I'd ported these before
mode_t existed. I'll fix that too.

Thanks!

-Kees

-- 
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html