FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 05-11-2010, 11:51 PM
Kees Cook
 
Default UBUNTU: SAUCE: fs: block 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 following a given symlink (i.e. a
root process follows a symlink belonging to another user).

The solution is to not permit symlinks to be followed when users do not
match, but only in a world-writable sticky directory (with an additional
improvement that the directory owner's symlinks can always be followed,
regardless who is following them).

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. Also, please reference
where POSIX says this.
- 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 grsecurity, which is similar to the
patch in Openwall. I have added a sysctl to toggle the behavior back
to the old handling via /proc/sys/fs/weak-sticky-symlinks, as well as
a ratelimited deprecation warning.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
include/linux/security.h | 1 +
kernel/sysctl.c | 8 ++++++++
security/capability.c | 6 ------
security/commoncap.c | 23 +++++++++++++++++++++++
4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 3158dd9..92eca95 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -67,6 +67,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
extern int cap_inode_need_killpriv(struct dentry *dentry);
extern int cap_inode_killpriv(struct dentry *dentry);
+extern int cap_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
extern int cap_file_mmap(struct file *file, unsigned long reqprot,
unsigned long prot, unsigned long flags,
unsigned long addr, unsigned long addr_only);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8686b0f..36a104c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -86,6 +86,7 @@ extern int sysctl_oom_dump_tasks;
extern int max_threads;
extern int core_uses_pid;
extern int suid_dumpable;
+extern int weak_sticky_symlinks;
extern char core_pattern[];
extern unsigned int core_pipe_limit;
extern int pid_max;
@@ -1416,6 +1417,13 @@ static struct ctl_table fs_table[] = {
.extra1 = &zero,
.extra2 = &two,
},
+ {
+ .procname = "weak-sticky-symlinks",
+ .data = &weak_sticky_symlinks,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = &proc_dointvec,
+ },
#if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
{
.procname = "binfmt_misc",
diff --git a/security/capability.c b/security/capability.c
index 4875142..d4633f3 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -200,12 +200,6 @@ static int cap_inode_readlink(struct dentry *dentry)
return 0;
}

-static int cap_inode_follow_link(struct dentry *dentry,
- struct nameidata *nameidata)
-{
- return 0;
-}
-
static int cap_inode_permission(struct inode *inode, int mask)
{
return 0;
diff --git a/security/commoncap.c b/security/commoncap.c
index 6166973..83d5a18 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -29,6 +29,9 @@
#include <linux/securebits.h>
#include <linux/syslog.h>

+/* sysctl for symlink permissions checking */
+int weak_sticky_symlinks;
+
/*
* If a non-root user executes a setuid-root binary in
* !secure(SECURE_NOROOT) mode, then we raise capabilities.
@@ -281,6 +284,27 @@ int cap_inode_killpriv(struct dentry *dentry)
return inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
}

+int cap_inode_follow_link(struct dentry *dentry,
+ struct nameidata *nameidata)
+{
+ const struct inode *parent = dentry->d_parent->d_inode;
+ const struct inode *inode = dentry->d_inode;
+ const struct cred *cred = current_cred();
+
+ if (weak_sticky_symlinks)
+ return 0;
+
+ if (S_ISLNK(inode->i_mode) && (parent->i_mode & S_ISVTX) &&
+ (parent->i_mode & S_IWOTH) && (parent->i_uid != inode->i_uid) &&
+ (cred->fsuid != inode->i_uid)) {
+ printk_ratelimited(KERN_INFO "deprecated sticky-directory"
+ " non-matching uid symlink following was attempted"
+ " by: %s
", current->comm);
+ return -EACCES;
+ }
+ return 0;
+}
+
/*
* Calculate the new process capability sets from the capability sets attached
* to a file.
--
1.7.0.4


--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-21-2010, 01:43 PM
Tim Gardner
 
Default UBUNTU: SAUCE: fs: block cross-uid sticky symlinks

On 05/11/2010 05:51 PM, 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).
>
> The solution is to not permit symlinks to be followed when users do not
> match, but only in a world-writable sticky directory (with an additional
> improvement that the directory owner's symlinks can always be followed,
> regardless who is following them).
>
> 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. Also, please reference
> where POSIX says this.
> - 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 grsecurity, which is similar to the
> patch in Openwall. I have added a sysctl to toggle the behavior back
> to the old handling via /proc/sys/fs/weak-sticky-symlinks, as well as
> a ratelimited deprecation warning.
>
> Signed-off-by: Kees Cook<kees.cook@canonical.com>
> ---
> include/linux/security.h | 1 +
> kernel/sysctl.c | 8 ++++++++
> security/capability.c | 6 ------
> security/commoncap.c | 23 +++++++++++++++++++++++
> 4 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3158dd9..92eca95 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -67,6 +67,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
> extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
> extern int cap_inode_need_killpriv(struct dentry *dentry);
> extern int cap_inode_killpriv(struct dentry *dentry);
> +extern int cap_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
> extern int cap_file_mmap(struct file *file, unsigned long reqprot,
> unsigned long prot, unsigned long flags,
> unsigned long addr, unsigned long addr_only);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8686b0f..36a104c 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -86,6 +86,7 @@ extern int sysctl_oom_dump_tasks;
> extern int max_threads;
> extern int core_uses_pid;
> extern int suid_dumpable;
> +extern int weak_sticky_symlinks;
> extern char core_pattern[];
> extern unsigned int core_pipe_limit;
> extern int pid_max;
> @@ -1416,6 +1417,13 @@ static struct ctl_table fs_table[] = {
> .extra1 =&zero,
> .extra2 =&two,
> },
> + {
> + .procname = "weak-sticky-symlinks",
> + .data =&weak_sticky_symlinks,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler =&proc_dointvec,
> + },
> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
> {
> .procname = "binfmt_misc",
> diff --git a/security/capability.c b/security/capability.c
> index 4875142..d4633f3 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -200,12 +200,6 @@ static int cap_inode_readlink(struct dentry *dentry)
> return 0;
> }
>
> -static int cap_inode_follow_link(struct dentry *dentry,
> - struct nameidata *nameidata)
> -{
> - return 0;
> -}
> -
> static int cap_inode_permission(struct inode *inode, int mask)
> {
> return 0;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 6166973..83d5a18 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -29,6 +29,9 @@
> #include<linux/securebits.h>
> #include<linux/syslog.h>
>
> +/* sysctl for symlink permissions checking */
> +int weak_sticky_symlinks;
> +
> /*
> * If a non-root user executes a setuid-root binary in
> * !secure(SECURE_NOROOT) mode, then we raise capabilities.
> @@ -281,6 +284,27 @@ int cap_inode_killpriv(struct dentry *dentry)
> return inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
> }
>
> +int cap_inode_follow_link(struct dentry *dentry,
> + struct nameidata *nameidata)
> +{
> + const struct inode *parent = dentry->d_parent->d_inode;
> + const struct inode *inode = dentry->d_inode;
> + const struct cred *cred = current_cred();
> +
> + if (weak_sticky_symlinks)
> + return 0;
> +
> + if (S_ISLNK(inode->i_mode)&& (parent->i_mode& S_ISVTX)&&
> + (parent->i_mode& S_IWOTH)&& (parent->i_uid != inode->i_uid)&&
> + (cred->fsuid != inode->i_uid)) {
> + printk_ratelimited(KERN_INFO "deprecated sticky-directory"
> + " non-matching uid symlink following was attempted"
> + " by: %s
", current->comm);
> + return -EACCES;
> + }
> + return 0;
> +}
> +
> /*
> * Calculate the new process capability sets from the capability sets attached
> * to a file.

Are you proposing this for Lucid?

The code looks fine, but I'm not familiar enough with file system
semantics to comment on cap_inode_follow_link(). However, its an easily
tested patch.

Acked-by: Tim Gardner <tim.gardner@canonical.com>

--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-21-2010, 05:05 PM
Leann Ogasawara
 
Default UBUNTU: SAUCE: fs: block cross-uid sticky symlinks

Applied to Maverick master via Tim's pull request [1].

Thanks,
Leann

[1] https://lists.ubuntu.com/archives/kernel-team/2010-May/010608.html



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-21-2010, 05:10 PM
Kees Cook
 
Default UBUNTU: SAUCE: fs: block cross-uid sticky symlinks

Hi Tim,

On Fri, May 21, 2010 at 07:43:48AM -0600, Tim Gardner wrote:
> Are you proposing this for Lucid?

Not presently. I was intending this for maverick only, but if there are no
problems, I may consider asking for an SRU, but that will be some time from
now.

> The code looks fine, but I'm not familiar enough with file system
> semantics to comment on cap_inode_follow_link().

If this turns out to be the wrong place, I can easily move it into the
callers of cap_inode_follow_link(), but since there were multiple callers,
using this location seemed the most efficient.

> However, its an easily tested patch.

To that end, I have a test script for this here:
http://bazaar.launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/annotate/head:/scripts/test-kernel-hardening.py

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 05-27-2010, 08:38 AM
Stefan Bader
 
Default UBUNTU: SAUCE: fs: block cross-uid sticky symlinks

This one looks as far as I can tell sensible. If my understanding of the
cap_follow_symlink is right this seems to be the right place for it. The impact
also seems acceptable and side-effects rather unlikely.
For SRU it probably should be in Maverick for a while and then we need a bug
stating it as a security issue.

On 05/12/2010 01:51 AM, 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).
>
> The solution is to not permit symlinks to be followed when users do not
> match, but only in a world-writable sticky directory (with an additional
> improvement that the directory owner's symlinks can always be followed,
> regardless who is following them).
>
> 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. Also, please reference
> where POSIX says this.
> - 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 grsecurity, which is similar to the
> patch in Openwall. I have added a sysctl to toggle the behavior back
> to the old handling via /proc/sys/fs/weak-sticky-symlinks, as well as
> a ratelimited deprecation warning.
>
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> include/linux/security.h | 1 +
> kernel/sysctl.c | 8 ++++++++
> security/capability.c | 6 ------
> security/commoncap.c | 23 +++++++++++++++++++++++
> 4 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3158dd9..92eca95 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -67,6 +67,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
> extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
> extern int cap_inode_need_killpriv(struct dentry *dentry);
> extern int cap_inode_killpriv(struct dentry *dentry);
> +extern int cap_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
> extern int cap_file_mmap(struct file *file, unsigned long reqprot,
> unsigned long prot, unsigned long flags,
> unsigned long addr, unsigned long addr_only);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8686b0f..36a104c 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -86,6 +86,7 @@ extern int sysctl_oom_dump_tasks;
> extern int max_threads;
> extern int core_uses_pid;
> extern int suid_dumpable;
> +extern int weak_sticky_symlinks;
> extern char core_pattern[];
> extern unsigned int core_pipe_limit;
> extern int pid_max;
> @@ -1416,6 +1417,13 @@ static struct ctl_table fs_table[] = {
> .extra1 = &zero,
> .extra2 = &two,
> },
> + {
> + .procname = "weak-sticky-symlinks",
> + .data = &weak_sticky_symlinks,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = &proc_dointvec,
> + },
> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
> {
> .procname = "binfmt_misc",
> diff --git a/security/capability.c b/security/capability.c
> index 4875142..d4633f3 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -200,12 +200,6 @@ static int cap_inode_readlink(struct dentry *dentry)
> return 0;
> }
>
> -static int cap_inode_follow_link(struct dentry *dentry,
> - struct nameidata *nameidata)
> -{
> - return 0;
> -}
> -
> static int cap_inode_permission(struct inode *inode, int mask)
> {
> return 0;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 6166973..83d5a18 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -29,6 +29,9 @@
> #include <linux/securebits.h>
> #include <linux/syslog.h>
>
> +/* sysctl for symlink permissions checking */
> +int weak_sticky_symlinks;
> +
> /*
> * If a non-root user executes a setuid-root binary in
> * !secure(SECURE_NOROOT) mode, then we raise capabilities.
> @@ -281,6 +284,27 @@ int cap_inode_killpriv(struct dentry *dentry)
> return inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
> }
>
> +int cap_inode_follow_link(struct dentry *dentry,
> + struct nameidata *nameidata)
> +{
> + const struct inode *parent = dentry->d_parent->d_inode;
> + const struct inode *inode = dentry->d_inode;
> + const struct cred *cred = current_cred();
> +
> + if (weak_sticky_symlinks)
> + return 0;
> +
> + if (S_ISLNK(inode->i_mode) && (parent->i_mode & S_ISVTX) &&
> + (parent->i_mode & S_IWOTH) && (parent->i_uid != inode->i_uid) &&
> + (cred->fsuid != inode->i_uid)) {
> + printk_ratelimited(KERN_INFO "deprecated sticky-directory"
> + " non-matching uid symlink following was attempted"
> + " by: %s
", current->comm);
> + return -EACCES;
> + }
> + return 0;
> +}
> +
> /*
> * Calculate the new process capability sets from the capability sets attached
> * to a file.



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 12:18 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org