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

[PATCH v2012.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 an audit notification.

Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
v2012.1:
 - Use GFP_KERNEL for audit log allocation, thanks to Ingo Molnar.
v2011.3:
 - Add pid/comm back to logging.
v2011.2:
 - Updated documentation, thanks to Randy Dunlap.
 - Switched Kconfig default to "y", added __read_mostly to sysctl,
   thanks to Ingo Molnar.
 - Switched to audit logging to gain safe path and name reporting when
   hitting the restriction.
v2011.1:
 - back from hiatus
---
 Documentation/sysctl/fs.txt |   21 ++++++++++
 fs/Kconfig                  |   16 ++++++++
 fs/namei.c                  |   87 ++++++++++++++++++++++++++++++++++++++++---
 kernel/sysctl.c             |   10 +++++
 4 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 88fd7f5..4b47cd5 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..26ede24 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
 endmenu
+
+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink following in sticky world-writable directories"
+	default y
+	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 be
+	  followed only 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..b826d2e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -624,10 +624,80 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
 	path_put(link);
 }
 
+int protected_sticky_symlinks __read_mostly =
+#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 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)
+{
+	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);
+
+#ifdef CONFIG_AUDIT
+	if (error) {
+		struct audit_buffer *ab;
+
+		ab = audit_log_start(current->audit_context,
+				     GFP_KERNEL, AUDIT_AVC);
+		audit_log_format(ab, "op=follow_link action=denied");
+		audit_log_format(ab, " pid=%d comm=", current->pid);
+		audit_log_untrustedstring(ab, current->comm);
+		audit_log_d_path(ab, "path=", &nameidata->path);
+		audit_log_format(ab, " name=");
+		audit_log_untrustedstring(ab, dentry->d_name.name);
+		audit_log_format(ab, " dev=%s ino=%lu",
+				 inode->i_sb->s_id,
+				 inode->i_ino);
+		audit_log_end(ab);
+	}
+#endif
+	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 +716,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 +1412,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 +1685,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 +2398,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 v2012.1] fs: symlink restrictions on sticky directories

[Ingo Molnar]

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

> @@ -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,
> +	},

Small detail:

Might make sense to change the .mode to 0600, to make it harder 
for unprivileged attack code to guess whether this protection 
(and the resulting audit warning to the administrator) is 
enabled on a system or not.

It can be probed, but only at the cost of generating a warning.

Likewise, distros should set /etc/sysctl.conf to 0600 as well, 
for similar reasons.

Thanks,

	Ingo

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

[Nick Bowler]

On 2012-01-04 12:18 -0800, 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.
[...]
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 5f4c45d..26ede24 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
>  source "fs/dlm/Kconfig"
>  
>  endmenu
> +
> +config PROTECTED_STICKY_SYMLINKS
> +	bool "Protect symlink following in sticky world-writable directories"
> +	default y
[...]

Why do we need a config option for this?  What's wrong with just using
the sysctl?

Why have you made this option "default y", when enabling it clearly
makes user-visible changes to kernel behaviour?

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

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

[Kees Cook]

On Thu, Jan 5, 2012 at 6:30 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
> On 2012-01-04 12:18 -0800, Kees Cook wrote:
>> diff --git a/fs/Kconfig b/fs/Kconfig
>> index 5f4c45d..26ede24 100644
>> --- a/fs/Kconfig
>> +++ b/fs/Kconfig
>> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
>>  source "fs/dlm/Kconfig"
>>
>>  endmenu
>> +
>> +config PROTECTED_STICKY_SYMLINKS
>> +     bool "Protect symlink following in sticky world-writable directories"
>> +     default y
> [...]
>
> Why do we need a config option for this?  What's wrong with just using
> the sysctl?

This way the sysctl can configured directly without needing to have a
distro add a new item to sysctl.conf.

> Why have you made this option "default y", when enabling it clearly
> makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

-Kees

-- 
Kees Cook
ChromeOS Security

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

[Kees Cook]

On Thu, Jan 5, 2012 at 1:17 AM, Ingo Molnar <mingo@elte.hu> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>
>> @@ -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,
>> +     },
>
> Small detail:
>
> Might make sense to change the .mode to 0600, to make it harder
> for unprivileged attack code to guess whether this protection
> (and the resulting audit warning to the administrator) is
> enabled on a system or not.

Sure, I have no problem with that. In addition to this change, what's
the best next step for this patch?

Thanks,

-Kees

-- 
Kees Cook
ChromeOS Security

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

[Nick Bowler]

On 2012-01-05 11:34 -0800, Kees Cook wrote:
> On Thu, Jan 5, 2012 at 6:30 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
> > On 2012-01-04 12:18 -0800, Kees Cook wrote:
> >> diff --git a/fs/Kconfig b/fs/Kconfig
> >> index 5f4c45d..26ede24 100644
> >> --- a/fs/Kconfig
> >> +++ b/fs/Kconfig
> >> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
> >>  source "fs/dlm/Kconfig"
> >>
> >>  endmenu
> >> +
> >> +config PROTECTED_STICKY_SYMLINKS
> >> +     bool "Protect symlink following in sticky world-writable directories"
> >> +     default y
> > [...]
> >
> > Why do we need a config option for this?  What's wrong with just using
> > the sysctl?
> 
> This way the sysctl can configured directly without needing to have a
> distro add a new item to sysctl.conf.

This seems totally pointless to me.  There are tons of sysctls that
don't have Kconfig options: what makes this one special?

> > Why have you made this option "default y", when enabling it clearly
> > makes user-visible changes to kernel behaviour?
> 
> Ingo specifically asked me to make it "default y".

But this is a brand new feature that changes longstanding behaviour of
various syscalls.  Making it default to enabled is rather mean to users
(since it will tend to get enabled by "oldconfig") and seems almost
guaranteed to cause regressions.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

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

[Kees Cook]

On Thu, Jan 5, 2012 at 12:08 PM, Nick Bowler <nbowler@elliptictech.com> wrote:
> On 2012-01-05 11:34 -0800, Kees Cook wrote:
>> On Thu, Jan 5, 2012 at 6:30 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
>> > On 2012-01-04 12:18 -0800, Kees Cook wrote:
>> >> diff --git a/fs/Kconfig b/fs/Kconfig
>> >> index 5f4c45d..26ede24 100644
>> >> --- a/fs/Kconfig
>> >> +++ b/fs/Kconfig
>> >> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
>> >>  source "fs/dlm/Kconfig"
>> >>
>> >>  endmenu
>> >> +
>> >> +config PROTECTED_STICKY_SYMLINKS
>> >> +     bool "Protect symlink following in sticky world-writable directories"
>> >> +     default y
>> > [...]
>> >
>> > Why do we need a config option for this?  What's wrong with just using
>> > the sysctl?
>>
>> This way the sysctl can configured directly without needing to have a
>> distro add a new item to sysctl.conf.
>
> This seems totally pointless to me.  There are tons of sysctls that
> don't have Kconfig options: what makes this one special?

Most are system tuning; this is directly related to vulnerability
mitigation. Besides, I like having CONFIGs for sysctls because then I
can build my kernel the way I want it without having to worry about
tweaking my userspace sysctl.conf file, or run newer kernels on older
userspaces, etc etc.

>> > Why have you made this option "default y", when enabling it clearly
>> > makes user-visible changes to kernel behaviour?
>>
>> Ingo specifically asked me to make it "default y".
>
> But this is a brand new feature that changes longstanding behaviour of
> various syscalls.  Making it default to enabled is rather mean to users
> (since it will tend to get enabled by "oldconfig") and seems almost
> guaranteed to cause regressions.

I couldn't disagree more. There has been zero evidence of this change
causing anything but regressions in _attacks_. :P If anything, I think
there should be no CONFIG and no sysctl, and it should be entirely
non-optional. But since this patch needs consensus, I have provided
knobs to control it. This is the way of security features. For
example, years back I added a knob for /proc/$pid/maps protection
being optional (and defaulted it to insecure because of people's fear
of regression), and eventually it changed to secure-by-default, and
then the knob went away completely because it didn't actually cause
problems.

-Kees

-- 
Kees Cook
ChromeOS Security

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

[Nick Bowler]

On 2012-01-05 12:55 -0800, Kees Cook wrote:
> On Thu, Jan 5, 2012 at 12:08 PM, Nick Bowler <nbowler@elliptictech.com> wrote:
> > On 2012-01-05 11:34 -0800, Kees Cook wrote:
> >> On Thu, Jan 5, 2012 at 6:30 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
> >> > On 2012-01-04 12:18 -0800, Kees Cook wrote:
> >> >> diff --git a/fs/Kconfig b/fs/Kconfig
> >> >> index 5f4c45d..26ede24 100644
> >> >> --- a/fs/Kconfig
> >> >> +++ b/fs/Kconfig
> >> >> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
> >> >>  source "fs/dlm/Kconfig"
> >> >>
> >> >>  endmenu
> >> >> +
> >> >> +config PROTECTED_STICKY_SYMLINKS
> >> >> +     bool "Protect symlink following in sticky world-writable directories"
> >> >> +     default y
> >> > [...]
> >> >
> >> > Why do we need a config option for this?  What's wrong with just using
> >> > the sysctl?
> >>
> >> This way the sysctl can configured directly without needing to have a
> >> distro add a new item to sysctl.conf.
> >
> > This seems totally pointless to me.  There are tons of sysctls that
> > don't have Kconfig options: what makes this one special?
> 
> Most are system tuning; this is directly related to vulnerability
> mitigation. Besides, I like having CONFIGs for sysctls because then I
> can build my kernel the way I want it without having to worry about
> tweaking my userspace sysctl.conf file, or run newer kernels on older
> userspaces, etc etc.

I agree that having kconfig knobs for sysctls may be convenient for some
users.  But every kconfig option we add requires the user to make a
decision before building their kernel.  In this case, this decision is
a waste of time because the option doesn't really affect the kernel in a
meaningful way: either choice can be easily changed from userspace after
booting.  A similar argument could be applied to almost any sysctl, and
we could add hundreds of new Kconfig options to control their default
values.  The result would be untenable.

Perhaps what we need instead is a way to set arbitrary sysctls from the
kernel command line.  This could easily be done by an initramfs, and not
require any changes to the kernel at all.

> >> > Why have you made this option "default y", when enabling it clearly
> >> > makes user-visible changes to kernel behaviour?
> >>
> >> Ingo specifically asked me to make it "default y".
> >
> > But this is a brand new feature that changes longstanding behaviour of
> > various syscalls.  Making it default to enabled is rather mean to users
> > (since it will tend to get enabled by "oldconfig") and seems almost
> > guaranteed to cause regressions.
> 
> I couldn't disagree more. There has been zero evidence of this change
> causing anything but regressions in _attacks_.

We have absolutely no idea what applications people are running that
will be affected by this change.  Of course there's no evidence of
breakage, because affected users (if any) have not had a single chance
to try this new feature out: it's not in the kernel yet.

> If anything, I think there should be no CONFIG and no sysctl, and it
> should be entirely non-optional. But since this patch needs consensus,
> I have provided knobs to control it. This is the way of security
> features. For example, years back I added a knob for /proc/$pid/maps
> protection being optional (and defaulted it to insecure because of
> people's fear of regression), and eventually it changed to
> secure-by-default, and then the knob went away completely because it
> didn't actually cause problems.

The process you describe above for /proc/$pid/maps is the right way to
change kernel behaviour while mitigating the risk of regressions.  With
this patch, you've skipped all those important steps!

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

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

[Kees Cook]

On Thu, Jan 5, 2012 at 2:18 PM, Nick Bowler <nbowler@elliptictech.com> wrote:
> On 2012-01-05 12:55 -0800, Kees Cook wrote:
>> On Thu, Jan 5, 2012 at 12:08 PM, Nick Bowler <nbowler@elliptictech.com> wrote:
>> > On 2012-01-05 11:34 -0800, Kees Cook wrote:
>> >> On Thu, Jan 5, 2012 at 6:30 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
>> >> > On 2012-01-04 12:18 -0800, Kees Cook wrote:
>> >> >> diff --git a/fs/Kconfig b/fs/Kconfig
>> >> >> index 5f4c45d..26ede24 100644
>> >> >> --- a/fs/Kconfig
>> >> >> +++ b/fs/Kconfig
>> >> >> @@ -278,3 +278,19 @@ source "fs/nls/Kconfig"
>> >> >>  source "fs/dlm/Kconfig"
>> >> >>
>> >> >>  endmenu
>> >> >> +
>> >> >> +config PROTECTED_STICKY_SYMLINKS
>> >> >> +     bool "Protect symlink following in sticky world-writable directories"
>> >> >> +     default y
>> >> > [...]
>> >> >
>> >> > Why do we need a config option for this?  What's wrong with just using
>> >> > the sysctl?
>> >>
>> >> This way the sysctl can configured directly without needing to have a
>> >> distro add a new item to sysctl.conf.
>> >
>> > This seems totally pointless to me.  There are tons of sysctls that
>> > don't have Kconfig options: what makes this one special?
>>
>> Most are system tuning; this is directly related to vulnerability
>> mitigation. Besides, I like having CONFIGs for sysctls because then I
>> can build my kernel the way I want it without having to worry about
>> tweaking my userspace sysctl.conf file, or run newer kernels on older
>> userspaces, etc etc.
>
> I agree that having kconfig knobs for sysctls may be convenient for some
> users.  But every kconfig option we add requires the user to make a
> decision before building their kernel.  In this case, this decision is
> a waste of time because the option doesn't really affect the kernel in a
> meaningful way: either choice can be easily changed from userspace after
> booting.  A similar argument could be applied to almost any sysctl, and
> we could add hundreds of new Kconfig options to control their default
> values.  The result would be untenable.
>
> Perhaps what we need instead is a way to set arbitrary sysctls from the
> kernel command line.  This could easily be done by an initramfs, and not
> require any changes to the kernel at all.

At present, I answer to Ingo and Al. I have no strong opinion on this
area of the patch. Ingo requested it be this way, so I'm leaving it.
:)

>> >> > Why have you made this option "default y", when enabling it clearly
>> >> > makes user-visible changes to kernel behaviour?
>> >>
>> >> Ingo specifically asked me to make it "default y".
>> >
>> > But this is a brand new feature that changes longstanding behaviour of
>> > various syscalls.  Making it default to enabled is rather mean to users
>> > (since it will tend to get enabled by "oldconfig") and seems almost
>> > guaranteed to cause regressions.
>>
>> I couldn't disagree more. There has been zero evidence of this change
>> causing anything but regressions in _attacks_.
>
> We have absolutely no idea what applications people are running that
> will be affected by this change.  Of course there's no evidence of
> breakage, because affected users (if any) have not had a single chance
> to try this new feature out: it's not in the kernel yet.

Ubuntu has been running with this restriction since Oct 2010. I've
seen 0 reports of this causing a regression. Openwall and grsecurity
have had this restriction for way longer without problem too.

>> If anything, I think there should be no CONFIG and no sysctl, and it
>> should be entirely non-optional. But since this patch needs consensus,
>> I have provided knobs to control it. This is the way of security
>> features. For example, years back I added a knob for /proc/$pid/maps
>> protection being optional (and defaulted it to insecure because of
>> people's fear of regression), and eventually it changed to
>> secure-by-default, and then the knob went away completely because it
>> didn't actually cause problems.
>
> The process you describe above for /proc/$pid/maps is the right way to
> change kernel behaviour while mitigating the risk of regressions.  With
> this patch, you've skipped all those important steps!

Like I said, I'm trying to keep the VFS maintainers happy. My original
patch had the default as 0 -- which was following my original
conservative approach.

-Kees

-- 
Kees Cook
ChromeOS Security

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

[Rik van Riel]

On 01/05/2012 03:55 PM, Kees Cook wrote:
> On Thu, Jan 5, 2012 at 12:08 PM, Nick Bowler<nbowler@elliptictech.com>  wrote:

>> But this is a brand new feature that changes longstanding behaviour of
>> various syscalls.  Making it default to enabled is rather mean to users
>> (since it will tend to get enabled by "oldconfig") and seems almost
>> guaranteed to cause regressions.
>
> I couldn't disagree more. There has been zero evidence of this change
> causing anything but regressions in _attacks_. :P If anything, I think
> there should be no CONFIG and no sysctl, and it should be entirely
> non-optional. But since this patch needs consensus, I have provided
> knobs to control it.

I agree with you, Kees.

The behaviour introduced by this patch should produce so
few issues, that the new behaviour should probably be on
by default.

The few people impacted can use a sysctl knob to disable
the check on their systems. I am not sure anyone will
actually be using that knob...

The config option is probably not needed. The new
behaviour is something that could just be on by default
for everyone, and disabled by the few (if any) people
who have some setup that breaks with it.

-- 
All rights reversed

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

[Ingo Molnar]

* Nick Bowler <nbowler@elliptictech.com> wrote:

> > >> +config PROTECTED_STICKY_SYMLINKS
> > >> +     bool "Protect symlink following in sticky world-writable directories"
> > >> +     default y
> > > [...]
> > >
> > > Why do we need a config option for this?  What's wrong 
> > > with just using the sysctl?
> > 
> > This way the sysctl can configured directly without needing 
> > to have a distro add a new item to sysctl.conf.
> 
> This seems totally pointless to me. [...]

It's how we add new features typically. From a distro's POV 
.config's are a lot more durable than system specific 
sysctl.conf's.

User can of course still override via the sysctl.conf, but the 
kernel (and the distro) wants to provide a sane default that 
does not depend on userspace settings.

Also, there are people who test new kernels but don't want to 
change the underlying distro. Twiddling such .config values is 
quite straightforward for them.

> [...]  There are tons of sysctls that don't have Kconfig 
> options: what makes this one special?

Those are old mistakes we want to forget about.

> > > Why have you made this option "default y", when enabling 
> > > it clearly makes user-visible changes to kernel behaviour?
> > 
> > Ingo specifically asked me to make it "default y".
> 
> But this is a brand new feature that changes longstanding 
> behaviour of various syscalls.  Making it default to enabled 
> is rather mean to users (since it will tend to get enabled by 
> "oldconfig") and seems almost guaranteed to cause regressions.

The changelog of the feature (which feature has been in use for 
years in various [admittedly smaller] distros) says that it does 
not break apps.

Worth a try: it will be very easy to flip it back if it causes a 
regression - but we'd like it to have at least *some* testing in 
the merge window to see whether there *is* some broken (or not 
so broken) app that relies on the current semantics.

Thanks,

	Ingo

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

[Ingo Molnar]

* Rik van Riel <riel@redhat.com> wrote:

> On 01/05/2012 03:55 PM, Kees Cook wrote:
> >On Thu, Jan 5, 2012 at 12:08 PM, Nick Bowler<nbowler@elliptictech.com>  wrote:
> 
> >>But this is a brand new feature that changes longstanding behaviour of
> >>various syscalls.  Making it default to enabled is rather mean to users
> >>(since it will tend to get enabled by "oldconfig") and seems almost
> >>guaranteed to cause regressions.
> >
> > I couldn't disagree more. There has been zero evidence of 
> > this change causing anything but regressions in _attacks_. 
> > :P If anything, I think there should be no CONFIG and no 
> > sysctl, and it should be entirely non-optional. But since 
> > this patch needs consensus, I have provided knobs to control 
> > it.
> 
> I agree with you, Kees.
> 
> The behaviour introduced by this patch should produce so few 
> issues, that the new behaviour should probably be on by 
> default.

Up to the point people report regressions.

And yes, I think Kees is perfectly right that the setting of the 
default should be evidence based. (Assuming Al and Linus is fine 
with the whole concept.)

The only specific counter-argument I can see is the spinlock 
performance impact I raised during review. I think we can (and 
should) live with that, and it's probably fixable, BYMMV.

Thanks,

	Ingo

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

[Ingo Molnar]

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

> On Thu, Jan 5, 2012 at 1:17 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> @@ -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,
> >> +     },
> >
> > Small detail:
> >
> > Might make sense to change the .mode to 0600, to make it 
> > harder for unprivileged attack code to guess whether this 
> > protection (and the resulting audit warning to the 
> > administrator) is enabled on a system or not.
> 
> Sure, I have no problem with that. In addition to this change, 
> what's the best next step for this patch?

Al and Linus's call I guess. Maybe ask Andrew whether he'd put 
it into -mm?

Thanks,

	Ingo

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

[Andrew Morton]

On Fri, 6 Jan 2012 08:36:35 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Kees Cook <keescook@chromium.org> wrote:
> 
> > On Thu, Jan 5, 2012 at 1:17 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > > * Kees Cook <keescook@chromium.org> wrote:
> > >
> > >> @@ -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,
> > >> +     },
> > >
> > > Small detail:
> > >
> > > Might make sense to change the .mode to 0600, to make it 
> > > harder for unprivileged attack code to guess whether this 
> > > protection (and the resulting audit warning to the 
> > > administrator) is enabled on a system or not.
> > 
> > Sure, I have no problem with that. In addition to this change, 
> > what's the best next step for this patch?
> 
> Al and Linus's call I guess. Maybe ask Andrew whether he'd put 
> it into -mm?

Sure, I can babysit it for Viro and get it a bit of testing meanwhile.

However, you've now made me go and look at it...


On Wed, 4 Jan 2012 12:18:00 -0800 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. 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.
> 
>
> ...
>
> +config PROTECTED_STICKY_SYMLINKS
> +	bool "Protect symlink following in sticky world-writable directories"
> +	default y
> +	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 be
> +	  followed only 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.

This is all quite misleading.  One would expect that
CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on or off
permanently.  ie, it controls whether the code is generated into
vmlinux in the usual fashion.  But it's not that at all - the user
gets the feature whether or not he wants it, and this variable only
sets the initial value.

Why are we forcing the user to have the feature if he doesn't want it,
btw?

And we appear to be enabling the feature if CONFIG_PROC_FS=n, which
might not be terribly useful?

> diff --git a/fs/namei.c b/fs/namei.c
> index 5008f01..b826d2e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -624,10 +624,80 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
>  	path_put(link);
>  }
>  
> +int protected_sticky_symlinks  read_mostly =

A nice convention is to prefix these globals with sysctl_.

	grep ".data.*&sysctl_" kernel/sysctl.c

has examples.

> +#ifdef CONFIG_PROTECTED_STICKY_SYMLINKS
> +				1;
> +#else
> +				0;
> +#endif

I have this niggling feeling that there's a trick to avoid this ugly,
and that Matt Mackall was involved.  But I think I misremember.  Maybe
the trick is to ensure (within Kconfig) that CONFIG_foo is always
defined, to either 0 or 1.  Still.  Ugly :)

> +/**
> + * 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 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)

Setting aside the issue that gcc will cheerily ignore the inline
directive, is it prudent to inline this?  Doing so will pork up the
follow_link() cache footprint.  Perhaps it should actually be noinline?

> +{
> +	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);
> +
> +#ifdef CONFIG_AUDIT
> +	if (error) {
> +		struct audit_buffer *ab;
> +
> +		ab = audit_log_start(current->audit_context,
> +				     GFP_KERNEL, AUDIT_AVC);
> +		audit_log_format(ab, "op=follow_link action=denied");
> +		audit_log_format(ab, " pid=%d comm=", current->pid);

The same pid can exist in multiple namespaces.  Now whatcha gonna do?

> +		audit_log_untrustedstring(ab, current->comm);
> +		audit_log_d_path(ab, "path=", &nameidata->path);
> +		audit_log_format(ab, " name=");
> +		audit_log_untrustedstring(ab, dentry->d_name.name);
> +		audit_log_format(ab, " dev=%s ino=%lu",
> +				 inode->i_sb->s_id,

Can s_id contain characters which audit_log_untrustedstring() would
have escaped?  I suspect so.

> +				 inode->i_ino);
> +		audit_log_end(ab);
> +	}
> +#endif
> +	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 +716,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 +1412,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);

follow_link() takes a bool, so this 0 should anally be "false". 
Multiple instances of this.

>  		if (!res)
>  			res = walk_component(nd, path, &nd->last,
>  					     nd->last_type, LOOKUP_FOLLOW);
> @@ -1612,7 +1685,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);
>
> ...
>

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

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > +config PROTECTED_STICKY_SYMLINKS
> > +	bool "Protect symlink following in sticky world-writable directories"
> > +	default y
> > +	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 be
> > +	  followed only 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.
> 
> This is all quite misleading.  One would expect that 
> CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on 
> or off permanently.  ie, it controls whether the code is 
> generated into vmlinux in the usual fashion.  But it's not 
> that at all - the user gets the feature whether or not he 
> wants it, and this variable only sets the initial value.
> 
> Why are we forcing the user to have the feature if he doesn't 
> want it, btw?

Basing on the (not yet fully confirmed) assertion that no apps 
are broken by this change but exploits, I'd argue that this is 
actually the sane and correct semantics for symlinks - i.e. we 
want this to be the default Linux behavior - not just a 
'feature'.

That way the configuration knobs are compat settings in essence 
- in case some app cares.

If people disagree and want it default off and as a separate 
feature then it has to be modularized out some more. There's 
notable silence from VFS folks on all this so Kees made an 
educated guess. It might be wrong.

> And we appear to be enabling the feature if CONFIG_PROC_FS=n, 
> which might not be terribly useful?

It can still be useful if it's default on - just cannot be 
turned off via /proc/sys/, right?

The combination that is not so useful is when it's off and 
there's !PROC_FS. If it's a compat feature then i wouldnt bother 
about it. If it's a separated out modular feature in a separate 
.c file then it can all be properly shaped via Kconfig 
dependencies.

Thanks,

	Ingo

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

[Andrew Morton]

On Fri, 6 Jan 2012 10:43:40 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > +config PROTECTED_STICKY_SYMLINKS
> > > +	bool "Protect symlink following in sticky world-writable directories"
> > > +	default y
> > > +	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 be
> > > +	  followed only 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.
> > 
> > This is all quite misleading.  One would expect that 
> > CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on 
> > or off permanently.  ie, it controls whether the code is 
> > generated into vmlinux in the usual fashion.  But it's not 
> > that at all - the user gets the feature whether or not he 
> > wants it, and this variable only sets the initial value.
> > 
> > Why are we forcing the user to have the feature if he doesn't 
> > want it, btw?
> 
> Basing on the (not yet fully confirmed) assertion that no apps 
> are broken by this change but exploits, I'd argue that this is 
> actually the sane and correct semantics for symlinks - i.e. we 
> want this to be the default Linux behavior - not just a 
> 'feature'.
> 
> That way the configuration knobs are compat settings in essence 
> - in case some app cares.
> 
> If people disagree and want it default off and as a separate 
> feature then it has to be modularized out some more. There's 
> notable silence from VFS folks on all this so Kees made an 
> educated guess. It might be wrong.

Maybe true for a general purpose computer, but someone who is making a
single-purpose device such as a digital TV or a wifi router won't want
it.

> > And we appear to be enabling the feature if CONFIG_PROC_FS=n, 
> > which might not be terribly useful?
> 
> It can still be useful if it's default on - just cannot be 
> turned off via /proc/sys/, right?
> 
> The combination that is not so useful is when it's off and 
> there's !PROC_FS. If it's a compat feature then i wouldnt bother 
> about it. If it's a separated out modular feature in a separate 
> .c file then it can all be properly shaped via Kconfig 
> dependencies.

Spose so.

I'd have thought the way to configure this feature would be to have
CONFIG_PROTECTED_STICKY_SYMLINKS to control the code generation then a
0 or 1 CONFIG_PROTECTED_STICKY_SYMLINKS_ENABLED to control the initial
setting.

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

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> Maybe true for a general purpose computer, but someone who is 
> making a single-purpose device such as a digital TV or a wifi 
> router won't want it.

That's the case for 99% of the features and semantics we have: 
by definition a single-purpose device uses only a small sub-set 
of an infinite purpose OS, right?

Still we only modularize semantics out if they easily fit into 
some existing plug-in/module concept, if the feature is arguably 
oddball that a sizable portion of people want to disable, or if 
it makes notable sense for size reasons. To me it looked 
distinctly silly to complicate things for such a small piece of 
code.

I doubt Kees would mind modularizing it, but it would be nice to 
get VFS maintainer feedback in the:

   { 'you are crazy, over my dead body' ... 'cool, merge it' }

continuous spectrum of possible answers.

Thanks,

	Ingo

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

[Andrew Morton]

On Fri, 6 Jan 2012 11:05:20 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > Maybe true for a general purpose computer, but someone who is 
> > making a single-purpose device such as a digital TV or a wifi 
> > router won't want it.
> 
> That's the case for 99% of the features and semantics we have: 
> by definition a single-purpose device uses only a small sub-set 
> of an infinite purpose OS, right?
> 
> Still we only modularize semantics out if they easily fit into 
> some existing plug-in/module concept, if the feature is arguably 
> oddball that a sizable portion of people want to disable, or if 
> it makes notable sense for size reasons. To me it looked 
> distinctly silly to complicate things for such a small piece of 
> code.

We're talking tens or hundreds of millions of machines for which the
patch is a straightforward speed and space regression.  Fixing this
needs just a little Kconfig twiddling and a #else clause.  We may as
well do it.

> I doubt Kees would mind modularizing it, but it would be nice to 
> get VFS maintainer feedback in the:
> 
>    { 'you are crazy, over my dead body' ... 'cool, merge it' }
> 
> continuous spectrum of possible answers.

Well yes.  We'll get there.

Alas, I've become rather slack in my maintainer patchbombing in the
past year or two.  It's just boring and depressing to spray patches at
maintainers and have 90% or more of them simply ignored.  I'm sitting
on 45 such patches at present so I suppose I should get off my tail and
do a respray.

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

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 6 Jan 2012 11:05:20 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > Maybe true for a general purpose computer, but someone who 
> > > is making a single-purpose device such as a digital TV or 
> > > a wifi router won't want it.
> > 
> > That's the case for 99% of the features and semantics we 
> > have: by definition a single-purpose device uses only a 
> > small sub-set of an infinite purpose OS, right?
> > 
> > Still we only modularize semantics out if they easily fit 
> > into some existing plug-in/module concept, if the feature is 
> > arguably oddball that a sizable portion of people want to 
> > disable, or if it makes notable sense for size reasons. To 
> > me it looked distinctly silly to complicate things for such 
> > a small piece of code.
> 
> We're talking tens or hundreds of millions of machines for 
> which the patch is a straightforward speed and space 
> regression.  Fixing this needs just a little Kconfig twiddling 
> and a #else clause.  We may as well do it.

No strong objections from me.

> > I doubt Kees would mind modularizing it, but it would be nice to 
> > get VFS maintainer feedback in the:
> > 
> >    { 'you are crazy, over my dead body' ... 'cool, merge it' }
> > 
> > continuous spectrum of possible answers.
> 
> Well yes.  We'll get there.
> 
> Alas, I've become rather slack in my maintainer patchbombing 
> in the past year or two.  It's just boring and depressing to 
> spray patches at maintainers and have 90% or more of them 
> simply ignored. [...]

How about just sending it to Linus after the first ignored patch 
[perhaps marked in a special way, to make Linus aware of the 
out-of-band nature of the patches], instead of buffering them 
indefinitely and increasing your overhead all around?

We'll no doubt regret some of those patches going upstream, but 
that's OK i think, this is an exception mechanism.

> [...] I'm sitting on 45 such patches at present so I suppose I 
> should get off my tail and do a respray.

(If I missed any then let me know.)

Thanks,

	Ingo

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

[Kees Cook]

On Fri, Jan 6, 2012 at 1:58 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 6 Jan 2012 10:43:40 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>> * Andrew Morton <akpm@linux-foundation.org> wrote:
>> > > +config PROTECTED_STICKY_SYMLINKS
>> > > + bool "Protect symlink following in sticky world-writable directories"
>> > > + default y
>> > > + 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 be
>> > > +   followed only 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.
>> >
>> > This is all quite misleading.  One would expect that
>> > CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on
>> > or off permanently.  ie, it controls whether the code is
>> > generated into vmlinux in the usual fashion.  But it's not
>> > that at all - the user gets the feature whether or not he
>> > wants it, and this variable only sets the initial value.
>> >
>> > Why are we forcing the user to have the feature if he doesn't
>> > want it, btw?
>>
>> Basing on the (not yet fully confirmed) assertion that no apps
>> are broken by this change but exploits, I'd argue that this is
>> actually the sane and correct semantics for symlinks - i.e. we
>> want this to be the default Linux behavior - not just a
>> 'feature'.
>>
>> That way the configuration knobs are compat settings in essence
>> - in case some app cares.
>>
>> If people disagree and want it default off and as a separate
>> feature then it has to be modularized out some more. There's
>> notable silence from VFS folks on all this so Kees made an
>> educated guess. It might be wrong.
>
> Maybe true for a general purpose computer, but someone who is making a
> single-purpose device such as a digital TV or a wifi router won't want
> it.
> [...]
> I'd have thought the way to configure this feature would be to have
> CONFIG_PROTECTED_STICKY_SYMLINKS to control the code generation then a
> 0 or 1 CONFIG_PROTECTED_STICKY_SYMLINKS_ENABLED to control the initial
> setting.

This seems like probably the best approach, though I dislike the
silliness required in Kconfig to get a boolean into 1/0 form instead
of set/unset in way that doesn't require the user to type "1" or "0".
I'm happy to do it, though.

-Kees

-- 
Kees Cook
ChromeOS Security

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

[Kees Cook]

On Fri, Jan 6, 2012 at 1:21 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 6 Jan 2012 08:36:35 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>> * Kees Cook <keescook@chromium.org> wrote:
>> > On Thu, Jan 5, 2012 at 1:17 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> > > * Kees Cook <keescook@chromium.org> wrote:
>> > >
>> > >> @@ -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,
>> > >> +     },
>> > >
>> > > Small detail:
>> > >
>> > > Might make sense to change the .mode to 0600, to make it
>> > > harder for unprivileged attack code to guess whether this
>> > > protection (and the resulting audit warning to the
>> > > administrator) is enabled on a system or not.
>> >
>> > Sure, I have no problem with that. In addition to this change,
>> > what's the best next step for this patch?
>>
>> Al and Linus's call I guess. Maybe ask Andrew whether he'd put
>> it into -mm?
>
> Sure, I can babysit it for Viro and get it a bit of testing meanwhile.

That would be wonderful. :)

> However, you've now made me go and look at it...
>
> On Wed, 4 Jan 2012 12:18:00 -0800 Kees Cook <keescook@chromium.org> wrote:
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 5008f01..b826d2e 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -624,10 +624,80 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
>>       path_put(link);
>>  }
>>
>> +int protected_sticky_symlinks  read_mostly =
>
> A nice convention is to prefix these globals with sysctl_.
>
>        grep ".data.*&sysctl_" kernel/sysctl.c
>
> has examples.

Excellent point, thanks. I will fix this.

>> +/**
>> + * 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 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)
>
> Setting aside the issue that gcc will cheerily ignore the inline
> directive, is it prudent to inline this?  Doing so will pork up the
> follow_link() cache footprint.  Perhaps it should actually be noinline?

Well, I was trying to balance a few potential issues. One thing to try
to avoid was slowing down symlink resolution. The first step was only
doing this check in the sensitive path, as Al had pointed out to me.
Since the code is relatively small, it seemed reasonable to inline it
where it was needed. If anyone has a strong opinion about this, it's
trivial for it to be changed.

>> +             ab = audit_log_start(current->audit_context,
>> +                                  GFP_KERNEL, AUDIT_AVC);
>> +             audit_log_format(ab, "op=follow_link action=denied");
>> +             audit_log_format(ab, " pid=%d comm=", current->pid);
>
> The same pid can exist in multiple namespaces.  Now whatcha gonna do?

Well, that's a wider problem that just this code. This style of report
is used in several other places. I'm open to suggestions.

>> +             audit_log_untrustedstring(ab, current->comm);
>> +             audit_log_d_path(ab, "path=", &nameidata->path);
>> +             audit_log_format(ab, " name=");
>> +             audit_log_untrustedstring(ab, dentry->d_name.name);
>> +             audit_log_format(ab, " dev=%s ino=%lu",
>> +                              inode->i_sb->s_id,
>
> Can s_id contain characters which audit_log_untrustedstring() would
> have escaped?  I suspect so.

Hunh. Interesting point. I will adjust this here and in the place I
originally saw it.

>> +                              inode->i_ino);
>> +             audit_log_end(ab);
>> +     }
>> +#endif
>> +     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 +716,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 +1412,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);
>
> follow_link() takes a bool, so this 0 should anally be "false".
> Multiple instances of this.

Ah yes, thanks. I will fix this.

Thanks for the review! I will post an update shortly.

-Kees

-- 
Kees Cook
ChromeOS Security