[PATCH] UBUNTU: SAUCE: fs: block cross-uid sticky symlinks

[Tim Gardner]

On 05/11/2010 05:51 PM, Kees Cook 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).
>
> The solution is to not permit symlinks to be followed when users do not
> match, but only in a world-writable sticky directory (with an additional
> improvement that the directory owner's symlinks can always be followed,
> regardless who is following them).
>
> 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. Also, please reference
>      where POSIX says this.
> - 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 grsecurity, which is similar to the
> patch in Openwall.  I have added a sysctl to toggle the behavior back
> to the old handling via /proc/sys/fs/weak-sticky-symlinks, as well as
> a ratelimited deprecation warning.
>
> Signed-off-by: Kees Cook<kees.cook at canonical.com>
> ---
>   include/linux/security.h |    1 +
>   kernel/sysctl.c          |    8 ++++++++
>   security/capability.c    |    6 ------
>   security/commoncap.c     |   23 +++++++++++++++++++++++
>   4 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3158dd9..92eca95 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 8686b0f..36a104c 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -86,6 +86,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;
>   extern char core_pattern[];
>   extern unsigned int core_pipe_limit;
>   extern int pid_max;
> @@ -1416,6 +1417,13 @@ 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,
> +	},
>   #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
>   	{
>   		.procname	= "binfmt_misc",
> diff --git a/security/capability.c b/security/capability.c
> index 4875142..d4633f3 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -200,12 +200,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 6166973..83d5a18 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,27 @@ 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)&&
> +	    (parent->i_mode&  S_IWOTH)&&  (parent->i_uid != inode->i_uid)&&
> +	    (cred->fsuid != inode->i_uid)) {
> +		printk_ratelimited(KERN_INFO "deprecated sticky-directory"
> +			" non-matching uid symlink following was attempted"
> +			" by: %s\n", current->comm);
> +		return -EACCES;
> +	}
> +	return 0;
> +}
> +
>   /*
>    * Calculate the new process capability sets from the capability sets attached
>    * to a file.

Are you proposing this for Lucid?

The code looks fine, but I'm not familiar enough with file system 
semantics to comment on cap_inode_follow_link(). However, its an easily 
tested patch.

Acked-by: Tim Gardner <tim.gardner at canonical.com>

-- 
Tim Gardner tim.gardner at canonical.com