[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20111202204514.GA5169@outflux.net>
Date: Fri, 2 Dec 2011 12:45:14 -0800
From: Kees Cook <kees@...ntu.com>
To: Kees Cook <keescook@...omium.org>
Cc: linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
kernel-hardening@...ts.openwall.com
Subject: Re: [RFC] fs: symlink restrictions on sticky directories
Hi,
I haven't seen any response to this -- should I move this from "RFC" to
a "PATCH" request, then?
-Kees
On Fri, Nov 18, 2011 at 03:22:08PM -0800, Kees Cook wrote:
> (In case symlink restrictions aren't going to live in Yama, here's a
> version in core VFS based on some feed-back from Al Viro.)
>
> 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 following a given symlink (i.e. a
> root process follows 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 followed when outside a sticky
> world-writable directory, or when the uid of the symlink and follower 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. I have
> added a sysctl to enable the protected behavior, documentation, and a
> ratelimited warning.
>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> Documentation/sysctl/fs.txt | 21 ++++++++++++
> fs/Kconfig | 15 ++++++++
> fs/namei.c | 76 +++++++++++++++++++++++++++++++++++++++---
> kernel/sysctl.c | 10 ++++++
> 4 files changed, 116 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 88fd7f5..939621b 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
> @@ -157,6 +158,26 @@ The default is 65534.
>
> ==============================================================
>
> +protected_sticky_symlinks:
> +
> +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 following a given symlink (i.e. a
> +root process follows 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
> +
> +When set to "0", symlink following behavior is unrestricted.
> +
> +When set to "1" symlinks are permitted to be followed only when outside
> +a sticky world-writable directory, or when the uid of the symlink and
> +follower match, or when the directory owner matches the symlink's owner.
> +
> +This protection is based on the restrictions in Openwall and grsecurity.
> +
> +==============================================================
> +
> 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 5f4c45d..74b9e49 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -278,3 +278,18 @@ source "fs/nls/Kconfig"
> source "fs/dlm/Kconfig"
>
> endmenu
> +
> +config PROTECTED_STICKY_SYMLINKS
> + bool "Protect symlink following 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 following a given symlink (i.e. a root process follows
> + a malicious symlink belonging to another user).
> +
> + Enabling this solves the problem by permitting symlinks to only
> + be followed when outside a sticky world-writable directory,
> + or when the uid of the symlink and follower match, or when
> + the directory and symlink owners match.
> diff --git a/fs/namei.c b/fs/namei.c
> index 5008f01..fc206f4 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -33,6 +33,7 @@
> #include <linux/device_cgroup.h>
> #include <linux/fs_struct.h>
> #include <linux/posix_acl.h>
> +#include <linux/ratelimit.h>
> #include <asm/uaccess.h>
>
> #include "internal.h"
> @@ -624,10 +625,68 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
> path_put(link);
> }
>
> +#ifdef CONFIG_PROTECTED_STICKY_SYMLINKS
> +int protected_sticky_symlinks = 1;
> +#else
> +int protected_sticky_symlinks;
> +#endif
> +
> +/**
> + * may_follow_link - Check symlink following 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 followed when outside a sticky
> + * world-writable directory, or when the uid of the symlink and follower
> + * match, or when the directory owner matches the symlink's owner.
> + *
> + * Returns 0 if following the symlink is allowed, -ve on error.
> + */
> +static inline int
> +may_follow_link(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;
> +
> + /* Allowed if owner and follower 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 "
> + "following attempted in sticky world-writable "
> + "directory by %s (fsuid %d)\n",
> + get_task_comm(name, current), cred->fsuid);
> + }
> + return error;
> +}
> +
> static __always_inline int
> -follow_link(struct path *link, struct nameidata *nd, void **p)
> +follow_link(struct path *link, struct nameidata *nd, void **p, bool sensitive)
> {
> - int error;
> + int error = 0;
> struct dentry *dentry = link->dentry;
>
> BUG_ON(nd->flags & LOOKUP_RCU);
> @@ -646,7 +705,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)
> touch_atime(link->mnt, dentry);
> nd_set_link(nd, NULL);
>
> - error = security_inode_follow_link(link->dentry, nd);
> + if (sensitive)
> + error = may_follow_link(link->dentry, nd);
> + if (!error)
> + error = security_inode_follow_link(link->dentry, nd);
> if (error) {
> *p = ERR_PTR(error); /* no ->put_link(), please */
> path_put(&nd->path);
> @@ -1339,7 +1401,7 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd)
> struct path link = *path;
> void *cookie;
>
> - res = follow_link(&link, nd, &cookie);
> + res = follow_link(&link, nd, &cookie, 0);
> if (!res)
> res = walk_component(nd, path, &nd->last,
> nd->last_type, LOOKUP_FOLLOW);
> @@ -1612,7 +1674,8 @@ static int path_lookupat(int dfd, const char *name,
> void *cookie;
> struct path link = path;
> nd->flags |= LOOKUP_PARENT;
> - err = follow_link(&link, nd, &cookie);
> +
> + err = follow_link(&link, nd, &cookie, 1);
> if (!err)
> err = lookup_last(nd, &path);
> put_link(nd, &link, cookie);
> @@ -2324,7 +2387,8 @@ static struct file *path_openat(int dfd, const char *pathname,
> }
> nd->flags |= LOOKUP_PARENT;
> nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
> - error = follow_link(&link, nd, &cookie);
> +
> + error = follow_link(&link, nd, &cookie, 1);
> if (unlikely(error))
> filp = ERR_PTR(error);
> else
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ae27196..cc2c5f9 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -92,6 +92,7 @@ extern int sysctl_overcommit_memory;
> extern int sysctl_overcommit_ratio;
> 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;
> @@ -1495,6 +1496,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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kees Cook
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
