| Message ID | 20100512160308.GK4433@outflux.net |
|---|---|
| State | Accepted |
| Delegated to: | Leann Ogasawara |
| Headers | show |
On 05/12/2010 10:03 AM, Kees Cook wrote: > Hardlinks can be abused in a similar fashion to symlinks above, but they > are not limited to world-writable directories. If /etc and /home are on > the same partition, a regular user can create a hardlink to /etc/shadow in > their home directory. While it retains the original owner and permissions, > it is possible for privileged programs that are otherwise symlink-safe > to mistakenly access the file through its hardlink. Additionally, a very > minor untraceable quota-bypassing local denial of service is possible by > an attacker exhausting disk space by filling a world-writable directory > with hardlinks. > > The solution is to not allow the creation of hardlinks to files that a > given user would be unable to read or write originally, or are otherwise > sensitive. > > Some links to the history of its discussion: > > 1997 Dec, Yuri Kuzmenko http://lkml.org/lkml/1997/12/29/20 > 2002 Apr, Chris Wright http://lkml.org/lkml/2002/4/13/99 > > 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 atd, courier, and other unknown applications that use this > feature. > - These applications are easy to spot and can be tested and > fixed. Applications that are vulnerable to hardlink attacks by not > having the change aren't. > - Applications should correctly drop privileges before attempting to > access user files. > - 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-nonaccess-hardlinks, as well as > a ratelimited deprecation warning. > > Signed-off-by: Kees Cook<kees.cook@canonical.com> > --- > include/linux/security.h | 4 ++++ > kernel/sysctl.c | 10 ++++++++++ > security/apparmor/lsm.c | 5 ++++- > security/capability.c | 6 ------ > security/commoncap.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 62 insertions(+), 7 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index 92eca95..6c1a6bf 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -79,6 +79,10 @@ extern int cap_task_setioprio(struct task_struct *p, int ioprio); > extern int cap_task_setnice(struct task_struct *p, int nice); > extern int cap_syslog(int type, bool from_file); > extern int cap_vm_enough_memory(struct mm_struct *mm, long pages); > +#ifdef CONFIG_SECURITY_PATH > +extern int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > + struct dentry *new_dentry); > +#endif > > struct msghdr; > struct sk_buff; > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 36a104c..4f3ffd0 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -87,6 +87,7 @@ extern int max_threads; > extern int core_uses_pid; > extern int suid_dumpable; > extern int weak_sticky_symlinks; > +extern int weak_nonaccess_hardlinks; > extern char core_pattern[]; > extern unsigned int core_pipe_limit; > extern int pid_max; > @@ -1424,6 +1425,15 @@ static struct ctl_table fs_table[] = { > .mode = 0644, > .proc_handler =&proc_dointvec, > }, > +#ifdef CONFIG_SECURITY_PATH > + { > + .procname = "weak-nonaccess-hardlinks", > + .data =&weak_nonaccess_hardlinks, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler =&proc_dointvec, > + }, > +#endif > #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE) > { > .procname = "binfmt_misc", > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 814b086..82e222d 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -317,7 +317,10 @@ static int apparmor_path_link(struct dentry *old_dentry, struct path *new_dir, > struct dentry *new_dentry) > { > struct aa_profile *profile; > - int error = 0; > + int error = 0, rc; > + > + if ( (rc = cap_path_link(old_dentry, new_dir, new_dentry)) ) > + return rc; > > if (!mediated_filesystem(old_dentry->d_inode)) > return 0; > diff --git a/security/capability.c b/security/capability.c > index d4633f3..75eb6b0 100644 > --- a/security/capability.c > +++ b/security/capability.c > @@ -285,12 +285,6 @@ static int cap_path_symlink(struct path *dir, struct dentry *dentry, > return 0; > } > > -static int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > - struct dentry *new_dentry) > -{ > - return 0; > -} > - > static int cap_path_rename(struct path *old_path, struct dentry *old_dentry, > struct path *new_path, struct dentry *new_dentry) > { > diff --git a/security/commoncap.c b/security/commoncap.c > index 83d5a18..133ca08 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -31,6 +31,8 @@ > > /* sysctl for symlink permissions checking */ > int weak_sticky_symlinks; > +/* sysctl for hardlink permissions checking */ > +int weak_nonaccess_hardlinks; > > /* > * If a non-root user executes a setuid-root binary in > @@ -304,6 +306,48 @@ int cap_inode_follow_link(struct dentry *dentry, > return 0; > } > > +#ifdef CONFIG_SECURITY_PATH > +/* > + * cap_path_link - verify that hardlinking is allowed > + * @old_dentry: the source inode/dentry to hardlink from > + * @new_dir: target directory > + * @new_dentry: the target inode/dentry to hardlink to > + * > + * Block hardlink when all of: > + * - fsuid does not match inode > + * - not CAP_FOWNER > + * - and at least one of: > + * - inode is not a regular file > + * - inode is setuid > + * - inode is setgid and group-exec > + * - access failure for read or write > + * > + * Returns 0 if successful, -ve on error. > + */ > +int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > + struct dentry *new_dentry) > +{ > + struct inode *inode = old_dentry->d_inode; > + const int mode = inode->i_mode; > + const struct cred *cred = current_cred(); > + > + if (weak_nonaccess_hardlinks) return 0; > + > + if (cred->fsuid != inode->i_uid&& > + (!S_ISREG(mode) || (mode& S_ISUID) || > + ((mode& (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) || > + (generic_permission(inode, MAY_READ | MAY_WRITE, NULL)))&& > + !capable(CAP_FOWNER)) { > + printk_ratelimited(KERN_INFO "deprecated non-accessible" > + " hardlink creation was attempted by: %s\n", > + current->comm); > + return -EPERM; > + } > + > + return 0; > +} > +#endif /* CONFIG_SECURITY_PATH */ > + > /* > * Calculate the new process capability sets from the capability sets attached > * to a file. Again, very similar to soft links and easily testable. 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
It is similar to the softlinks, though maybe a little harder to see the results it causes. The patch itself seems to be ok. For Lucid SRU I would definitely want to see effects on applications. In the view of SRU changes not being allowed to cause regressions, would it be an option to make the default weak_nonaccess_hardlinks=1 for the Lucid version and therefore rather allow an opt-in? -Stefan On 05/12/2010 06:03 PM, Kees Cook wrote: > Hardlinks can be abused in a similar fashion to symlinks above, but they > are not limited to world-writable directories. If /etc and /home are on > the same partition, a regular user can create a hardlink to /etc/shadow in > their home directory. While it retains the original owner and permissions, > it is possible for privileged programs that are otherwise symlink-safe > to mistakenly access the file through its hardlink. Additionally, a very > minor untraceable quota-bypassing local denial of service is possible by > an attacker exhausting disk space by filling a world-writable directory > with hardlinks. > > The solution is to not allow the creation of hardlinks to files that a > given user would be unable to read or write originally, or are otherwise > sensitive. > > Some links to the history of its discussion: > > 1997 Dec, Yuri Kuzmenko http://lkml.org/lkml/1997/12/29/20 > 2002 Apr, Chris Wright http://lkml.org/lkml/2002/4/13/99 > > 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 atd, courier, and other unknown applications that use this > feature. > - These applications are easy to spot and can be tested and > fixed. Applications that are vulnerable to hardlink attacks by not > having the change aren't. > - Applications should correctly drop privileges before attempting to > access user files. > - 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-nonaccess-hardlinks, as well as > a ratelimited deprecation warning. > > Signed-off-by: Kees Cook <kees.cook@canonical.com> > --- > include/linux/security.h | 4 ++++ > kernel/sysctl.c | 10 ++++++++++ > security/apparmor/lsm.c | 5 ++++- > security/capability.c | 6 ------ > security/commoncap.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 62 insertions(+), 7 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index 92eca95..6c1a6bf 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -79,6 +79,10 @@ extern int cap_task_setioprio(struct task_struct *p, int ioprio); > extern int cap_task_setnice(struct task_struct *p, int nice); > extern int cap_syslog(int type, bool from_file); > extern int cap_vm_enough_memory(struct mm_struct *mm, long pages); > +#ifdef CONFIG_SECURITY_PATH > +extern int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > + struct dentry *new_dentry); > +#endif > > struct msghdr; > struct sk_buff; > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 36a104c..4f3ffd0 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -87,6 +87,7 @@ extern int max_threads; > extern int core_uses_pid; > extern int suid_dumpable; > extern int weak_sticky_symlinks; > +extern int weak_nonaccess_hardlinks; > extern char core_pattern[]; > extern unsigned int core_pipe_limit; > extern int pid_max; > @@ -1424,6 +1425,15 @@ static struct ctl_table fs_table[] = { > .mode = 0644, > .proc_handler = &proc_dointvec, > }, > +#ifdef CONFIG_SECURITY_PATH > + { > + .procname = "weak-nonaccess-hardlinks", > + .data = &weak_nonaccess_hardlinks, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = &proc_dointvec, > + }, > +#endif > #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE) > { > .procname = "binfmt_misc", > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 814b086..82e222d 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -317,7 +317,10 @@ static int apparmor_path_link(struct dentry *old_dentry, struct path *new_dir, > struct dentry *new_dentry) > { > struct aa_profile *profile; > - int error = 0; > + int error = 0, rc; > + > + if ( (rc = cap_path_link(old_dentry, new_dir, new_dentry)) ) > + return rc; > > if (!mediated_filesystem(old_dentry->d_inode)) > return 0; > diff --git a/security/capability.c b/security/capability.c > index d4633f3..75eb6b0 100644 > --- a/security/capability.c > +++ b/security/capability.c > @@ -285,12 +285,6 @@ static int cap_path_symlink(struct path *dir, struct dentry *dentry, > return 0; > } > > -static int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > - struct dentry *new_dentry) > -{ > - return 0; > -} > - > static int cap_path_rename(struct path *old_path, struct dentry *old_dentry, > struct path *new_path, struct dentry *new_dentry) > { > diff --git a/security/commoncap.c b/security/commoncap.c > index 83d5a18..133ca08 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -31,6 +31,8 @@ > > /* sysctl for symlink permissions checking */ > int weak_sticky_symlinks; > +/* sysctl for hardlink permissions checking */ > +int weak_nonaccess_hardlinks; > > /* > * If a non-root user executes a setuid-root binary in > @@ -304,6 +306,48 @@ int cap_inode_follow_link(struct dentry *dentry, > return 0; > } > > +#ifdef CONFIG_SECURITY_PATH > +/* > + * cap_path_link - verify that hardlinking is allowed > + * @old_dentry: the source inode/dentry to hardlink from > + * @new_dir: target directory > + * @new_dentry: the target inode/dentry to hardlink to > + * > + * Block hardlink when all of: > + * - fsuid does not match inode > + * - not CAP_FOWNER > + * - and at least one of: > + * - inode is not a regular file > + * - inode is setuid > + * - inode is setgid and group-exec > + * - access failure for read or write > + * > + * Returns 0 if successful, -ve on error. > + */ > +int cap_path_link(struct dentry *old_dentry, struct path *new_dir, > + struct dentry *new_dentry) > +{ > + struct inode *inode = old_dentry->d_inode; > + const int mode = inode->i_mode; > + const struct cred *cred = current_cred(); > + > + if (weak_nonaccess_hardlinks) return 0; > + > + if (cred->fsuid != inode->i_uid && > + (!S_ISREG(mode) || (mode & S_ISUID) || > + ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) || > + (generic_permission(inode, MAY_READ | MAY_WRITE, NULL))) && > + !capable(CAP_FOWNER)) { > + printk_ratelimited(KERN_INFO "deprecated non-accessible" > + " hardlink creation was attempted by: %s\n", > + current->comm); > + return -EPERM; > + } > + > + return 0; > +} > +#endif /* CONFIG_SECURITY_PATH */ > + > /* > * Calculate the new process capability sets from the capability sets attached > * to a file.
Hi Stefan, On Thu, May 27, 2010 at 11:28:20AM +0200, Stefan Bader wrote: > It is similar to the softlinks, though maybe a little harder to see the results > it causes. The patch itself seems to be ok. > For Lucid SRU I would definitely want to see effects on applications. In the > view of SRU changes not being allowed to cause regressions, would it be an > option to make the default weak_nonaccess_hardlinks=1 for the Lucid version and > therefore rather allow an opt-in? If it gets an SRU, I would want the default set to 1 for sure. Originally I didn't intend for this to go into Lucid, but if there is demand, I would support it being done that way. -Kees
diff --git a/include/linux/security.h b/include/linux/security.h index 92eca95..6c1a6bf 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -79,6 +79,10 @@ extern int cap_task_setioprio(struct task_struct *p, int ioprio); extern int cap_task_setnice(struct task_struct *p, int nice); extern int cap_syslog(int type, bool from_file); extern int cap_vm_enough_memory(struct mm_struct *mm, long pages); +#ifdef CONFIG_SECURITY_PATH +extern int cap_path_link(struct dentry *old_dentry, struct path *new_dir, + struct dentry *new_dentry); +#endif struct msghdr; struct sk_buff; diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 36a104c..4f3ffd0 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -87,6 +87,7 @@ extern int max_threads; extern int core_uses_pid; extern int suid_dumpable; extern int weak_sticky_symlinks; +extern int weak_nonaccess_hardlinks; extern char core_pattern[]; extern unsigned int core_pipe_limit; extern int pid_max; @@ -1424,6 +1425,15 @@ static struct ctl_table fs_table[] = { .mode = 0644, .proc_handler = &proc_dointvec, }, +#ifdef CONFIG_SECURITY_PATH + { + .procname = "weak-nonaccess-hardlinks", + .data = &weak_nonaccess_hardlinks, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = &proc_dointvec, + }, +#endif #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE) { .procname = "binfmt_misc", diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 814b086..82e222d 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -317,7 +317,10 @@ static int apparmor_path_link(struct dentry *old_dentry, struct path *new_dir, struct dentry *new_dentry) { struct aa_profile *profile; - int error = 0; + int error = 0, rc; + + if ( (rc = cap_path_link(old_dentry, new_dir, new_dentry)) ) + return rc; if (!mediated_filesystem(old_dentry->d_inode)) return 0; diff --git a/security/capability.c b/security/capability.c index d4633f3..75eb6b0 100644 --- a/security/capability.c +++ b/security/capability.c @@ -285,12 +285,6 @@ static int cap_path_symlink(struct path *dir, struct dentry *dentry, return 0; } -static int cap_path_link(struct dentry *old_dentry, struct path *new_dir, - struct dentry *new_dentry) -{ - return 0; -} - static int cap_path_rename(struct path *old_path, struct dentry *old_dentry, struct path *new_path, struct dentry *new_dentry) { diff --git a/security/commoncap.c b/security/commoncap.c index 83d5a18..133ca08 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -31,6 +31,8 @@ /* sysctl for symlink permissions checking */ int weak_sticky_symlinks; +/* sysctl for hardlink permissions checking */ +int weak_nonaccess_hardlinks; /* * If a non-root user executes a setuid-root binary in @@ -304,6 +306,48 @@ int cap_inode_follow_link(struct dentry *dentry, return 0; } +#ifdef CONFIG_SECURITY_PATH +/* + * cap_path_link - verify that hardlinking is allowed + * @old_dentry: the source inode/dentry to hardlink from + * @new_dir: target directory + * @new_dentry: the target inode/dentry to hardlink to + * + * Block hardlink when all of: + * - fsuid does not match inode + * - not CAP_FOWNER + * - and at least one of: + * - inode is not a regular file + * - inode is setuid + * - inode is setgid and group-exec + * - access failure for read or write + * + * Returns 0 if successful, -ve on error. + */ +int cap_path_link(struct dentry *old_dentry, struct path *new_dir, + struct dentry *new_dentry) +{ + struct inode *inode = old_dentry->d_inode; + const int mode = inode->i_mode; + const struct cred *cred = current_cred(); + + if (weak_nonaccess_hardlinks) return 0; + + if (cred->fsuid != inode->i_uid && + (!S_ISREG(mode) || (mode & S_ISUID) || + ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) || + (generic_permission(inode, MAY_READ | MAY_WRITE, NULL))) && + !capable(CAP_FOWNER)) { + printk_ratelimited(KERN_INFO "deprecated non-accessible" + " hardlink creation was attempted by: %s\n", + current->comm); + return -EPERM; + } + + return 0; +} +#endif /* CONFIG_SECURITY_PATH */ + /* * Calculate the new process capability sets from the capability sets attached * to a file.
Hardlinks can be abused in a similar fashion to symlinks above, but they are not limited to world-writable directories. If /etc and /home are on the same partition, a regular user can create a hardlink to /etc/shadow in their home directory. While it retains the original owner and permissions, it is possible for privileged programs that are otherwise symlink-safe to mistakenly access the file through its hardlink. Additionally, a very minor untraceable quota-bypassing local denial of service is possible by an attacker exhausting disk space by filling a world-writable directory with hardlinks. The solution is to not allow the creation of hardlinks to files that a given user would be unable to read or write originally, or are otherwise sensitive. Some links to the history of its discussion: 1997 Dec, Yuri Kuzmenko http://lkml.org/lkml/1997/12/29/20 2002 Apr, Chris Wright http://lkml.org/lkml/2002/4/13/99 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 atd, courier, and other unknown applications that use this feature. - These applications are easy to spot and can be tested and fixed. Applications that are vulnerable to hardlink attacks by not having the change aren't. - Applications should correctly drop privileges before attempting to access user files. - 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-nonaccess-hardlinks, as well as a ratelimited deprecation warning. Signed-off-by: Kees Cook <kees.cook@canonical.com> --- include/linux/security.h | 4 ++++ kernel/sysctl.c | 10 ++++++++++ security/apparmor/lsm.c | 5 ++++- security/capability.c | 6 ------ security/commoncap.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+), 7 deletions(-)