[PATCH v2011.1] fs: symlink restrictions on sticky directories

[PATCH v2011.1] fs: symlink restrictions on sticky directories

[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
 2010 May, Kees Cook
  https://lkml.org/lkml/2010/5/30/144

Past objections and rebuttals could be summarized as:

 - Violates POSIX.
   - POSIX didn't consider this situation and it's not useful to follow
     a broken specification at the cost of security.
 - Might break unknown applications that use this feature.
   - Applications that break because of the change are easy to spot and
     fix. Applications that are vulnerable to symlink ToCToU by not having
     the change aren't. Additionally, no applications have yet been found
     that rely on this behavior.
 - Applications should just use mkstemp() or O_CREATE|O_EXCL.
   - True, but applications are not perfect, and new software is written
     all the time that makes these mistakes; blocking this flaw at the
     kernel is a single solution to the entire class of vulnerability.
 - This should live in the core VFS.
   - This should live in an LSM. (https://lkml.org/lkml/2010/5/31/135)
 - This should live in an LSM.
   - This should live in the core VFS. (https://lkml.org/lkml/2010/8/2/188)

This patch is based on the patch in Openwall and grsecurity, along with
suggestions from Al Viro. I have added a sysctl to enable the protected
behavior, documentation, and a ratelimited warning.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/sysctl/fs.txt |   21 ++++++++++++
 fs/Kconfig                  |   15 ++++++++
 fs/namei.c                  |   77 +++++++++++++++++++++++++++++++++++++++---
 kernel/sysctl.c             |   10 +++++
 4 files changed, 117 insertions(+), 6 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 88fd7f5..939621b 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
@@ -157,6 +158,26 @@ The default is 65534.
 
 ==============================================================
 
+protected_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 following a given symlink (i.e. a
+root process follows a symlink belonging to another user). For a likely
+incomplete list of hundreds of examples across the years, please see:
+http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp
+
+When set to "0", symlink following behavior is unrestricted.
+
+When set to "1" symlinks are permitted to be followed only when outside
+a sticky world-writable directory, or when the uid of the symlink and
+follower match, or when the directory owner matches the symlink's owner.
+
+This protection is based on the restrictions in Openwall and grsecurity.
+
+==============================================================
+
 suid_dumpable:
 
 This value can be used to query and set the core dump mode for setuid
diff --git a/fs/Kconfig b/fs/Kconfig
index 5f4c45d..74b9e49 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -278,3 +278,18 @@ source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
 endmenu
+
+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink following 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 following a given symlink (i.e. a root process follows
+	  a malicious symlink belonging to another user).
+
+	  Enabling this solves the problem by permitting 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 and symlink owners match.
diff --git a/fs/namei.c b/fs/namei.c
index 5008f01..c4d0bfc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -33,6 +33,7 @@
 #include <linux/device_cgroup.h>
 #include <linux/fs_struct.h>
 #include <linux/posix_acl.h>
+#include <linux/ratelimit.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
@@ -624,10 +625,69 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
 	path_put(link);
 }
 
+int protected_sticky_symlinks =
+#ifdef CONFIG_PROTECTED_STICKY_SYMLINKS
+				1;
+#else
+				0;
+#endif
+
+/**
+ * may_follow_link - Check symlink following 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 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.
+ *
+ * Returns 0 if following the symlink is allowed, -ve on error.
+ */
+static inline int
+may_follow_link(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;
+
+	/* Allowed if owner and follower 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 "
+			"following attempted in sticky world-writable "
+			"directory by %s (fsuid %d)\n",
+			get_task_comm(name, current), cred->fsuid);
+	}
+	return error;
+}
+
 static __always_inline int
-follow_link(struct path *link, struct nameidata *nd, void **p)
+follow_link(struct path *link, struct nameidata *nd, void **p, bool sensitive)
 {
-	int error;
+	int error = 0;
 	struct dentry *dentry = link->dentry;
 
 	BUG_ON(nd->flags & LOOKUP_RCU);
@@ -646,7 +706,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)
 	touch_atime(link->mnt, dentry);
 	nd_set_link(nd, NULL);
 
-	error = security_inode_follow_link(link->dentry, nd);
+	if (sensitive)
+		error = may_follow_link(link->dentry, nd);
+	if (!error)
+		error = security_inode_follow_link(link->dentry, nd);
 	if (error) {
 		*p = ERR_PTR(error); /* no ->put_link(), please */
 		path_put(&nd->path);
@@ -1339,7 +1402,7 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd)
 		struct path link = *path;
 		void *cookie;
 
-		res = follow_link(&link, nd, &cookie);
+		res = follow_link(&link, nd, &cookie, 0);
 		if (!res)
 			res = walk_component(nd, path, &nd->last,
 					     nd->last_type, LOOKUP_FOLLOW);
@@ -1612,7 +1675,8 @@ static int path_lookupat(int dfd, const char *name,
 			void *cookie;
 			struct path link = path;
 			nd->flags |= LOOKUP_PARENT;
-			err = follow_link(&link, nd, &cookie);
+
+			err = follow_link(&link, nd, &cookie, 1);
 			if (!err)
 				err = lookup_last(nd, &path);
 			put_link(nd, &link, cookie);
@@ -2324,7 +2388,8 @@ static struct file *path_openat(int dfd, const char *pathname,
 		}
 		nd->flags |= LOOKUP_PARENT;
 		nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
-		error = follow_link(&link, nd, &cookie);
+
+		error = follow_link(&link, nd, &cookie, 1);
 		if (unlikely(error))
 			filp = ERR_PTR(error);
 		else
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ae27196..cc2c5f9 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -92,6 +92,7 @@ extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern int max_threads;
 extern int core_uses_pid;
+extern int protected_sticky_symlinks;
 extern int suid_dumpable;
 extern char core_pattern[];
 extern unsigned int core_pipe_limit;
@@ -1495,6 +1496,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

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

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

Correct.

>  - Might break unknown applications that use this feature.
>    - Applications that break because of the change are easy to spot and
>      fix. Applications that are vulnerable to symlink ToCToU by not having
>      the change aren't. Additionally, no applications have yet been found
>      that rely on this behavior.

Are there *known* applications that break?

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

Right.

>  - This should live in the core VFS.
>    - This should live in an LSM. (https://lkml.org/lkml/2010/5/31/135)
>  - This should live in an LSM.
>    - This should live in the core VFS. (https://lkml.org/lkml/2010/8/2/188)

Ugh - and people continue to get exploited from a preventable, 
fixable and already fixed VFS design flaw.

> This patch is based on the patch in Openwall and grsecurity, 
> along with suggestions from Al Viro. I have added a sysctl to 
> enable the protected behavior, documentation, and a 
> ratelimited warning.

> +config PROTECTED_STICKY_SYMLINKS
> +	bool "Protect symlink following 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 following a given symlink (i.e. a root process follows
> +	  a malicious symlink belonging to another user).
> +
> +	  Enabling this solves the problem by permitting 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 and symlink owners match.

Unless there are known apps that regress, shouldnt this be 
default y? Even if we were forced to not actually release such a 
final kernel, doing it in -rc1 would flush weird apps out of the 
woodwork.

> +int protected_sticky_symlinks =

__read_mostly, most emphatically.

> +static inline int
> +may_follow_link(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;
> +
> +	/* Allowed if owner and follower 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);

Taking the global /tmp and /lib, /usr/lib dentry spinlocks here 
every time we follow a symlink is a bit sad: there are a lot of 
high-profile symlinks in a Linux installation in those places, 
followed by non-owners.

Nor is it really a realistic race that happens often: neither 
/tmp nor the library directories are being renamed or their 
permissions changed all that often for this parent lock taking 
to matter in practice.

Couldnt we somehow export this information outside the dentry 
lock, allowing safe lockless access to it?

> +
> +	if (error) {
> +		char name[sizeof(current->comm)];
> +		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
> +			"following attempted in sticky world-writable "
> +			"directory by %s (fsuid %d)\n",
> +			get_task_comm(name, current), cred->fsuid);

I don't think the get_task_comm() extra layer is really 
necessary here - this is called in the *current* task's context 
- and it's not like that tasks are changing their own names all 
that often while they are busy executing VFS code, right? The 
race with some other task writing to /proc/PID/comm is 
uninteresting.

The more important piece of forensic information to log would be 
the file name that got attempted and perhaps the directory name 
as well. If there's an unknown hole in an unknown privileged app 
then this warning alone does not give the admin any clues where 
to look - the name of the expoiting task is probably obfuscated 
anyway, it's the identity of the attack vector that matters - 
and that name can generally not be controlled and obfuscated by 
the attacker.

If those issues are resolved:

 Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Randy Dunlap]

On 12/06/2011 03:58 PM, Kees Cook wrote:
>  Documentation/sysctl/fs.txt |   21 ++++++++++++
>  fs/Kconfig                  |   15 ++++++++
>  fs/namei.c                  |   77 +++++++++++++++++++++++++++++++++++++++---
>  kernel/sysctl.c             |   10 +++++
>  4 files changed, 117 insertions(+), 6 deletions(-)


> diff --git a/fs/Kconfig b/fs/Kconfig
> index 5f4c45d..74b9e49 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -278,3 +278,18 @@ source "fs/nls/Kconfig"
>  source "fs/dlm/Kconfig"
>  
>  endmenu
> +
> +config PROTECTED_STICKY_SYMLINKS
> +	bool "Protect symlink following 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 following a given symlink (i.e. a root process follows
> +	  a malicious symlink belonging to another user).
> +
> +	  Enabling this solves the problem by permitting symlinks to only

better:

	  Enabling this solves the problem by permitting symlinks to be followed
	  only when the uid ...

> +	  be followed when outside a sticky world-writable directory,
> +	  or when the uid of the symlink and follower match, or when
> +	  the directory and symlink owners match.
> diff --git a/fs/namei.c b/fs/namei.c
> index 5008f01..c4d0bfc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -624,10 +625,69 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
> +
> +/**
> + * may_follow_link - Check symlink following 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 followed when outside a sticky

similar:

      It will permit symlinks to be followed only when outside a sticky

> + * world-writable directory, or when the uid of the symlink and follower
> + * match, or when the directory owner matches the symlink's owner.
> + *
> + * Returns 0 if following the symlink is allowed, -ve on error.
> + */
> +static inline int
> +may_follow_link(struct dentry *dentry, struct nameidata *nameidata)


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Kees Cook]

On Tue, Dec 6, 2011 at 11:30 PM, Ingo Molnar <mingo@elte.hu> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>
>> Past objections and rebuttals could be summarized as:
>> ...
>>  - Might break unknown applications that use this feature.
>>    - Applications that break because of the change are easy to spot and
>>      fix. Applications that are vulnerable to symlink ToCToU by not having
>>      the change aren't. Additionally, no applications have yet been found
>>      that rely on this behavior.
>
> Are there *known* applications that break?

Not that I've seen and none have been mentioned in any of the previous
discussions I've been able to find. (From above: "Additionally, no
applications have yet been found that rely on this behavior.")

>>  - This should live in the core VFS.
>>    - This should live in an LSM. (https://lkml.org/lkml/2010/5/31/135)
>>  - This should live in an LSM.
>>    - This should live in the core VFS. (https://lkml.org/lkml/2010/8/2/188)
>
> Ugh - and people continue to get exploited from a preventable,
> fixable and already fixed VFS design flaw.

Right.

>> +config PROTECTED_STICKY_SYMLINKS
>> +     bool "Protect symlink following 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 following a given symlink (i.e. a root process follows
>> +       a malicious symlink belonging to another user).
>> +
>> +       Enabling this solves the problem by permitting 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 and symlink owners match.
>
> Unless there are known apps that regress, shouldnt this be
> default y? Even if we were forced to not actually release such a
> final kernel, doing it in -rc1 would flush weird apps out of the
> woodwork.

I would totally agree. I was trying to be conservative here, but would
much rather this default to "y".

>> +int protected_sticky_symlinks =
>
> __read_mostly, most emphatically.

Ah yes. I will fix this.

>> +static inline int
>> +may_follow_link(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;
>> +
>> +     /* Allowed if owner and follower 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);
>
> Taking the global /tmp and /lib, /usr/lib dentry spinlocks here
> every time we follow a symlink is a bit sad: there are a lot of
> high-profile symlinks in a Linux installation in those places,
> followed by non-owners.
>
> Nor is it really a realistic race that happens often: neither
> /tmp nor the library directories are being renamed or their
> permissions changed all that often for this parent lock taking
> to matter in practice.
>
> Couldnt we somehow export this information outside the dentry
> lock, allowing safe lockless access to it?

I'm open to whatever is needed, though I think this change would be
outside the scope of this patch's intent.

>> +     if (error) {
>> +             char name[sizeof(current->comm)];
>> +             printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
>> +                     "following attempted in sticky world-writable "
>> +                     "directory by %s (fsuid %d)\n",
>> +                     get_task_comm(name, current), cred->fsuid);
>
> I don't think the get_task_comm() extra layer is really
> necessary here - this is called in the *current* task's context
> - and it's not like that tasks are changing their own names all
> that often while they are busy executing VFS code, right? The
> race with some other task writing to /proc/PID/comm is
> uninteresting.

I would rather not have any kind of race here. It's only during the
failure condition, so we don't need speed.

> The more important piece of forensic information to log would be
> the file name that got attempted and perhaps the directory name
> as well. If there's an unknown hole in an unknown privileged app
> then this warning alone does not give the admin any clues where
> to look - the name of the expoiting task is probably obfuscated
> anyway, it's the identity of the attack vector that matters -
> and that name can generally not be controlled and obfuscated by
> the attacker.

Agreed. I will work on extracting this information and presenting it sanely.

> If those issues are resolved:
>
>  Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks for looking it over!

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Kees Cook]

On Wed, Dec 7, 2011 at 10:22 AM, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On 12/06/2011 03:58 PM, Kees Cook wrote:
>> +       Enabling this solves the problem by permitting symlinks to only
>
> better:
>
>          Enabling this solves the problem by permitting symlinks to be followed
>          only when the uid ...
>
>> + * It will permit symlinks to only be followed when outside a sticky
>
> similar:
>
>      It will permit symlinks to be followed only when outside a sticky

Thanks! I'll update these.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Linus Torvalds]

On Tue, Dec 6, 2011 at 3:58 PM, Kees Cook <keescook@chromium.org> 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.

Ugh. I really dislike the implementation.

Wouldn't it be much nicer to instead actually use the symlink
protection fields, and make the rules be:

 - creating a symlink as root does the traditional "lrwxrwxrwx" thing
 - creating a symlink in a directory you own similarly does "lrwxrwxrwx"
 - creating a symlink anywhere else (which implies either sticky or
world-writable directory) does "lrwx------", so only you can use it.

That seems to be much nicer semantics, and makes the protection
*visible* instead of some kind of hacky run-time random behavior
depending on some invisible config option that people aren't even
aware of.

Of course, it needs to handle renames too (and maybe lchown?), and it
would still need to be an option you enable explicitly, but it seems
much more polite to make things like this something you can actually
see.

No, I have not thought this through deeply, but I really dislike your
kind of "change random semantics in ways that are very hidden and
subtle". The symlink protection approach seems to be much less hidden
and subtle, and should result in largely the same result.

Notably, you can install a system without it on, and turn it on even
after install - who cares if there are *long-term* symlinks with
lrwxrwxrwx and that thus allow all access, it's the racily-created
ones we need to worry about, so it should actually be perfectly ok to
enable the symlink creation protection dynamically. In fact, it could
be a inheritable per-*process* (or per-container) flag rather than a
global flag that says how symlink creation acts.

I dunno.

                    Linus

Re: [PATCH v2011.1] fs: symlink restrictions on sticky directories

[Kees Cook]

On Wed, Dec 7, 2011 at 10:41 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 6, 2011 at 3:58 PM, Kees Cook <keescook@chromium.org> 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.
>
> Ugh. I really dislike the implementation.
>
> Wouldn't it be much nicer to instead actually use the symlink
> protection fields, and make the rules be:
>
>  - creating a symlink as root does the traditional "lrwxrwxrwx" thing
>  - creating a symlink in a directory you own similarly does "lrwxrwxrwx"
>  - creating a symlink anywhere else (which implies either sticky or
> world-writable directory) does "lrwx------", so only you can use it.
>
> That seems to be much nicer semantics, and makes the protection
> *visible* instead of some kind of hacky run-time random behavior
> depending on some invisible config option that people aren't even
> aware of.
>
> Of course, it needs to handle renames too (and maybe lchown?), and it
> would still need to be an option you enable explicitly, but it seems
> much more polite to make things like this something you can actually
> see.
>
> No, I have not thought this through deeply, but I really dislike your
> kind of "change random semantics in ways that are very hidden and
> subtle". The symlink protection approach seems to be much less hidden
> and subtle, and should result in largely the same result.
>
> Notably, you can install a system without it on, and turn it on even
> after install - who cares if there are *long-term* symlinks with
> lrwxrwxrwx and that thus allow all access, it's the racily-created
> ones we need to worry about, so it should actually be perfectly ok to
> enable the symlink creation protection dynamically. In fact, it could
> be a inheritable per-*process* (or per-container) flag rather than a
> global flag that says how symlink creation acts.
>
> I dunno.

This seems like a much deeper and wider change. This would expand it
to non-sticky, non-world-writable directories as well, meaning
symlinks on fileservers couldn't be managed by non-directory owners
any more, etc. I think there would be a lot of negative fall-out from
that.

Also it wouldn't track the state of a potentially dangerous
environment. Creation isn't the problem; existing is the problem. As
an attacker, I could: "mkdir /tmp/foo; ln -s /etc/shadow
/tmp/foo/evil-symlink; mv /tmp/foo/evil-symlink /tmp" and suddenly the
lrwxrwxrwx would follow the mv. To fix this we'd have to change the
permissions across a move, or other weird hidden behaviors.

Given that the patch is tweaking what "sticky directory" means, I
don't think it's impolite. Only attackers have ever depended on this
behavior, so let's just eliminate it.

-Kees

-- 
Kees Cook
ChromeOS Security
--
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 v2011.1] fs: symlink restrictions on sticky directories

[Frank Kingswood]

On 08/12/11 02:41, Linus Torvalds wrote:
> On Tue, Dec 6, 2011 at 3:58 PM, Kees Cook<keescook@chromium.org>  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.
>
> Ugh. I really dislike the implementation.
>
> Wouldn't it be much nicer to instead actually use the symlink
> protection fields, and make the rules be:
>
>   - creating a symlink as root does the traditional "lrwxrwxrwx" thing
>   - creating a symlink in a directory you own similarly does "lrwxrwxrwx"
>   - creating a symlink anywhere else (which implies either sticky or
> world-writable directory) does "lrwx------", so only you can use it.
>
> That seems to be much nicer semantics, and makes the protection
> *visible* instead of some kind of hacky run-time random behavior
> depending on some invisible config option that people aren't even
> aware of.

This sounds really good way to go about it, but it seems that an 
implementation might be invasive.

Currently the symlinks are created in the file system e.g. ext4_symlink 
has the S_IFLNK|S_IRWXUGO buried in it. It is likely other fs's follow 
this model too.

Also, having tried creating such a link, it seems that the read-side 
permissions check (at least in generic_readlink) is missing.

Frank
-- 
------------------------------------------------------------------------
Frank A. Kingswood                      frank@kingswood-consulting.co.uk
Cambridge, United Kingdom                               +44-7545-209 100