| Message ID | 20100511235124.GF4433@outflux.net |
|---|---|
| State | Accepted |
| Delegated to: | Leann Ogasawara |
| Headers | show |
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\n", 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>
Applied to Maverick master via Tim's pull request [1]. Thanks, Leann [1] https://lists.ubuntu.com/archives/kernel-team/2010-May/010608.html
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
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\n", current->comm); > + return -EACCES; > + } > + return 0; > +} > + > /* > * Calculate the new process capability sets from the capability sets attached > * to a file.
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\n", current->comm); + return -EACCES; + } + return 0; +} + /* * Calculate the new process capability sets from the capability sets attached * to a file.
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(-)