Re: [PATCH] fs: block cross-uid sticky symlinks

[Dave Young]

On Fri, May 28, 2010 at 4:16 AM, Kees Cook <kees.cook@xxxxxxxxxxxxx> 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 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 toggle the behavior back to the old handling via
> /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited
> warning.
>
> Signed-off-by: Kees Cook <kees.cook@xxxxxxxxxxxxx>
> ---
> ÂDocumentation/sysctl/kernel.txt | Â 16 ++++++++++++++++
> Âinclude/linux/security.h    Â|  Â1 +
> Âkernel/sysctl.c         |  10 ++++++++++
> Âsecurity/capability.c      |  Â6 ------
> Âsecurity/commoncap.c      Â|  25 +++++++++++++++++++++++++
> Â5 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 3894eaa..6b059f6 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -66,6 +66,7 @@ show up in /proc/sys/kernel:
> Â- threads-max
> Â- unknown_nmi_panic
> Â- version
> +- weak-sticky-symlinks
>
> Â==============================================================
>
> @@ -526,3 +527,18 @@ A small number of systems do generate NMI's for bizarre random reasons such as
> Âpower management so the default is off. That sysctl works like the existing
> Âpanic controls already in that directory.
>
> +==============================================================
> +
> +weak-sticky-symlinks:
> +
> +Following symlinks in world-writable sticky directories (like /tmp) can
> +be dangerous due to time-of-check-time-of-use races that frequently result
> +in security vulnerabilities. ÂBy default, symlinks can only be followed in
> +sticky world-writable directories if the symlink and the follower's uid
> +match (or if the symlink is owned by the owner of the world-writable directory
> +itself).
> +
> +The default value is "0". ÂTo disable this protection, setting a value of "1"
> +will allow symlinks in sticky world-writable directories to be followed by
> +anyone.
> +
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0c88191..a06d568 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -67,6 +67,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
> Âextern int cap_inode_removexattr(struct dentry *dentry, const char *name);
> Âextern int cap_inode_need_killpriv(struct dentry *dentry);
> Âextern int cap_inode_killpriv(struct dentry *dentry);
> +extern int cap_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
> Âextern int cap_file_mmap(struct file *file, unsigned long reqprot,
> Â Â Â Â Â Â Â Â Â Â Â Â unsigned long prot, unsigned long flags,
> Â Â Â Â Â Â Â Â Â Â Â Â unsigned long addr, unsigned long addr_only);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 997080f..bf2d68b 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -88,6 +88,7 @@ extern int sysctl_oom_dump_tasks;
> Âextern int max_threads;
> Âextern int core_uses_pid;
> Âextern int suid_dumpable;
> +extern int weak_sticky_symlinks;

It will be better to put in security.h

> Âextern char core_pattern[];
> Âextern unsigned int core_pipe_limit;
> Âextern int pid_max;
> @@ -1463,6 +1464,15 @@ static struct ctl_table fs_table[] = {
> Â Â Â Â Â Â Â Â.extra1 Â Â Â Â = &zero,
> Â Â Â Â Â Â Â Â.extra2 Â Â Â Â = &two,
> Â Â Â Â},
> + Â Â Â {
> +        .procname    = "weak-sticky-symlinks",
> +        .data      = &weak_sticky_symlinks,
> +        .maxlen     = sizeof(int),
> +        .mode      = 0644,
> +        .proc_handler  = proc_dointvec_minmax,
> + Â Â Â Â Â Â Â .extra1 Â Â Â Â = &zero,
> + Â Â Â Â Â Â Â .extra2 Â Â Â Â = &one,
> + Â Â Â },
> Â#if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
> Â Â Â Â{
>        Â.procname    = "binfmt_misc",
> diff --git a/security/capability.c b/security/capability.c
> index 8168e3e..ff34291 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -169,12 +169,6 @@ static int cap_inode_readlink(struct dentry *dentry)
> Â Â Â Âreturn 0;
> Â}
>
> -static int cap_inode_follow_link(struct dentry *dentry,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct nameidata *nameidata)
> -{
> - Â Â Â return 0;
> -}
> -
> Âstatic int cap_inode_permission(struct inode *inode, int mask)
> Â{
> Â Â Â Âreturn 0;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 4e01599..e7eb397 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -29,6 +29,9 @@
> Â#include <linux/securebits.h>
> Â#include <linux/syslog.h>
>
> +/* sysctl for symlink permissions checking */
> +int weak_sticky_symlinks;
> +
> Â/*
> Â* If a non-root user executes a setuid-root binary in
> Â* !secure(SECURE_NOROOT) mode, then we raise capabilities.
> @@ -281,6 +284,28 @@ int cap_inode_killpriv(struct dentry *dentry)
> Â Â Â Âreturn inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
> Â}
>
> +int cap_inode_follow_link(struct dentry *dentry,
> + Â Â Â Â Â Â Â Â Â Â Â Â struct nameidata *nameidata)
> +{
> + Â Â Â const struct inode *parent = dentry->d_parent->d_inode;
> + Â Â Â const struct inode *inode = dentry->d_inode;
> + Â Â Â const struct cred *cred = current_cred();
> +
> + Â Â Â if (weak_sticky_symlinks)
> + Â Â Â Â Â Â Â return 0;
> +
> + Â Â Â if (S_ISLNK(inode->i_mode) &&
> + Â Â Â Â Â (parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
> + Â Â Â Â Â parent->i_uid != inode->i_uid &&
> + Â Â Â Â Â cred->fsuid != inode->i_uid) {
> + Â Â Â Â Â Â Â printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
> + Â Â Â Â Â Â Â Â Â Â Â "following attempted in sticky-directory by "
> + Â Â Â Â Â Â Â Â Â Â Â "%s (fsuid %d)\n", current->comm, cred->fsuid);
> + Â Â Â Â Â Â Â return -EACCES;
> + Â Â Â }
> + Â Â Â return 0;
> +}
> +
> Â/*
> Â* Calculate the new process capability sets from the capability sets attached
> Â* to a file.
> --
> 1.7.0.4
>
>
> --
> Kees Cook
> Ubuntu Security Team
>



-- 
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/