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

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

[Kees Cook]

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 logic via
/proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited
warning.

v2:
 - dropped redundant S_ISLNK check.
 - moved sysctl extern into security.h.
 - asked to include CC to linux-fsdevel.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 Documentation/sysctl/kernel.txt |   16 ++++++++++++++++
 include/linux/security.h        |    3 +++
 kernel/sysctl.c                 |    9 +++++++++
 security/capability.c           |    6 ------
 security/commoncap.c            |   24 ++++++++++++++++++++++++
 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..ee31647 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -43,6 +43,8 @@
 #define SECURITY_CAP_NOAUDIT 0
 #define SECURITY_CAP_AUDIT 1
 
+extern int weak_sticky_symlinks;
+
 struct ctl_table;
 struct audit_krule;
 
@@ -67,6 +69,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..cd7fee4 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1463,6 +1463,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..0ff07a6 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 ((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

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

[Eric W. Biederman]

Kees Cook <kees.cook@canonical.com> writes:

> 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 logic via
> /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited
> warning.

The name of the sysctl is horrible it is a double negative, which
makes thinking about it hard.

Why not simply put each user in a different mount namespace and have separate
/tmp directories per user?  That works today, with no kernel changes.

Placing this in cap_inode_follow_link is horrible naming.  There is nothing
capabilities about this.  Either this needs to go into one or several
of the security modules or this needs to go into the core vfs. 

I can't argue with taking action to close the too frequently security
issues in /tmp, but this changes appears to be unnecessary, difficult
to maintain, and difficult to understand.

Eric

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

On Sun, 2010-05-30 at 20:04 -0700, 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).  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 logic via
> /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited
> warning.
> 
> v2:
>  - dropped redundant S_ISLNK check.
>  - moved sysctl extern into security.h.
>  - asked to include CC to linux-fsdevel.

We need to call this function in the SELinux case.  So you'll need a
patch like the one attached (not even compiled but I think it is right)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c9f25b..d6ebee2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2668,8 +2668,13 @@ static int selinux_inode_readlink(struct dentry *dentry)
 
 static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
 {
+       int rc;
        const struct cred *cred = current_cred();
 
+       rc = cap_inode_follow_link(dentry, nameidata);
+       if (rc)
+               return rc;
+
        return dentry_has_perm(cred, NULL, dentry, FILE__READ);
 }
 

> +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 ((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;
> +}

What stops us from racing between the assignment of parent and it's
first use with a rename on our object and rmdir on the old parent?  I'm
wondering if we need to be doing this test holding dentry->d_lock (which
is what protects dentry->d_parent if I recall correctly)

Certainly doesn't fix all of the raciness, but I think it would close
the opps part.  Maybe someone who knows the VFS better can tell me if I
am misguided.

-Eric

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

[Eric Paris]

On Sun, 2010-05-30 at 20:04 -0700, 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).  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 logic via
> /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited
> warning.
> 
> v2:
>  - dropped redundant S_ISLNK check.
>  - moved sysctl extern into security.h.
>  - asked to include CC to linux-fsdevel.

We need to call this function in the SELinux case.  So you'll need a
patch like the one attached (not even compiled but I think it is right)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c9f25b..d6ebee2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2668,8 +2668,13 @@ static int selinux_inode_readlink(struct dentry *dentry)
 
 static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
 {
+       int rc;
        const struct cred *cred = current_cred();
 
+       rc = cap_inode_follow_link(dentry, nameidata);
+       if (rc)
+               return rc;
+
        return dentry_has_perm(cred, NULL, dentry, FILE__READ);
 }
 

> +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 ((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;
> +}

What stops us from racing between the assignment of parent and it's
first use with a rename on our object and rmdir on the old parent?  I'm
wondering if we need to be doing this test holding dentry->d_lock (which
is what protects dentry->d_parent if I recall correctly)

Certainly doesn't fix all of the raciness, but I think it would close
the opps part.  Maybe someone who knows the VFS better can tell me if I
am misguided.

-Eric



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

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

[Kees Cook]

Hi Eric,

On Sun, May 30, 2010 at 08:50:53PM -0700, Eric W. Biederman wrote:
> The name of the sysctl is horrible it is a double negative, which
> makes thinking about it hard.

Hmm, I see your point, the "safe" value is "weak: not".  I was trying to
be descriptive without needing a "true" value, but I guess that's silly;
we already have things like randomize_va_space set to "2" by default, etc.
What would you suggest instead?  "protected_sticky_symlinks" (and reverse
the default and test logic)?

> Why not simply put each user in a different mount namespace and have separate
> /tmp directories per user?  That works today, with no kernel changes.

The key here is "no kernel changes" -- trying to isolate every user
and service from each other using different mount namespaces will not
work quickly in current distributions.  Even doing bind-mount tricks
to keep /tmp away from different users is overkill, especially when
you have situations like "screen" using a common /tmp directory tree
(in the setuid version), etc.  Things (correctly) expect to share /tmp
in some cases.  However, this one kernel change will allow everything
to continue without userspace overhead and without breaking anything
terribly.  Using containers will probably be the future, but I want to
solve this in the general case today.

> Placing this in cap_inode_follow_link is horrible naming.  There is nothing
> capabilities about this.  Either this needs to go into one or several
> of the security modules or this needs to go into the core vfs.

My thinking was that most of the LSMs call down to commoncaps first,
so it's a single place to put this.  When I was looking at this code
originally, I thought that if it doesn't go in security_inode_follow_link,
then a new function would be added to the VFS and both callers
of security_inode_follow_link would need to call it just before
security_inode_follow_link.  It seemed like putting it in there reduced
duplication of logic.

However, on closer examination, it seems that this code could live in
__do_follow_link instead.

fs/namei.c:
...
                error = security_inode_follow_link(path.dentry, &nd);
                if (error)
                        goto exit_dput;
                error = __do_follow_link(&path, &nd, &cookie);
...
        err = security_inode_follow_link(path->dentry, nd);
        if (err)
                goto loop;
        current->link_count++;
        current->total_link_count++;
        nd->depth++;
        err = __do_follow_link(path, nd, &cookie);
...

What would you suggest for the best approach here?

> I can't argue with taking action to close the too frequently security
> issues in /tmp, but this changes appears to be unnecessary, difficult
> to maintain, and difficult to understand.

Well, we disagree about "unnecessary".  :)  Finding an easy to maintain
solution is my goal here, and if it's difficult to understand, then I need
to fix that too.  What could use better clarification?

Thanks!

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Kees Cook]

Hi Eric,

On Sun, May 30, 2010 at 11:54:23PM -0400, Eric Paris wrote:
> We need to call this function in the SELinux case.  So you'll need a
> patch like the one attached (not even compiled but I think it is right)
> [..]
>  static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
> [..]
> +       rc = cap_inode_follow_link(dentry, nameidata);

Yeah, when I quickly checked SELinux and AppArmor, it seemed that they
were always calling down to all commoncaps functions, but it looks like
not in all cases.  I think that Eric Biederman's observations here makes
the most sense: this check needs to happen without involving the LSMs
at all.

> > +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 ((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;
> > +}
> 
> What stops us from racing between the assignment of parent and it's
> first use with a rename on our object and rmdir on the old parent?  I'm
> wondering if we need to be doing this test holding dentry->d_lock (which
> is what protects dentry->d_parent if I recall correctly)
> 
> Certainly doesn't fix all of the raciness, but I think it would close
> the opps part.  Maybe someone who knows the VFS better can tell me if I
> am misguided.

The only other use of d_parent I can see there is in may_delete().  With
vfs_unlink() calling that, it would seem to be racey too if we needed to
hold a lock for that.  But it's not clear to me in vfs_follow_link is doing
locking somehow.

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Alan Cox]

> Past objections and rebuttals could be summarized as:

You missed a couple at least

1.We have things like SELinux so you can write rules to bound apps
  which should be able to do this, if not then we should fix the security
  policy to generally solve it

2. If it worries you that much then you can *fix* /tmp/ for good using
the work Al Viro did years ago on mountpoints. Give people their
own /tmp. End of problem.

>  - Violates POSIX.
>    - POSIX didn't consider this situation and it's not useful to follow
>      a broken specification at the cost of security.

(If you fill your computer with concrete it will also violate POSIX but
it will be more secure than your proposal. Both will of course has some
impact on applications).

>  - 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

What if they are things like binary only games - now you can neither turn
on your feature nor fix the app if you want to use it. If it was using
SELinux or similar rules you could create a single exception rule. If it
was using per user /tmp nobody would have to do anything

>  - 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.

So new software is also being written all the time which has other holes.
Actually its also according to your argument being written all the time
so it'll break if you turn the feature on ?


> +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.

Wrong way around. You don't enable non-compliance and misbehaviour by
default. For it to be any use you need a userspace adapted to run on your
odd environment, so your distro can flip the flag from Linux mode to odd.


> +	if ((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);

This is of minimal use - current->comm is arbitary and user controlled.
Also are we guaranteed to hold enough locks here to stop the parent uid
changing ?

You've also stopped root doing this. Given DAC override that makes no
sense and is asking to confuse stuff.

Give your users their own /tmp. No kernel mods, no misbehaviours, no
weirdomatic path walking hackery. No kernel patch needed that I can see.


Alan

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

[Christoph Hellwig]

NAK for changing this in core code.  These are long-living and
documented semantics that we can't simply break, and as Alan mentioned
having there's good enough workaround that don't break applications.

Feel free to shovel it into the crackpot LSM of your choice.

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

[tytso]

On Sun, May 30, 2010 at 08:04:02PM -0700, Kees Cook wrote:
>  - Violates POSIX.
>    - POSIX didn't consider this situation and it's not useful to follow
>      a broken specification at the cost of security.

POSIX allows implementations to fail requested operations with a
permission denied for reasons beyond that which is specified by the
POSIX implementation.  Indeed, POSIX doesn't specify the sticky bit at
all, so implementations that implemented the sticky bit were able to
pass POSIX precisely because it's OK to provide stricter semantics
than that which is specified by POSIX.  (The sticky bit is specified
in the XSI exstension to the Single Unix Specification, but the same
principle applies; it's OK for us have additional situations where we
return EACCESS beyond that which was contemplated by POSIX/SUS; this
alone wouldn't cause us to 'violate POSIX'.)

So for people who to argue against this (which I believe to be a
useful restriction, and not one that necessarily has to be done in a
LSM), it's not sufficient to say that it is a POSIX violation, because
it isn't.  The sticky bit itself wasn't originally considered by
POSIX, and many systems which implemented the sticky bit had no
problems becoming ceritifed as POSIX compliant.

						- Ted

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

[Kees Cook]

Hi Alan,

On Mon, May 31, 2010 at 11:23:14AM +0100, Alan Cox wrote:
> 1.We have things like SELinux so you can write rules to bound apps
>   which should be able to do this, if not then we should fix the security
>   policy to generally solve it

This requires that policy be written for all applications ever, which isn't
feasible.

> 2. If it worries you that much then you can *fix* /tmp/ for good using
> the work Al Viro did years ago on mountpoints. Give people their
> own /tmp. End of problem.

This has the same flaws in interoperability that you mention below for
hypothetical binary games.

> (If you fill your computer with concrete it will also violate POSIX but
> it will be more secure than your proposal. Both will of course has some
> impact on applications).

Well, my point was that in the past, a stand-alone objection was that it
violates POSIX.  I don't feel that this is a sufficient objection -- POSIX
did not consider this corner-case.  Filling my computer with concrete has a
nasty side-effect of making all my applications fail.  This patch isn't
that intrusive.  :)

> What if they are things like binary only games - now you can neither turn
> on your feature nor fix the app if you want to use it. If it was using
> SELinux or similar rules you could create a single exception rule. If it
> was using per user /tmp nobody would have to do anything

I do not believe there are any applications that depend on following
symlinks in sticky world-writable directories.  Besides, just because
something lacks source doesn't mean I can't change its behavior via binary
editing or LD_PRELOAD.  I do not feel that non-existent hypothetical
applications needing this feature is a sufficient reason to reject the
patch.

> >  - 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.
> 
> So new software is also being written all the time which has other holes.
> Actually its also according to your argument being written all the time
> so it'll break if you turn the feature on ?

Yes, everything is buggy -- I am seeking to solve a specific class
of security issue so that those bugs don't turn into an more serious problem.
One that has been fully solved with no known bad side-effects in a
hand-full of distros for possibly more than 10 years now.

> > +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.
> 
> Wrong way around. You don't enable non-compliance and misbehaviour by
> default. For it to be any use you need a userspace adapted to run on your
> odd environment, so your distro can flip the flag from Linux mode to odd.

I'm happy to make that adjustment.

> > +	if ((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);
> 
> This is of minimal use - current->comm is arbitary and user controlled.

It's used strictly for reporting.  I'm open to other suggestions -- I just
want to give a system admin some information in the case where an
application is being attacked with /tmp symlink races.

> Also are we guaranteed to hold enough locks here to stop the parent uid
> changing ?

I don't know the answer to that -- I'm happy to add any needed locking.

> You've also stopped root doing this. Given DAC override that makes no
> sense and is asking to confuse stuff.

This should specifically ignore DAC override.  In most cases, it is root we
are trying to protect from the symlink race.

> Give your users their own /tmp. No kernel mods, no misbehaviours, no
> weirdomatic path walking hackery. No kernel patch needed that I can see.

Some real applications expect to share /tmp (e.g. "screen").  Splitting up
/tmp for every user isn't quickly feasible and requires every distro
change.  I would note that separate /tmp also breaks the hypothetical
binary game if it really is required to follow cross-uid symlinks.

I know that for the VFS folks, this is not a popular patch.  I have no
illusions there, but I want to find a kernel solution that is acceptable.
Fixing this in the kernel is trivial.  Fixing this in userspace is not
trivial -- it requires that every distro change the very semantics of
how /tmp works.  I can't say that's a bad idea, but I can say that
it will not be globally successful.  This kernel changes breaks no
applications, and fixes a whole class of problem forever, is small,
and has well-defined rules.

I would prefer it be solved at the VFS layer.  Doing it in commoncaps
is possible (and is even what I proposed), but I want to avoid having
everyone punt this patch around to different subsystem maintainers.

It's a real problem. I have proposed a real solution. If it's not okay,
I'd like to see an alternative solution that you're comfortable with
that fixes it to the same global scope.  (i.e. not userspace or tied to
a specific LSM.)

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Kees Cook]

Hi Christoph,

On Mon, May 31, 2010 at 06:35:10AM -0400, Christoph Hellwig wrote:
> NAK for changing this in core code.  These are long-living and
> documented semantics that we can't simply break, and as Alan mentioned
> having there's good enough workaround that don't break applications.

I totally disagree -- there are no applications that depend on this
behavior in sticky world-writable directories.  Adding this logic (where
ever it lands) fixes /tmp symlink races forever and breaks nothing.

> Feel free to shovel it into the crackpot LSM of your choice.

I have no strong opinion about where it should live.  The strong opinion
I have is that all Linux users, regardless of LSM choice, should benefit
from the fix.

I'd like to separate objections about implementation from objections about
the change in semantics itself.  If every user of Linux gains this symlink
protection, does it matter if it's in core VFS or in commoncaps?

Expecting the push-back from VFS, I wrote this patch against commoncaps.

James, would you take it there (along with the patch to SELinux to call
out to commoncaps) if there is no way to proceed with the VFS core towards
solving this?

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Alan Cox]

On Mon, 31 May 2010 10:50:08 -0700
Kees Cook <kees.cook@canonical.com> wrote:

> Hi Alan,
> 
> On Mon, May 31, 2010 at 11:23:14AM +0100, Alan Cox wrote:
> > 1.We have things like SELinux so you can write rules to bound apps
> >   which should be able to do this, if not then we should fix the security
> >   policy to generally solve it
> 
> This requires that policy be written for all applications ever, which isn't
> feasible.

Wrong, you can write a generic policy then write exceptions.

> > 2. If it worries you that much then you can *fix* /tmp/ for good using
> > the work Al Viro did years ago on mountpoints. Give people their
> > own /tmp. End of problem.
> 
> This has the same flaws in interoperability that you mention below for
> hypothetical binary games.

Not that I can see - please explain.

> nasty side-effect of making all my applications fail.  This patch isn't
> that intrusive.  :)

I would disagree there, and it seems Christoph and others likjewise.

> I do not believe there are any applications that depend on following
> symlinks in sticky world-writable directories.  Besides, just because
> something lacks source doesn't mean I can't change its behavior via binary
> editing or LD_PRELOAD.  I do not feel that non-existent hypothetical
> applications needing this feature is a sufficient reason to reject the
> patch.

Maybe you are right - but you can do this without a kernel patch or as a
security module and leave the rest of the kernel alone.

> Yes, everything is buggy -- I am seeking to solve a specific class
> of security issue so that those bugs don't turn into an more serious problem.
> One that has been fully solved with no known bad side-effects in a
> hand-full of distros for possibly more than 10 years now.

Plan 9 solved this years ago. Linux added the necessary per user /tmp (or
per session /tmp if you prefer) support years ago.

> > > +		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
> > > +			"following attempted in sticky-directory by "
> > > +			"%s (fsuid %d)\n", current->comm, cred->fsuid);
> > 
> > This is of minimal use - current->comm is arbitary and user controlled.
> 
> It's used strictly for reporting.  I'm open to other suggestions -- I just
> want to give a system admin some information in the case where an
> application is being attacked with /tmp symlink races.

current->comm can be a random mess of control codes, it could include a
newline to fool the printk paths. You don't printk user provided
untrusted bits...

> This should specifically ignore DAC override.  In most cases, it is root we
> are trying to protect from the symlink race.

So how does root override the DAC override ignore when she needs to ?

> 
> > Give your users their own /tmp. No kernel mods, no misbehaviours, no
> > weirdomatic path walking hackery. No kernel patch needed that I can see.
> 
> Some real applications expect to share /tmp (e.g. "screen").  Splitting up
> /tmp for every user isn't quickly feasible and requires every distro
> change.  I would note that separate /tmp also breaks the hypothetical
> binary game if it really is required to follow cross-uid symlinks.

I've yet to meet a game that tries to share a /tmp file across users.

> Fixing this in the kernel is trivial.  Fixing this in userspace is not
> trivial -- it requires that every distro change the very semantics of
> how /tmp works.  I can't say that's a bad idea, but I can say that
> it will not be globally successful.  This kernel changes breaks no
> applications, and fixes a whole class of problem forever, is small,
> and has well-defined rules.

At best you fix one of a zillion /tmp problems and corner cases from
rootfs cross linking into /var/mail to quota avoidance. Putting /tmp into
a per user context fixes the lot (and doesn't break screen for users).

> I would prefer it be solved at the VFS layer.  Doing it in commoncaps
> is possible (and is even what I proposed), but I want to avoid having
> everyone punt this patch around to different subsystem maintainers.

You can do it in an existing LSM, you can write your own new LSM if you
want. This is why we have LSM hooks.

> It's a real problem. I have proposed a real solution. If it's not okay,
> I'd like to see an alternative solution that you're comfortable with
> that fixes it to the same global scope.  (i.e. not userspace or tied to
> a specific LSM.)

The fact you can do it already rather clearly says your patch is not
needed. If you must do it then write your own Ubuntu LSM, that way nobody
has to care. If you want to actually *fix* /tmp then you want a per
login user or per session /tmp. A per session /tmp does confuse a few
things, a per login user tmp works nicely. About the only thing you do
need to tweak slightly is the gdm/kdm handing off of the session - both
to do the mount and to migrate the X socket into it.

Alan

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

[Kees Cook]

On Mon, May 31, 2010 at 07:09:36PM +0100, Alan Cox wrote:
> On Mon, 31 May 2010 10:50:08 -0700
> Kees Cook <kees.cook@canonical.com> wrote:
> > On Mon, May 31, 2010 at 11:23:14AM +0100, Alan Cox wrote:
> > > 1.We have things like SELinux so you can write rules to bound apps
> > >   which should be able to do this, if not then we should fix the security
> > >   policy to generally solve it
> > 
> > This requires that policy be written for all applications ever, which isn't
> > feasible.
> 
> Wrong, you can write a generic policy then write exceptions.

This depends on your LSM.  And some people run with no LSM at all.

> > nasty side-effect of making all my applications fail.  This patch isn't
> > that intrusive.  :)
> 
> I would disagree there, and it seems Christoph and others likjewise.

I think there is a difference between "offensive" and "intrusive".  The
patch itself is about 4 lines of logic.  That in itself isn't intrusive.
But a lot of people (now and in the past) find the change in VFS semantics
offensive.  What I'd like to do is get people to really think about what
this change means -- it's blocking a specific pathological situation that
has no positive outcome.  If we can all agree "this needs to be fixed",
then we can work on a common solution.

> > I do not believe there are any applications that depend on following
> > symlinks in sticky world-writable directories.  Besides, just because
> > something lacks source doesn't mean I can't change its behavior via binary
> > editing or LD_PRELOAD.  I do not feel that non-existent hypothetical
> > applications needing this feature is a sufficient reason to reject the
> > patch.
> 
> Maybe you are right - but you can do this without a kernel patch or as a
> security module and leave the rest of the kernel alone.

I see this as a flaw in DAC.  As such, I'd like to see it fixed globally.

> Plan 9 solved this years ago. Linux added the necessary per user /tmp (or
> per session /tmp if you prefer) support years ago.

Right, I don't disagree on this point -- having per-user /tmp is great, but
it hasn't happened because it's not trivial.  I feel that fixing symlink
following and per-user /tmp are tangential issues, both worthy of being
fixed.  And I realize we disagree on this point.  :)

> > > This is of minimal use - current->comm is arbitary and user controlled.
> > 
> > It's used strictly for reporting.  I'm open to other suggestions -- I just
> > want to give a system admin some information in the case where an
> > application is being attacked with /tmp symlink races.
> 
> current->comm can be a random mess of control codes, it could include a
> newline to fool the printk paths. You don't printk user provided
> untrusted bits...

Right, of course.  I was, however, modelling this off of existing warnings,
though perhaps I didn't do it right.  Here is kernel/capabilities.c,
warn_legacy_capability_use():

                printk(KERN_INFO "warning: `%s' uses 32-bit capabilities"
                       " (legacy support in use)\n",
                       get_task_comm(name, current));

If this is unsafe, then all users of get_task_comm() should be audited.  I
had the impression that current->comm was sanitized very early... hm, it's
not.  It's sanitized for newlines in fs/proc/array.c task_name(), but not
by other callers.  Seems like the sanitizing should happen in
get_task_comm().  Thoughts?

> So how does root override the DAC override ignore when she needs to ?

By turning off the entire sysctl.  It's designed to protect privileged
users, so it doesn't make sense to have it be bypassed.

> I've yet to meet a game that tries to share a /tmp file across users.

Well, right, that was my point -- no such application needs to follow
symlinks in /tmp across users.  Such symlinks would be impossible to create
with per-user /tmp mounts.  So, if per-user /tmp is a viable solution to
symlink races, then so is denying cross-user symlink following.

> At best you fix one of a zillion /tmp problems and corner cases from
> rootfs cross linking into /var/mail to quota avoidance. Putting /tmp into
> a per user context fixes the lot (and doesn't break screen for users).

IIRC, screen, when setuid, allows users to share screen sessions (following
some system-defined ACLs) but it does it via the /tmp directory trees it
creates.  Per-user /tmp would break this (but yes, it's solvable using some
kind of /var/lib/screen which maybe even already exists).

> You can do it in an existing LSM, you can write your own new LSM if you
> want. This is why we have LSM hooks.

Right, I'm well aware of that.  I was hoping to solve this problem for all
Linux users, since it has no operational downside.

> The fact you can do it already rather clearly says your patch is not
> needed. If you must do it then write your own Ubuntu LSM, that way nobody
> has to care.

I'm slightly sensitive here, as people (hi Greg KH :P) like to take
pot-shots at Ubuntu sometimes, so apologies if I've misunderstood you.
For some reason you've specifically brought up Ubuntu, which makes me
wonder if this patch would have been received differently if I had sent
it from my personal email account.  Regardless, you seem to be saying
a few things, and I want to make sure I get it:

 1) "I don't want this change to symlink logic in the mainline kernel"
 2) "Go add it to Ubuntu only"
 3) "Nobody will care if Ubuntu carries this patch"

I'm hoping to change your mind about #1 -- it has no downsides and only has
benefits.

#2 is already done -- but my goal is to protect everyone else.  While a
large share of Linux users run an Ubuntu kernel, there are still a lot of
other people that will continue to suffer from this vulnerability, and I
have what I feel is a moral obligation to try to get this fixed globally.

For #3, if by "nobody" you mean "no Linux kernel developer", then
I think your opinion lack credence since I consider myself a Linux
kernel developer (not a huge one, but I do contribute).  And I do care
about this, obviously.  If by "nobody" you mean "nobody using Linux",
then I again think your opinion lacks credence since I've already had
a few people glad to see this fixed in Ubuntu.  That's more than zero
people, so not "nobody".  If instead, by "nobody" you mean "no Linux VFS
developer" than I'm just puzzled since that would imply that all Linux VFS
developers don't care about fixing this problem (which I can't believe).
Or at least they don't care if the semantics change, as long as it's
not in the VFS core, which also makes no sense to me.  Either the logic
changes or it doesn't.  And since it needs to be changed, it seems that
everyone should be working together to fix it.

If instead, you mean "if it's changed in an LSM, then VFS developers
don't have to think about it", then I remain puzzled, since it's so
clearly in need of being fixed.  What does it matter where it goes?

> If you want to actually *fix* /tmp then you want a per
> login user or per session /tmp. A per session /tmp does confuse a few
> things, a per login user tmp works nicely. About the only thing you do
> need to tweak slightly is the gdm/kdm handing off of the session - both
> to do the mount and to migrate the X socket into it.

I totally agree.  But I view it as tangential to solving symlink races
(even though it does address it, that solution is a much different
endeavour).

What is the path to take for having this change in the mainline kernel,
and LSM-agnostic?  I want to see it solved globally.

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Al Viro]

On Mon, May 31, 2010 at 10:50:08AM -0700, Kees Cook wrote:

> > Give your users their own /tmp. No kernel mods, no misbehaviours, no
> > weirdomatic path walking hackery. No kernel patch needed that I can see.
> 
> Some real applications expect to share /tmp (e.g. "screen").

/tmp/screen-exchange, you mean?  Such a brilliant idea on multiuser system...

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

[Al Viro]

On Mon, May 31, 2010 at 12:07:54PM -0700, Kees Cook wrote:

> IIRC, screen, when setuid, allows users to share screen sessions (following
> some system-defined ACLs) but it does it via the /tmp directory trees it
> creates.  Per-user /tmp would break this (but yes, it's solvable using some
> kind of /var/lib/screen which maybe even already exists).

screen(1) does *not* put directories in /tmp these days, TYVM.

al@duke:~/linux/trees/vfs-next$ ls -l /var/run/screen/
total 1
drwx------ 2 al al 1024 May 20 21:50 S-al

That's lenny/x86_64; I can't be arsed to install ubuntu, but in case you
have a really ancient screen(1), pulling one from debian -stable would
suffice.  IIRC, -oldstable would work as well, actually, but I could be
wrong on that.

In any case, the suggested "improvement" breaks realistic use cases, AFAICS.
In particular,

cd /tmp
tar jxf foo-2.42.orig.tar.bz2
<...>
tar jxf foo-gtk-wank-wank-wank-2.69.orig.tar.bz2
<...>
ln -s foo-gtk-wank-wank-wank-2.69/docs/GNOME/design/ crap
<...>
lpr crap/taste-is-optional.ps
lpr crap/why-options-are-wrong.ps

is going to break with that, isn't it?

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

[Kees Cook]

Hi Al,

On Mon, May 31, 2010 at 08:52:30PM +0100, Al Viro wrote:
> On Mon, May 31, 2010 at 12:07:54PM -0700, Kees Cook wrote:
> > IIRC, screen, when setuid, allows users to share screen sessions (following
> > some system-defined ACLs) but it does it via the /tmp directory trees it
> > creates.  Per-user /tmp would break this (but yes, it's solvable using some
> > kind of /var/lib/screen which maybe even already exists).
> 
> screen(1) does *not* put directories in /tmp these days, TYVM.
> 
> al@duke:~/linux/trees/vfs-next$ ls -l /var/run/screen/
> total 1
> drwx------ 2 al al 1024 May 20 21:50 S-al

Okay, good; that's a relief.

> In any case, the suggested "improvement" breaks realistic use cases, AFAICS.
> In particular,
> 
> cd /tmp
> tar jxf foo-2.42.orig.tar.bz2
> <...>
> tar jxf foo-gtk-wank-wank-wank-2.69.orig.tar.bz2
> <...>
> ln -s foo-gtk-wank-wank-wank-2.69/docs/GNOME/design/ crap
> <...>
> lpr crap/taste-is-optional.ps
> lpr crap/why-options-are-wrong.ps
> 
> is going to break with that, isn't it?

Nope.  To be fair, it depends on the implementation of of LPR.  In the
case of CUPS, this is fine since lpr will run as the local user, follow
the symlink and read the file contents before POSTing the contents to
the CUPS server.  The privilege boundary is crossed at the network,
not the filesystem in this case.

I would note however that without the symlink following patch a hypothetical
attacker would be able to race you for the "foo-gtk-wank-wank-wank-2.69"
entry, or the "crap" entry, since either could be directed out from under
"tar" and "ln" to symlinks controlled by the attacker.  Unpacking archives
in a sticky world-writable directory is dangerous without this symlink
following patch.

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[James Morris]

On Mon, 31 May 2010, Kees Cook wrote:

> Expecting the push-back from VFS, I wrote this patch against commoncaps.
> 
> James, would you take it there (along with the patch to SELinux to call
> out to commoncaps) if there is no way to proceed with the VFS core towards
> solving this?

Isn't this just bypassing the VFS objections?


- James
-- 
James Morris
<jmorris@namei.org>

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

[Kees Cook]

On Tue, Jun 01, 2010 at 09:09:10AM +1000, James Morris wrote:
> On Mon, 31 May 2010, Kees Cook wrote:
> 
> > Expecting the push-back from VFS, I wrote this patch against commoncaps.
> > 
> > James, would you take it there (along with the patch to SELinux to call
> > out to commoncaps) if there is no way to proceed with the VFS core towards
> > solving this?
> 
> Isn't this just bypassing the VFS objections?

Well, that's what I'm trying to understand.  It sounds like there is some
general agreement that the issue needs to be solved, but some folks do not
want it in the core VFS.  As in, the objections aren't with how symlink
behavior is changed, just that the changes would be in the fs/ directory.

My rationale is that if it's in commoncaps, it's effective for everyone, so
it might as well be in core VFS.  If the VFS objections really do boil down
to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

Ironically, doing this in commoncaps is the patch I sent.  :P

To me, the VFS objections really seem like a obfuscated version of
"this should not be in the kernel in any way", which I think does not
admit to the problem existing in the first place.  Either there is a
problem or there isn't.  If there is a problem, it should be fixed.
I think there is a problem with the kernel semantics of symlinks.
Therefore, it should be fixed in the kernel.

I personally do not care if it's in fs/ or security/, I'm just seeking
the most efficient and complete solution.

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Christoph Hellwig]

On Mon, May 31, 2010 at 08:24:23PM -0700, Kees Cook wrote:
> Well, that's what I'm trying to understand.  It sounds like there is some
> general agreement that the issue needs to be solved, but some folks do not
> want it in the core VFS.  As in, the objections aren't with how symlink
> behavior is changed, just that the changes would be in the fs/ directory.

No, it's not.  It's not a change we can make for the default that
everyone uses.  If you're keen to mess up installations you control (aka
ubuntu valuedadd viper) push it into a special LSM or rather a
non-standard rule for it.  It really doesn't matter if it's in fs/ or
security/ but it's simplify not going to happen by default.

> My rationale is that if it's in commoncaps, it's effective for everyone, so
> it might as well be in core VFS.  If the VFS objections really do boil down
> to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking
some really bad stuff.

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

[Eric Paris]

On Tue, 2010-06-01 at 03:55 -0400, Christoph Hellwig wrote:
> On Mon, May 31, 2010 at 08:24:23PM -0700, Kees Cook wrote:

> > My rationale is that if it's in commoncaps, it's effective for everyone, so
> > it might as well be in core VFS.  If the VFS objections really do boil down
> > to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.
> 
> If you think the objection is about having things in fs/ you're smoking
> some really bad stuff.

Sounds to me like we should probably follow the same path as
mmap_min_addr.  We should add these hooks right in the VFS where they
belong (much like mmap_min_addr hooks into the vm) and control them 2
ways.

1) a Kconfig so distros can choose to turn it on or off by default
2) a /proc interface so root can turn it off

Nothing about that precludes additional similar checks inside an LSM
(like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled.  So
maybe we want to follow up with the core VFS check with new checks in
SELinux (and maybe apparmour).  This allows the user to disable the
general check and still be provided with some modicum of protection.
You might ask why not ONLY do the check in SELinux and drop the generic
check, but we have seen with mmap_min_addr that the SELinux unconfined
user can do damn well anything it wants to, so having a non-LSM version
of appropriate security checks is highly regarded.

(see eparis.livejournal.com for a long discussion on the LSM creating a
weaker machine in some cases when there are no non-LSM checks)

-Eric

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

[Kees Cook]

On Tue, Jun 01, 2010 at 07:55:02AM -0400, Eric Paris wrote:
> On Tue, 2010-06-01 at 03:55 -0400, Christoph Hellwig wrote:
> > On Mon, May 31, 2010 at 08:24:23PM -0700, Kees Cook wrote:
> 
> > > My rationale is that if it's in commoncaps, it's effective for everyone, so
> > > it might as well be in core VFS.  If the VFS objections really do boil down
> > > to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.
> > 
> > If you think the objection is about having things in fs/ you're smoking
> > some really bad stuff.
> 
> Sounds to me like we should probably follow the same path as
> mmap_min_addr.  We should add these hooks right in the VFS where they
> belong (much like mmap_min_addr hooks into the vm) and control them 2
> ways.
> 
> 1) a Kconfig so distros can choose to turn it on or off by default
> 2) a /proc interface so root can turn it off
> 
> Nothing about that precludes additional similar checks inside an LSM
> (like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled.  So
> maybe we want to follow up with the core VFS check with new checks in
> SELinux (and maybe apparmour).  This allows the user to disable the
> general check and still be provided with some modicum of protection.
> You might ask why not ONLY do the check in SELinux and drop the generic
> check, but we have seen with mmap_min_addr that the SELinux unconfined
> user can do damn well anything it wants to, so having a non-LSM version
> of appropriate security checks is highly regarded.

Would a CONFIG for this be overkill?  mmap_min_addr is a little different
in that there was desire to control a bottom limit on it, etc.  Given this
is either "on" or "off", I think just a sysctl is needed?

I will send a v3 patch that fixes the sysctl name and default, so that it
is up to the distro and end user how to configure their symlink semantics.

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Kees Cook]

On Tue, Jun 01, 2010 at 03:55:29AM -0400, Christoph Hellwig wrote:
> On Mon, May 31, 2010 at 08:24:23PM -0700, Kees Cook wrote:
> > Well, that's what I'm trying to understand.  It sounds like there is some
> > general agreement that the issue needs to be solved, but some folks do not
> > want it in the core VFS.  As in, the objections aren't with how symlink
> > behavior is changed, just that the changes would be in the fs/ directory.
> 
> No, it's not.  It's not a change we can make for the default that
> everyone uses.  If you're keen to mess up installations you control (aka
> ubuntu valuedadd viper) push it into a special LSM or rather a
> non-standard rule for it.  It really doesn't matter if it's in fs/ or
> security/ but it's simplify not going to happen by default.

Okay, thanks; that clarifies some of my confusion.  It sounds like
there are some people that genuinely believe that the symlink-following
logic should not change.  I would pose, then, a question of "what
are legitimate and safe situations that require following cross-user
symlinks in a sticky world-writable directory?"  And if the answers to
that aren't very convincing, then I think it's reasonable to include at
least an option to change the behavior.

> > My rationale is that if it's in commoncaps, it's effective for everyone, so
> > it might as well be in core VFS.  If the VFS objections really do boil down
> > to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.
> 
> If you think the objection is about having things in fs/ you're smoking
> some really bad stuff.

Right, that was my point exactly.  It didn't make sense to object to it
being in fs/.  The objection was to having it in the kernel at all.  So now
I can focus my efforts on convincing people about the value of making this
a setting in the kernel, like turning on or off TCP syn-flood protection.
Some people may demand it, some people may hate it, but the choice it
up to the end user.

-Kees

-- 
Kees Cook
Ubuntu Security Team

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

[Eric Paris]

On Tue, 2010-06-01 at 07:52 -0700, Kees Cook wrote:
> On Tue, Jun 01, 2010 at 07:55:02AM -0400, Eric Paris wrote:
> > On Tue, 2010-06-01 at 03:55 -0400, Christoph Hellwig wrote:
> > > On Mon, May 31, 2010 at 08:24:23PM -0700, Kees Cook wrote:
> > 
> > > > My rationale is that if it's in commoncaps, it's effective for everyone, so
> > > > it might as well be in core VFS.  If the VFS objections really do boil down
> > > > to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.
> > > 
> > > If you think the objection is about having things in fs/ you're smoking
> > > some really bad stuff.
> > 
> > Sounds to me like we should probably follow the same path as
> > mmap_min_addr.  We should add these hooks right in the VFS where they
> > belong (much like mmap_min_addr hooks into the vm) and control them 2
> > ways.
> > 
> > 1) a Kconfig so distros can choose to turn it on or off by default
> > 2) a /proc interface so root can turn it off
> > 
> > Nothing about that precludes additional similar checks inside an LSM
> > (like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled.  So
> > maybe we want to follow up with the core VFS check with new checks in
> > SELinux (and maybe apparmour).  This allows the user to disable the
> > general check and still be provided with some modicum of protection.
> > You might ask why not ONLY do the check in SELinux and drop the generic
> > check, but we have seen with mmap_min_addr that the SELinux unconfined
> > user can do damn well anything it wants to, so having a non-LSM version
> > of appropriate security checks is highly regarded.
> 
> Would a CONFIG for this be overkill?  mmap_min_addr is a little different
> in that there was desire to control a bottom limit on it, etc.  Given this
> is either "on" or "off", I think just a sysctl is needed?

Seems like one of Alan's main arguments is that you should not turn it
on 'by default.'  I assume most distros will want it on by default.
Alan made the same argument against mmap_min_addr (known to break dos
emu) but I think most major distros have it on by default these days
even if it does break those weird obscure use cases.  I guess distros
can do it through sysctl but Fedora, at least, likes to keep those
default if possible, which is why I suggested the CONFIG.  In any case,
putting this right square in the VFS where it happens makes the most
sense to me.

I'd also like to point out that I don't buy the argument that per
user /tmp/ is a 'better' solution for the general case.  Any application
that would be broken by this change will also be broken by per
user /tmp.  Now, if we used filesystem namespaces regularly for years
and users, administrators, and developers dealt with them often I agree
that would probably be the preferred solution.  It would solve this
issue, but in introduces a whole host of other problems that are even
more obvious and even likely to bite people.

I probably would move the security hook down into __do_follow_link and
put this check down there as well, but I think you still have a problem
with d_parent.  I don't see what keeps d_parent from being freed while
you are using it....

-Eric

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

[tytso]

On Tue, Jun 01, 2010 at 11:34:37AM -0400, Eric Paris wrote:
> 
> Seems like one of Alan's main arguments is that you should not turn it
> on 'by default.'  I assume most distros will want it on by default.
> Alan made the same argument against mmap_min_addr (known to break dos
> emu) but I think most major distros have it on by default these days
> even if it does break those weird obscure use cases.  I guess distros
> can do it through sysctl but Fedora, at least, likes to keep those
> default if possible, which is why I suggested the CONFIG.  In any case,
> putting this right square in the VFS where it happens makes the most
> sense to me.

If the use cases where people would really want to support following a
third-party symlink located in a world-writable sticky-bitted
directory are real (I'm not really convinced they really are real),
then it might make more sense to do it as a sysctl-controlled variable
with distro's chooinsg to enable it by default in their shipped
/etc/sysctl.conf file.  That way their users/customers will be able to
more easily disable the security feature if it really matters.

That being said, I'm not at all convinced there are any legtimiate use
cases that would be impacted by this change, and as I've pointed out
earlier, I don't think the "POSIX doesn't allow this change" argument
holds water, since POSIX originally didn't even allow for the
semantics for the sticky-bit on directories.  The real argument is
going to come from traditionalists, who will argue, "that isn't the
traditional behaviour!"

How strongly you buy this as an argument is going to be religious
argument, since it's doubtful that it people can come up with real use
cases that will actually break with that sort of change --- especially
given that the vast majority of Linux systems are single-user systems,
and the main concern will be a privilege escalation attacks.

> I'd also like to point out that I don't buy the argument that per
> user /tmp/ is a 'better' solution for the general case.  Any application
> that would be broken by this change will also be broken by per
> user /tmp.  Now, if we used filesystem namespaces regularly for years
> and users, administrators, and developers dealt with them often I agree
> that would probably be the preferred solution.  It would solve this
> issue, but in introduces a whole host of other problems that are even
> more obvious and even likely to bite people.

Per-user /tmp solves other problems as well, though --- especially for
people who care a whole lot about mandatory access control scenarios,
for example.  

I do have a slight preference against per-user /tmp mostly because it
gets confusing for administrators, and because it could be used by
rootkits to hide themselves in ways that would be hard for most system
administrators to find.

					- Ted