Message-ID: <alpine.LRH.2.02.1208031425530.25196@tundra.namei.org>
Date:	Fri, 3 Aug 2012 14:26:05 +1000 (EST)
From:	James Morris <jmorris@...ei.org>
To:	kernel-hardening@...ts.openwall.com
cc:	Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Eric Paris <eparis@...hat.com>,
	Matthew Wilcox <matthew@....cx>,
	Doug Ledford <dledford@...hat.com>,
	Joe Korty <joe.korty@...r.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Ingo Molnar <mingo@...e.hu>,
	David Howells <dhowells@...hat.com>,
	James Morris <james.l.morris@...cle.com>,
	linux-doc@...r.kernel.org,
	Dan Rosenberg <drosenberg@...curity.com>,
	Kees Cook <keescook@...omium.org>
Subject: Re: [kernel-hardening] [PATCH 1/2] fs: add link restrictions

On Wed, 25 Jul 2012, Kees Cook wrote:

> This adds symlink and hardlink restrictions to the Linux VFS.

Is Al happy with this now?


> 
> 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
> 
> 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
>  2010 May, Kees Cook
>   https://lkml.org/lkml/2010/5/30/144
> 
> 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. Additionally, no applications have yet been found
>      that rely on this behavior.
>  - 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 should live in the core VFS.
>    - This should live in an LSM. (https://lkml.org/lkml/2010/5/31/135)
>  - This should live in an LSM.
>    - This should live in the core VFS. (https://lkml.org/lkml/2010/8/2/188)
> 
> Hardlinks:
> 
> 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.
> 
> [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
> 
> This patch is based on the patches in Openwall and grsecurity, along with
> suggestions from Al Viro. I have added a sysctl to enable the protected
> behavior, and documentation.
> 
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Acked-by: Ingo Molnar <mingo@...e.hu>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> 
> ---
> v2012.5:
>  - updates requested by Al Viro:
>    - remove CONFIGs
>    - pass nd for parent checking
>    - release path on error
> v2012.4:
>  - split audit functions into a separate patch, suggested by Eric Paris.
> v2012.3:
>  While this code has been living in -mm and linux-next for 2 releases,
>  this is a small rework based on feedback from Al Viro:
>  - Moved audit functions into audit.c.
>  - Added tests directly to path_openat/path_lookupat.
>  - Merged with hardlink restriction patch to make things more sensible.
> v2012.2:
>  - Change sysctl mode to 0600, suggested by Ingo Molnar.
>  - Rework CONFIG logic to split code from default behavior.
>  - Renamed sysctl to have a "sysctl_" prefix, suggested by Andrew Morton.
>  - Use "true/false" instead of "1/0" for bool arg, thanks to Andrew Morton.
>  - Do not trust s_id to be safe to print, suggested by Andrew Morton.
> v2012.1:
>  - Use GFP_KERNEL for audit log allocation, thanks to Ingo Molnar.
> v2011.3:
>  - Add pid/comm back to logging.
> v2011.2:
>  - Updated documentation, thanks to Randy Dunlap.
>  - Switched Kconfig default to "y", added __read_mostly to sysctl,
>    thanks to Ingo Molnar.
>  - Switched to audit logging to gain safe path and name reporting when
>    hitting the restriction.
> v2011.1:
>  - back from hiatus
> ---
>  Documentation/sysctl/fs.txt |   42 +++++++++++++++
>  fs/namei.c                  |  121 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h          |    2 +
>  kernel/sysctl.c             |   18 ++++++
>  4 files changed, 183 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 13d6166..d4a372e 100644
> --- a/Documentation/sysctl/fs.txt
> +++ b/Documentation/sysctl/fs.txt
> @@ -32,6 +32,8 @@ Currently, these files are in /proc/sys/fs:
>  - nr_open
>  - overflowuid
>  - overflowgid
> +- protected_hardlinks
> +- protected_symlinks
>  - suid_dumpable
>  - super-max
>  - super-nr
> @@ -157,6 +159,46 @@ The default is 65534.
>  
>  ==============================================================
>  
> +protected_hardlinks:
> +
> +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,
> +on systems without separated partitions, this stops unauthorized users
> +from "pinning" vulnerable setuid/setgid files against being upgraded by
> +the administrator, or linking to special files.
> +
> +When set to "0", hardlink creation behavior is unrestricted.
> +
> +When set to "1" hardlinks cannot be created by users if they do not
> +already own the source file, or do not have read/write access to it.
> +
> +This protection is based on the restrictions in Openwall and grsecurity.
> +
> +==============================================================
> +
> +protected_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/namei.c b/fs/namei.c
> index 2ccc35c..e5ad2db 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -650,6 +650,118 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
>  	path_put(link);
>  }
>  
> +int sysctl_protected_symlinks __read_mostly = 1;
> +int sysctl_protected_hardlinks __read_mostly = 1;
> +
> +/**
> + * may_follow_link - Check symlink following for unsafe situations
> + * @link: The path of the symlink
> + *
> + * 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
> + * from under them by way of other users creating malicious symlinks.
> + * It will permit symlinks 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.
> + *
> + * Returns 0 if following the symlink is allowed, -ve on error.
> + */
> +static inline int may_follow_link(struct path *link, struct nameidata *nd)
> +{
> +	const struct inode *inode;
> +	const struct inode *parent;
> +
> +	if (!sysctl_protected_symlinks)
> +		return 0;
> +
> +	/* Allowed if owner and follower match. */
> +	inode = link->dentry->d_inode;
> +	if (current_cred()->fsuid == inode->i_uid)
> +		return 0;
> +
> +	/* Allowed if parent directory not sticky and world-writable. */
> +	parent = nd->path.dentry->d_inode;
> +	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) != (S_ISVTX|S_IWOTH))
> +		return 0;
> +
> +	/* Allowed if parent directory and link owner match. */
> +	if (parent->i_uid == inode->i_uid)
> +		return 0;
> +
> +	path_put(&nd->path);
> +	return -EACCES;
> +}
> +
> +/**
> + * safe_hardlink_source - Check for safe hardlink conditions
> + * @inode: the source inode to hardlink from
> + *
> + * Return false if at least one of the following conditions:
> + *    - inode is not a regular file
> + *    - inode is setuid
> + *    - inode is setgid and group-exec
> + *    - access failure for read and write
> + *
> + * Otherwise returns true.
> + */
> +static bool safe_hardlink_source(struct inode *inode)
> +{
> +	umode_t mode = inode->i_mode;
> +
> +	/* Special files should not get pinned to the filesystem. */
> +	if (!S_ISREG(mode))
> +		return false;
> +
> +	/* Setuid files should not get pinned to the filesystem. */
> +	if (mode & S_ISUID)
> +		return false;
> +
> +	/* Executable setgid files should not get pinned to the filesystem. */
> +	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
> +		return false;
> +
> +	/* Hardlinking to unreadable or unwritable sources is dangerous. */
> +	if (inode_permission(inode, MAY_READ | MAY_WRITE))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * may_linkat - Check permissions for creating a hardlink
> + * @link: the source to hardlink from
> + *
> + * Block hardlink when all of:
> + *  - sysctl_protected_hardlinks enabled
> + *  - fsuid does not match inode
> + *  - hardlink source is unsafe (see safe_hardlink_source() above)
> + *  - not CAP_FOWNER
> + *
> + * Returns 0 if successful, -ve on error.
> + */
> +static int may_linkat(struct path *link)
> +{
> +	const struct cred *cred;
> +	struct inode *inode;
> +
> +	if (!sysctl_protected_hardlinks)
> +		return 0;
> +
> +	cred = current_cred();
> +	inode = link->dentry->d_inode;
> +
> +	/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
> +	 * otherwise, it must be a safe source.
> +	 */
> +	if (cred->fsuid == inode->i_uid || safe_hardlink_source(inode) ||
> +	    capable(CAP_FOWNER))
> +		return 0;
> +
> +	return -EPERM;
> +}
> +
>  static __always_inline int
>  follow_link(struct path *link, struct nameidata *nd, void **p)
>  {
> @@ -1818,6 +1930,9 @@ static int path_lookupat(int dfd, const char *name,
>  		while (err > 0) {
>  			void *cookie;
>  			struct path link = path;
> +			err = may_follow_link(&link, nd);
> +			if (unlikely(err))
> +				break;
>  			nd->flags |= LOOKUP_PARENT;
>  			err = follow_link(&link, nd, &cookie);
>  			if (err)
> @@ -2777,6 +2892,9 @@ static struct file *path_openat(int dfd, const char *pathname,
>  			error = -ELOOP;
>  			break;
>  		}
> +		error = may_follow_link(&link, nd);
> +		if (unlikely(error))
> +			break;
>  		nd->flags |= LOOKUP_PARENT;
>  		nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
>  		error = follow_link(&link, nd, &cookie);
> @@ -3436,6 +3554,9 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
>  	error = -EXDEV;
>  	if (old_path.mnt != new_path.mnt)
>  		goto out_dput;
> +	error = may_linkat(&old_path);
> +	if (unlikely(error))
> +		goto out_dput;
>  	error = mnt_want_write(new_path.mnt);
>  	if (error)
>  		goto out_dput;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 8fabb03..c8fb6df 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -437,6 +437,8 @@ extern unsigned long get_max_files(void);
>  extern int sysctl_nr_open;
>  extern struct inodes_stat_t inodes_stat;
>  extern int leases_enable, lease_break_time;
> +extern int sysctl_protected_symlinks;
> +extern int sysctl_protected_hardlinks;
>  
>  struct buffer_head;
>  typedef int (get_block_t)(struct inode *inode, sector_t iblock,
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4ab1187..5d9a1d2 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1494,6 +1494,24 @@ static struct ctl_table fs_table[] = {
>  #endif
>  #endif
>  	{
> +		.procname	= "protected_symlinks",
> +		.data		= &sysctl_protected_symlinks,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0600,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &one,
> +	},
> +	{
> +		.procname	= "protected_hardlinks",
> +		.data		= &sysctl_protected_hardlinks,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0600,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &one,
> +	},
> +	{
>  		.procname	= "suid_dumpable",
>  		.data		= &suid_dumpable,
>  		.maxlen		= sizeof(int),
> -- 
> 1.7.0.4
> 

-- 
James Morris
<jmorris@...ei.org>
--
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/

Powered by blists - more mailing lists