[PATCH v6] fs: allow protected cross-uid sticky symlinks

[PATCH v6] fs: allow protected 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 opening a file through a given
symlink (i.e. a root process opens 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 opened when outside a sticky
world-writable directory, or when the uid of the symlink and opener 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, but with the
scope changed to be only "opening" a symlink.  I have added a sysctl to
enable the protected behavior, documentation, and a ratelimited warning.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---

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

v3:
 - move into VFS core.
 - add CONFIG entry for build-time default.
 - rename sysctl, invert logic.
 - use get_task_comm for task name.
 - lock dentry when checking parent.

v4:
 - limit check to leaf symlink opening.

v5:
 - Kconfig whitespace regressed (thanks to Randy Dunlap for pointing it out)

v6:
 - move sysctl extern to fs.h.
---
 Documentation/sysctl/fs.txt |   15 ++++++++++
 fs/Kconfig                  |   15 ++++++++++
 fs/namei.c                  |   61 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h          |    1 +
 kernel/sysctl.c             |    9 ++++++
 5 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 6268250..9986bce 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -32,6 +32,7 @@ Currently, these files are in /proc/sys/fs:
 - nr_open
 - overflowuid
 - overflowgid
+- protected-sticky-symlinks
 - suid_dumpable
 - super-max
 - super-nr
@@ -158,6 +159,20 @@ The default is 65534.
 
 ==============================================================
 
+protected-sticky-symlinks:
+
+Opening symlinks in sticky world-writable directories (like /tmp) can be
+dangerous due to time-of-check-time-of-use races that frequently result
+in security vulnerabilities.
+
+The default value is "0", leaving the behavior of symlink opening
+unchanged from POSIX.  A value of "1" will enable the protection, causing
+symlinks to be openable only if outside a sticky world-writable directory,
+or if the symlink and the opener's uid match, or if the symlink and its
+directory are owned by the same uid.
+
+==============================================================
+
 suid_dumpable:
 
 This value can be used to query and set the core dump mode for setuid
diff --git a/fs/Kconfig b/fs/Kconfig
index 5f85b59..b2cdff3 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -256,3 +256,18 @@ source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
 endmenu
+
+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink opening in sticky world-writable directories"
+	help
+	  A long-standing class of security issues is the symlink-based
+	  time-of-check-time-of-use race, most commonly seen in
+	  world-writable directories like /tmp. The common method of
+	  exploitation of this flaw is to cross privilege boundaries
+	  when opening a given symlink (i.e. a root process opens a
+	  malicious symlink belonging to another user).
+
+	  Enabling this solves the problem by permitting symlinks to only
+	  be opened when outside a sticky world-writable directory, or
+	  when the uid of the symlink and opener match, or when the
+	  directory and symlink owners match.
diff --git a/fs/namei.c b/fs/namei.c
index 868d0cb..ee9d493 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -32,6 +32,7 @@
 #include <linux/fcntl.h>
 #include <linux/device_cgroup.h>
 #include <linux/fs_struct.h>
+#include <linux/ratelimit.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
@@ -530,6 +531,60 @@ static inline void path_to_nameidata(struct path *path, struct nameidata *nd)
 	nd->path.dentry = path->dentry;
 }
 
+int protected_sticky_symlinks = CONFIG_PROTECTED_STICKY_SYMLINKS;
+
+/**
+ * may_open_sticky_symlink - Check symlink opening for unsafe situations
+ * @dentry: The inode/dentry of the symlink
+ * @nameidata: The path data of the symlink
+ *
+ * In the case of the protected_sticky_symlinks sysctl being enabled,
+ * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
+ * in a sticky world-writable directory.  This is to protect privileged
+ * processes from failing races against path names that may change out
+ * from under them by way of other users creating malicious symlinks.
+ * It will permit symlinks to only be opened when outside a sticky
+ * world-writable directory, or when the uid of the symlink and opener
+ * match, or when the directory owner matches the symlink's owner.
+ *
+ * Returns 0 if opening the symlink is allowed, -ve on error.
+ */
+static __always_inline int
+may_open_sticky_symlink(struct dentry *dentry, struct nameidata *nameidata)
+{
+	int error = 0;
+	const struct inode *parent;
+	const struct inode *inode;
+	const struct cred *cred;
+
+	if (!protected_sticky_symlinks)
+		return 0;
+
+	/* owner and opener match? */
+	cred = current_cred();
+	inode = dentry->d_inode;
+	if (cred->fsuid == inode->i_uid)
+		return 0;
+
+	/* check parent directory mode and owner */
+	spin_lock(&dentry->d_lock);
+	parent = dentry->d_parent->d_inode;
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid) {
+		error = -EACCES;
+	}
+	spin_unlock(&dentry->d_lock);
+
+	if (error) {
+		char name[sizeof(current->comm)];
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"opening attempted in sticky world-writable "
+			"directory by %s (fsuid %d)\n",
+			get_task_comm(name, current), cred->fsuid);
+	}
+	return error;
+}
+
 static __always_inline int
 __do_follow_link(struct path *path, struct nameidata *nd, void **p)
 {
@@ -1844,6 +1899,12 @@ reval:
 			goto exit_dput;
 		if (count++ == 32)
 			goto exit_dput;
+
+		/* check if this symlink is in a sticky world-write dir */
+		error = may_open_sticky_symlink(path.dentry, &nd);
+		if (error)
+			goto exit_dput;
+
 		/*
 		 * This is subtle. Instead of calling do_follow_link() we do
 		 * the thing by hands. The reason is that this way we have zero
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3428393..0daecd6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -405,6 +405,7 @@ extern void __init files_init(unsigned long);
 extern struct files_stat_struct files_stat;
 extern int get_max_files(void);
 extern int sysctl_nr_open;
+extern int protected_sticky_symlinks;
 extern struct inodes_stat_t inodes_stat;
 extern int leases_enable, lease_break_time;
 #ifdef CONFIG_DNOTIFY
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 997080f..431f013 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1455,6 +1455,15 @@ static struct ctl_table fs_table[] = {
 #endif
 #endif
 	{
+		.procname	= "protected-sticky-symlinks",
+		.data		= &protected_sticky_symlinks,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
+	{
 		.procname	= "suid_dumpable",
 		.data		= &suid_dumpable,
 		.maxlen		= sizeof(int),
-- 
1.7.0.4


-- 
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Alan Cox]

> Past objections and rebuttals could be summarized as:

You've forgotten to update this with the list of the objections from your
last few days postings.

You've forgotten to update it as suggested so its a security policy


Do you plan to post this daily until we get fed up of seeing it ?

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Kees Cook]

On Thu, Jun 03, 2010 at 10:41:49AM +0100, Alan Cox wrote:
> > Past objections and rebuttals could be summarized as:
> 
> You've forgotten to update this with the list of the objections from your
> last few days postings.

I didn't think the recent discussions added anything thematically new.
"It changes how symlinks work" is a variation on "breaks POSIX", and
"should be done with per-user /tmp" is a variabtion on "userspace should
fix it".  I can certainly reword the commit log, though.

> You've forgotten to update it as suggested so its a security policy

It is a sysctl with a CONFIG, which is what Eric Paris was asking for.
I apologize if I missed something, but if there are further improvements
desired, I'm happy to add patches.

> Do you plan to post this daily until we get fed up of seeing it ?

I plan on getting this functionality into the kernel.  As such, whenever
I've been directed to improve it before it will be accepted, I will send
an updated version.  Having the lifecycle of this patch blocking on me
seems counter-productive and slightly rude.

At this point, I believe I've addressed the specific concerns that Al Viro,
Eric Paris, and a few others pointed out.  What else needs fixing?

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v6] fs: allow protected 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 opening a file through a given
> symlink (i.e. a root process opens 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 opened when outside a sticky
> world-writable directory, or when the uid of the symlink and opener 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, but with the
> scope changed to be only "opening" a symlink.  I have added a sysctl to
> enable the protected behavior, documentation, and a ratelimited warning.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

This approach to fix the problem to of /tmp looks to me like it
will have the opposite effect.  I think this patch will encourage
more badly written applications.

This patch does not actually fix the problem of recurring security
issues in /tmp.  Fundamentally /tmp is a design flaw. You have
evidence that even the introduction of the sticky bit, and O_NOFOLLOW
did not fix this design flaw, I don't see how yet another kernel feature
will make finally make /tmp secure.  The design flaw in /tmp is that we
encourage storing files that we care little about (temporary files) in
a location that is shared between everyone.  From a security point of
view sharing of files needs to be done very carefully, and as such
should only be done for files that we really care about and intend to
share.

Furthermore the set of applications with security problems you are intending
to address are privileged applications.   There is no excuse not to audit
and fix those applications.  If you include this patch I see no reason why
audits won't get more lax and other related security issues won't crop up.

The real solution to this problem of inadvertently sharing things that are
not safe to be shared is not to have a shared /tmp at all.

Per user tmp can be as simple as:
export TMPDIR=$HOME/tmp/
chmod 0500 /tmp

So is your interest in actually fixing this problem, or do you just
want to encourage more bad applications to be written without worrying
about inadvertent sharing in /tmp?

Eric

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Kees Cook]

On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> 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
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> This approach to fix the problem to of /tmp looks to me like it
> will have the opposite effect.  I think this patch will encourage
> more badly written applications.

How to safely deal with /tmp has been well understood for well over
a decade.  I don't think this change would "encourage" poor code.

> This patch does not actually fix the problem of recurring security
> issues in /tmp.  Fundamentally /tmp is a design flaw. You have
> evidence that even the introduction of the sticky bit, and O_NOFOLLOW
> did not fix this design flaw, I don't see how yet another kernel feature
> will make finally make /tmp secure.  The design flaw in /tmp is that we
> encourage storing files that we care little about (temporary files) in
> a location that is shared between everyone.  From a security point of
> view sharing of files needs to be done very carefully, and as such
> should only be done for files that we really care about and intend to
> share.

I agree about /tmp being perilous.  This change, however, fixes a class of
problems that exist now and has a simple solution.

> Furthermore the set of applications with security problems you are intending
> to address are privileged applications.   There is no excuse not to audit
> and fix those applications.  If you include this patch I see no reason why
> audits won't get more lax and other related security issues won't crop up.

If there were no excuse, then all software would be audited.  This just
isn't the reality of our world.  Auditing is not a binary process and
auditing for security issues is, unfortunately, not a high priority for
many projects or organizations.  If auditing was critically important,
able to find all flaws, and there was no excuse not to do it, then Linux
would never have any security vulnerabilities.  I do not believe in
perfect security; I believe in layered security.  Find and fix what we
can when we can.  This change provides coverage for all software running
on the kernel for this flaw.

> The real solution to this problem of inadvertently sharing things that are
> not safe to be shared is not to have a shared /tmp at all.
> 
> Per user tmp can be as simple as:
> export TMPDIR=$HOME/tmp/
> chmod 0500 /tmp

Again, I agree the traditional /tmp should ultimately go away.  It is,
however, not that simple today.  I'm all for having distros go this
route and fix the various issues surrounding getting rid of the common
/tmp area.  This patch is to solve one specific problem with no trade-off.

> So is your interest in actually fixing this problem, or do you just
> want to encourage more bad applications to be written without worrying
> about inadvertent sharing in /tmp?

I obviously don't believe it is one or the other.  I want to fix /tmp
symlink races today and then see to fixing /tmp sharing next.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Al Viro]

On Thu, Jun 03, 2010 at 11:40:54AM -0700, Kees Cook wrote:

> At this point, I believe I've addressed the specific concerns that Al Viro,
> Eric Paris, and a few others pointed out.  What else needs fixing?

The hell you have.  Let me spell it out for you:

1) You _still_ have not posted the analysis of changes it causes, let alone
explained why they are the right thing to do.

2) You are still doing that for each symlink, no matter where in the path
it might be.  Do (1) and you'll see why it is a BS.

3) You have not bothered to explain why e.g. stat(2) should fail on such
symlinks.  Nevermind figuring out which syscalls need that and which do
not.  Again, (1) would be the starting point required for the rest.  And
it is needed to decide how to deal with these checks.  Really.

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Kees Cook]

On Fri, Jun 04, 2010 at 05:39:06AM +0100, Al Viro wrote:
> On Thu, Jun 03, 2010 at 11:40:54AM -0700, Kees Cook wrote:
> 
> > At this point, I believe I've addressed the specific concerns that Al Viro,
> > Eric Paris, and a few others pointed out.  What else needs fixing?
> 
> The hell you have.  Let me spell it out for you:
> 
> 1) You _still_ have not posted the analysis of changes it causes, let alone
> explained why they are the right thing to do.

I have to say I don't know specifically what you want here.  Do you
want runtime analysis?  What sort of analysis?  What would be meaningful
for you?  I have a small testsuite[1] that I've been using to validate
my patch attempts.  That's a form of analysis.

As for "why", I thought that was already pretty clear: stop the largest
class of malicious symlink races that read, write, or truncate files
through a single-depth symlink living in a sticky world-writable
directory.

I cannot know everything, but I can demonstrate that this method is
well tested in the traditionally security-hardened distros like OWL
and grsecurity.  This isn't some new crazy idea, it's an old crazy idea
that happens to provide a solid protection.  Could it be better?  Maybe.
But let's work towards incremental improvement.

> 2) You are still doing that for each symlink, no matter where in the path
> it might be.  Do (1) and you'll see why it is a BS.

I've asked for help on this, and I'm sorry I keep getting it wrong.
Based on your hints on earlier patches, I chose do_filp_open() for v4 of
the patch.  It seems I was right, at least in that part, based on your
comment[2] on LWN that actually answers the question I asked earlier[3]
on lkml that went unanswered.

I'd really like to be improving this patch in one thread of discussion
instead of having to go collecting hints from multiple sources.  Your
objection here now is that I still did it wrong.  Can you please help?
This is the point of collaborative development, right?  It seems to
me that it's really close (near "do_last", in the check for "trailing
symlink") so I'd really appreciate some better direction.

> 3) You have not bothered to explain why e.g. stat(2) should fail on such
> symlinks.  Nevermind figuring out which syscalls need that and which do
> not.  Again, (1) would be the starting point required for the rest.  And
> it is needed to decide how to deal with these checks.  Really.

Well, I think this is a bit of a tangent, but okay.  I would say stat(2)
is blocked because it's inconsistent to allow stat but block open.
But I also feel like this is a trick question.  :)  I was originally for
blocking all following, since that doesn't seem to actually break any
valid use-cases.  But I'm happy to more finely specify the limitations.
I was figuring it provided a more understandable behavior in the more
general case.

You mention in the later LWN comments:

> It's not a matter of overhead; it's not that large to start with. The
> real issue here is that you generally want the behaviour of system to
> make sense and its mental model to be understandable. I.e. the rationale
> for restrictions should be simple and specific; the more arbitrary they
> are, the worse.
> 
> Again, I have no serious objections against that kind of restrictions.
> open()/creat() and probably truncate() on the final step of pathname
> resolution. But it really needs to be done right.

Can you propose a patch you're happy with?  You seem to have a very clear
understanding of what you want to see that would fix this, and you know the
code much better than I do.

Thanks,

-Kees

[1] http://bazaar.launchpad.net/%7Eubuntu-bugcontrol/qa-regression-testing/master/annotate/head%3A/scripts/test-kernel-hardening.py#L62
[2] http://lwn.net/Articles/390889/
[3] http://lkml.org/lkml/2010/6/1/473

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

(Sorry for the late reply, didn't have time last few days to drink from the
lkml firehose)

On Thu, 03 Jun 2010 14:00:51 PDT, Kees Cook said:
> On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> > 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
> > 
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > This approach to fix the problem to of /tmp looks to me like it
> > will have the opposite effect.  I think this patch will encourage
> > more badly written applications.
> 
> How to safely deal with /tmp has been well understood for well over
> a decade.  I don't think this change would "encourage" poor code.

The fact that you're proposing this patch a decade after we "well understood"
the problem should suggest that it *will* encourage poor code, as the same
programmers who don't currently get it right (and are thus the targets of your
patch) will quite likely just say "Oh, I saw a patch for that, I don't have to
try to do it right..."


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Kees Cook]

On Mon, Jun 07, 2010 at 12:18:56PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 03 Jun 2010 14:00:51 PDT, Kees Cook said:
> > On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> > > 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
> > > 
> > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > 
> > > This approach to fix the problem to of /tmp looks to me like it
> > > will have the opposite effect.  I think this patch will encourage
> > > more badly written applications.
> > 
> > How to safely deal with /tmp has been well understood for well over
> > a decade.  I don't think this change would "encourage" poor code.
> 
> The fact that you're proposing this patch a decade after we "well understood"
> the problem should suggest that it *will* encourage poor code, as the same
> programmers who don't currently get it right (and are thus the targets of your
> patch) will quite likely just say "Oh, I saw a patch for that, I don't have to
> try to do it right..."

This objection and its rebuttal are entirely conjecture and I don't think
it matters since we can never know the objective truth about the education
or motivation of nameless coders.  That said, I would assume that anyone
well-educated enough about /tmp races to think "oh I saw a kernel patch
for that" was either going to get it right to begin with or was going to
ignore best practices anyway.  The issue is more about the people that
just don't think about it at all.  I would argue that people are still
doing it, for over a decade, when it's still vulnerable, is evidence
that it is not something that will improve.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Eric W. Biederman]

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

> On Mon, Jun 07, 2010 at 12:18:56PM -0400, Valdis.Kletnieks@vt.edu wrote:
>> On Thu, 03 Jun 2010 14:00:51 PDT, Kees Cook said:
>> > On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
>> > > 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
>> > > 
>> > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> > > 
>> > > This approach to fix the problem to of /tmp looks to me like it
>> > > will have the opposite effect.  I think this patch will encourage
>> > > more badly written applications.
>> > 
>> > How to safely deal with /tmp has been well understood for well over
>> > a decade.  I don't think this change would "encourage" poor code.
>> 
>> The fact that you're proposing this patch a decade after we "well understood"
>> the problem should suggest that it *will* encourage poor code, as the same
>> programmers who don't currently get it right (and are thus the targets of your
>> patch) will quite likely just say "Oh, I saw a patch for that, I don't have to
>> try to do it right..."
>
> This objection and its rebuttal are entirely conjecture and I don't think
> it matters since we can never know the objective truth about the education
> or motivation of nameless coders. 

But we can know about distributions.

I see a patch put forward with the argument that it solves all the
problems of /tmp.  The patch does not actually solve all of the
problems of /tmp but more refined claims have not been made.

With the existence of that patch I see no interest in actually solving
the slightly more challenging problem at the distribution level of
unnecessary sharing in /tmp because a handful of uses of /tmp will
have to be fixed.

> That said, I would assume that anyone
> well-educated enough about /tmp races to think "oh I saw a kernel patch
> for that" was either going to get it right to begin with or was going to
> ignore best practices anyway.  The issue is more about the people that
> just don't think about it at all.  I would argue that people are still
> doing it, for over a decade, when it's still vulnerable, is evidence
> that it is not something that will improve.

Furthermore making it impossible to exploit the races with symlinks
(the best your patch can do).  Is not going to make the races impossible
to exploit by placing a different file in place of the one to be
created, or using hard links instead of symlinks.

So I would be surprised if after just your patch a system is actually
more secure from attack, once the attackers notice the change.

Eric

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Serge E. Hallyn]

Quoting Valdis.Kletnieks@vt.edu (Valdis.Kletnieks@vt.edu):
> (Sorry for the late reply, didn't have time last few days to drink from the
> lkml firehose)
> 
> On Thu, 03 Jun 2010 14:00:51 PDT, Kees Cook said:
> > On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> > > 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
> > > 
> > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > 
> > > This approach to fix the problem to of /tmp looks to me like it
> > > will have the opposite effect.  I think this patch will encourage
> > > more badly written applications.
> > 
> > How to safely deal with /tmp has been well understood for well over
> > a decade.  I don't think this change would "encourage" poor code.
> 
> The fact that you're proposing this patch a decade after we "well understood"
> the problem should suggest that it *will* encourage poor code, as the same
> programmers who don't currently get it right (and are thus the targets of your
> patch) will quite likely just say "Oh, I saw a patch for that, I don't have to
> try to do it right..."

Come on, now, that's a leap, really...

I'm all for doing both this patch AND pushing for per-user /tmp.

-serge

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Kees Cook]

On Mon, Jun 07, 2010 at 11:36:29AM -0700, Eric W. Biederman wrote:
> Kees Cook <kees.cook@canonical.com> writes:
> > On Mon, Jun 07, 2010 at 12:18:56PM -0400, Valdis.Kletnieks@vt.edu wrote:
> >> On Thu, 03 Jun 2010 14:00:51 PDT, Kees Cook said:
> >> > On Thu, Jun 03, 2010 at 01:02:48PM -0700, Eric W. Biederman wrote:
> >> > > 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
> >> > > 
> >> > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> > > 
> >> > > This approach to fix the problem to of /tmp looks to me like it
> >> > > will have the opposite effect.  I think this patch will encourage
> >> > > more badly written applications.
> >> > 
> >> > How to safely deal with /tmp has been well understood for well over
> >> > a decade.  I don't think this change would "encourage" poor code.
> >> 
> >> The fact that you're proposing this patch a decade after we "well understood"
> >> the problem should suggest that it *will* encourage poor code, as the same
> >> programmers who don't currently get it right (and are thus the targets of your
> >> patch) will quite likely just say "Oh, I saw a patch for that, I don't have to
> >> try to do it right..."
> >
> > This objection and its rebuttal are entirely conjecture and I don't think
> > it matters since we can never know the objective truth about the education
> > or motivation of nameless coders. 
> 
> But we can know about distributions.
> 
> I see a patch put forward with the argument that it solves all the
> problems of /tmp.  The patch does not actually solve all of the
> problems of /tmp but more refined claims have not been made.

I never claimed this solved "all the problems of /tmp".  I think my
original descriptions were not specific enough, and I made a refined
claim for what it should be expected to solve:

"stop the largest class of malicious symlink races that read, write, or
truncate files through a single-depth symlink living in a sticky
world-writable directory."  -- http://lkml.org/lkml/2010/6/4/44

> With the existence of that patch I see no interest in actually solving
> the slightly more challenging problem at the distribution level of
> unnecessary sharing in /tmp because a handful of uses of /tmp will
> have to be fixed.

We seem to disagree here.  I do concede that the urgency of per-user-/tmp
is reduced if this patch is in use.  I don't feel it eliminates the need
for per-user-/tmp, though.

> > That said, I would assume that anyone
> > well-educated enough about /tmp races to think "oh I saw a kernel patch
> > for that" was either going to get it right to begin with or was going to
> > ignore best practices anyway.  The issue is more about the people that
> > just don't think about it at all.  I would argue that people are still
> > doing it, for over a decade, when it's still vulnerable, is evidence
> > that it is not something that will improve.
> 
> Furthermore making it impossible to exploit the races with symlinks
> (the best your patch can do).  Is not going to make the races impossible
> to exploit by placing a different file in place of the one to be
> created, or using hard links instead of symlinks.

Right, so we agree: this patch doesn't eliminate the need for
per-user-/tmp.

> So I would be surprised if after just your patch a system is actually
> more secure from attack, once the attackers notice the change.

It sounds like you object on the basis that the patch does not provide
perfect security.  I do not feel perfect security is a realistic goal.
Incremental layered security is; this patch helps.  We can move on to the
next thing in need of securing after that.

If it helps to have a concrete example, I will choose the first
vulnerability of 2010 listed in a search for "/tmp" at Mitre:

 CVE-2010-0156: Puppet 0.24.x before 0.24.9 and 0.25.x before 0.25.2 allows
 local users to overwrite arbitrary files via a symlink attack on the (1)
 /tmp/daemonout, (2) /tmp/puppetdoc.txt, (3) /tmp/puppetdoc.tex, or (4)
 /tmp/puppetdoc.aux temporary file. 

The patch would make all cases unexploitable.  So, if you find a
system using Puppet before the above versions, with multiple local
users, where /tmp is its own filesystem, that system is more secure.
(Though to put a finer point on your "actually more secure" qualifier,
I would have to additionally add "with a malicious local attacker" to
my above system definition, since without the attacker, the system is
no more safe than it was, since there was no real danger to start with if
no one is attacking it.)


For the people objecting to what semantic changes the patch makes,
we clearly differ in opinion.  I and others think it is a valuable
protection, some do not.  I don't think this disagreement can be resolved
as it tends to boil down to "perfect security" vs "layered security",
which is why I don't object to having a sysctl.  I want to have the
choice to make my systems more secure without forcing it on for those
that want to wait for their distributions to provide a per-user /tmp.

For the people objecting to the actual code of the patch, I'm very
interested in finding a solution.  I am obviously not a Linux kernel VFS
maintainer.  I'm very keen to find an acceptable version of the patch
based on feedback from those that know this area of the kernel intimately.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v6] fs: allow protected cross-uid sticky symlinks

[Alan Cox]

> For the people objecting to the actual code of the patch, I'm very
> interested in finding a solution.  I am obviously not a Linux kernel VFS
> maintainer.  I'm very keen to find an acceptable version of the patch
> based on feedback from those that know this area of the kernel intimately.

You've already had those suggestions some days ago. Use a security module,
either by using something like SELinux (where you can do this just fine
as far as I can see including exceptions by label for problem apps), or
write your own little seucrity module that does it. Or for that matter
just leave it as an Ubuntu patch.

Since you didn't seem t get the hint earlier I also suspecifically NAK the
current patch.

Alan