fs: allow protected cross-uid sticky symlinks

A long-standing class of security issues is the symlink-based time-of-check-time-of-use race, most commonly seen in world-writable directories like /tmp. The common method of exploitation of this flaw is to cross privilege boundaries when 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.

---

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

index 6268250..9986bce 100644
@@ -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

index 5f85b59..b2cdff3 100644
@@ -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.

index 3428393..0daecd6 100644
@@ -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

index 997080f..431f013 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 ?

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.

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.

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

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.

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.

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.

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:

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

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

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.

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

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.

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.

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

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

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

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.

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

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

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.

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

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

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

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

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

-serge