Re: [PATCH v4] fs: allow protected cross-uid sticky symlinks

[Dave Young <hidave.darkstar@gmail.com>]

On Thu, Jun 3, 2010 at 6:23 AM, Kees Cook <kees.cook@canonical.com> wrote:
> A long-standing class of security issues is the symlink-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 opening a file through a given
> symlink (i.e. a root process opens a symlink belonging to another user).
> For a likely incomplete list of hundreds of examples across the years,
> please see: http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp
>
> The solution is to permit symlinks to only be opened when outside a sticky
> world-writable directory, or when the uid of the symlink and opener match,
> or when the directory owner matches the symlink's owner.
>
> Some pointers to the history of earlier discussion that I could find:
>
>  1996 Aug, Zygo Blaxell
>  http://marc.info/?l=bugtraq&m=87602167419830&w=2
>  1996 Oct, Andrew Tridgell
>  http://lkml.indiana.edu/hypermail/linux/kernel/9610.2/0086.html
>  1997 Dec, Albert D Cahalan
>  http://lkml.org/lkml/1997/12/16/4
>  2005 Feb, Lorenzo Hernández García-Hierro
>  http://lkml.indiana.edu/hypermail/linux/kernel/0502.0/1896.html
>
> Past objections and rebuttals could be summarized as:
>
>  - Violates POSIX.
>   - POSIX didn't consider this situation and it's not useful to follow
>     a broken specification at the cost of security.
>  - Might break unknown applications that use this feature.
>   - Applications that break because of the change are easy to spot and
>     fix. Applications that are vulnerable to symlink ToCToU by not having
>     the change aren't.
>  - Applications should just use mkstemp() or O_CREATE|O_EXCL.
>   - True, but applications are not perfect, and new software is written
>     all the time that makes these mistakes; blocking this flaw at the
>     kernel is a single solution to the entire class of vulnerability.
>
> This patch is based on the patch in Openwall and grsecurity, but with the
> scope changed to be only "opening" a symlink.  I have added a sysctl to
> enable the protected behavior, documentation, and a ratelimited warning.
>
> v2:
>  - dropped redundant S_ISLNK check.
>  - moved sysctl extern into security.h.

Not in v4?

>  - asked to include CC to linux-fsdevel.
>
> v3:
>  - move into VFS core.
>  - add CONFIG entry for build-time default.
>  - rename sysctl, invert logic.
>  - use get_task_comm for task name.
>  - lock dentry when checking parent.
>
> v4:
>  - limit check to leaf symlink opening.
>
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> ---
>  Documentation/sysctl/fs.txt |   15 ++++++++++
>  fs/Kconfig                  |   15 ++++++++++
>  fs/namei.c                  |   61 +++++++++++++++++++++++++++++++++++++++++++
>  kernel/sysctl.c             |   10 +++++++
>  4 files changed, 101 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 6268250..9986bce 100644
> --- a/Documentation/sysctl/fs.txt
> +++ b/Documentation/sysctl/fs.txt
> @@ -32,6 +32,7 @@ Currently, these files are in /proc/sys/fs:
>  - nr_open
>  - overflowuid
>  - overflowgid
> +- protected-sticky-symlinks
>  - suid_dumpable
>  - super-max
>  - super-nr
> @@ -158,6 +159,20 @@ The default is 65534.
>
>  ==============================================================
>
> +protected-sticky-symlinks:
> +
> +Opening symlinks in sticky world-writable directories (like /tmp) can be
> +dangerous due to time-of-check-time-of-use races that frequently result
> +in security vulnerabilities.
> +
> +The default value is "0", leaving the behavior of symlink opening
> +unchanged from POSIX.  A value of "1" will enable the protection, causing
> +symlinks to be openable only if outside a sticky world-writable directory,
> +or if the symlink and the opener's uid match, or if the symlink and its
> +directory are owned by the same uid.
> +
> +==============================================================
> +
>  suid_dumpable:
>
>  This value can be used to query and set the core dump mode for setuid
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 5f85b59..48df7cd 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -256,3 +256,18 @@ source "fs/nls/Kconfig"
>  source "fs/dlm/Kconfig"
>
>  endmenu
> +
> +config PROTECTED_STICKY_SYMLINKS
> +       bool "Protect symlink opening in sticky world-writable directories"
> +       help
> +         A long-standing class of security issues is the symlink-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 opening a given symlink (i.e. a root process opens a
> +          malicious symlink belonging to another user).
> +
> +         Enabling this solves the problem by permitting symlinks to only
> +         be opened when outside a sticky world-writable directory, or
> +          when the uid of the symlink and opener match, or when the
> +          directory and symlink owners match.
> diff --git a/fs/namei.c b/fs/namei.c
> index 868d0cb..ee9d493 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -32,6 +32,7 @@
>  #include <linux/fcntl.h>
>  #include <linux/device_cgroup.h>
>  #include <linux/fs_struct.h>
> +#include <linux/ratelimit.h>
>  #include <asm/uaccess.h>
>
>  #include "internal.h"
> @@ -530,6 +531,60 @@ static inline void path_to_nameidata(struct path *path, struct nameidata *nd)
>        nd->path.dentry = path->dentry;
>  }
>
> +int protected_sticky_symlinks = CONFIG_PROTECTED_STICKY_SYMLINKS;
> +
> +/**
> + * may_open_sticky_symlink - Check symlink opening for unsafe situations
> + * @dentry: The inode/dentry of the symlink
> + * @nameidata: The path data of the symlink
> + *
> + * In the case of the protected_sticky_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
> + * from under them by way of other users creating malicious symlinks.
> + * It will permit symlinks to only be opened when outside a sticky
> + * world-writable directory, or when the uid of the symlink and opener
> + * match, or when the directory owner matches the symlink's owner.
> + *
> + * Returns 0 if opening the symlink is allowed, -ve on error.
> + */
> +static __always_inline int
> +may_open_sticky_symlink(struct dentry *dentry, struct nameidata *nameidata)
> +{
> +       int error = 0;
> +       const struct inode *parent;
> +       const struct inode *inode;
> +       const struct cred *cred;
> +
> +       if (!protected_sticky_symlinks)
> +               return 0;
> +
> +       /* owner and opener match? */
> +       cred = current_cred();
> +       inode = dentry->d_inode;
> +       if (cred->fsuid == inode->i_uid)
> +               return 0;
> +
> +       /* check parent directory mode and owner */
> +       spin_lock(&dentry->d_lock);
> +       parent = dentry->d_parent->d_inode;
> +       if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
> +           parent->i_uid != inode->i_uid) {
> +               error = -EACCES;
> +       }
> +       spin_unlock(&dentry->d_lock);
> +
> +       if (error) {
> +               char name[sizeof(current->comm)];
> +               printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
> +                       "opening attempted in sticky world-writable "
> +                       "directory by %s (fsuid %d)\n",
> +                       get_task_comm(name, current), cred->fsuid);
> +       }
> +       return error;
> +}
> +
>  static __always_inline int
>  __do_follow_link(struct path *path, struct nameidata *nd, void **p)
>  {
> @@ -1844,6 +1899,12 @@ reval:
>                        goto exit_dput;
>                if (count++ == 32)
>                        goto exit_dput;
> +
> +               /* check if this symlink is in a sticky world-write dir */
> +               error = may_open_sticky_symlink(path.dentry, &nd);
> +               if (error)
> +                       goto exit_dput;
> +
>                /*
>                 * This is subtle. Instead of calling do_follow_link() we do
>                 * the thing by hands. The reason is that this way we have zero
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 997080f..56affd6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -87,6 +87,7 @@ extern int sysctl_oom_kill_allocating_task;
>  extern int sysctl_oom_dump_tasks;
>  extern int max_threads;
>  extern int core_uses_pid;
> +extern int protected_sticky_symlinks;
>  extern int suid_dumpable;
>  extern char core_pattern[];
>  extern unsigned int core_pipe_limit;
> @@ -1455,6 +1456,15 @@ static struct ctl_table fs_table[] = {
>  #endif
>  #endif
>        {
> +               .procname       = "protected-sticky-symlinks",
> +               .data           = &protected_sticky_symlinks,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = &zero,
> +               .extra2         = &one,
> +       },
> +       {
>                .procname       = "suid_dumpable",
>                .data           = &suid_dumpable,
>                .maxlen         = sizeof(int),
> --
> 1.7.0.4
>
>
> --
> Kees Cook
> Ubuntu Security Team
>



-- 
Regards
dave