* [PATCH 0/2] setgid hardening @ 2017-01-25 21:06 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski The kernel has some dangerous behavior involving the creation and modification of setgid executables. These issues aren't kernel security bugs per se, but they have been used to turn various filesystem permission oddities into reliably privilege escalation exploits. See http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/ for a nice writeup. Let's fix them for real. Andy Lutomirski (2): fs: Check f_cred instead of current's creds in should_remove_suid() fs: Harden against open(..., O_CREAT, 02777) in a setgid directory fs/inode.c | 37 ++++++++++++++++++++++++++++++------- fs/internal.h | 2 +- fs/ocfs2/file.c | 4 ++-- fs/open.c | 2 +- include/linux/fs.h | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/2] setgid hardening @ 2017-01-25 21:06 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski The kernel has some dangerous behavior involving the creation and modification of setgid executables. These issues aren't kernel security bugs per se, but they have been used to turn various filesystem permission oddities into reliably privilege escalation exploits. See http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/ for a nice writeup. Let's fix them for real. Andy Lutomirski (2): fs: Check f_cred instead of current's creds in should_remove_suid() fs: Harden against open(..., O_CREAT, 02777) in a setgid directory fs/inode.c | 37 ++++++++++++++++++++++++++++++------- fs/internal.h | 2 +- fs/ocfs2/file.c | 4 ++-- fs/open.c | 2 +- include/linux/fs.h | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-) -- 2.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() 2017-01-25 21:06 ` Andy Lutomirski @ 2017-01-25 21:06 ` Andy Lutomirski -1 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski, stable If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit. Fix it by checking f_cred instead of current's creds whenever a struct file is involved. I don't know why we check capabilities at all, and we could probably get away with clearing the setgid bit regardless of capabilities, but this change should be less likely to break some weird program. This mitigates exploits that take advantage of world-writable setgid files or directories. Cc: stable@vger.kernel.org Signed-off-by: Andy Lutomirski <luto@kernel.org> --- fs/inode.c | 16 +++++++++++----- fs/internal.h | 2 +- fs/ocfs2/file.c | 4 ++-- fs/open.c | 2 +- include/linux/fs.h | 2 +- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/fs/inode.c b/fs/inode.c index 88110fd0b282..f7029c40cfbd 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime); * * if suid or (sgid and xgrp) * remove privs + * + * If a file is provided, we assume that this is ftruncate() or similar + * on that file. If a file is not provided, we assume that no file + * descriptor is involved. */ -int should_remove_suid(struct dentry *dentry) +int should_remove_suid(struct dentry *dentry, struct file *file) { umode_t mode = d_inode(dentry)->i_mode; int kill = 0; @@ -1750,7 +1754,9 @@ int should_remove_suid(struct dentry *dentry) if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) kill |= ATTR_KILL_SGID; - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) + if (unlikely(kill && S_ISREG(mode) && + !(file ? file_ns_capable(file, &init_user_ns, CAP_FSETID) : + capable(CAP_FSETID)))) return kill; return 0; @@ -1762,7 +1768,7 @@ EXPORT_SYMBOL(should_remove_suid); * response to write or truncate. Return 0 if nothing has to be changed. * Negative value on error (change should be denied). */ -int dentry_needs_remove_privs(struct dentry *dentry) +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file) { struct inode *inode = d_inode(dentry); int mask = 0; @@ -1771,7 +1777,7 @@ int dentry_needs_remove_privs(struct dentry *dentry) if (IS_NOSEC(inode)) return 0; - mask = should_remove_suid(dentry); + mask = should_remove_suid(dentry, file); ret = security_inode_need_killpriv(dentry); if (ret < 0) return ret; @@ -1807,7 +1813,7 @@ int file_remove_privs(struct file *file) if (IS_NOSEC(inode)) return 0; - kill = dentry_needs_remove_privs(dentry); + kill = dentry_needs_remove_privs(dentry, file); if (kill < 0) return kill; if (kill) diff --git a/fs/internal.h b/fs/internal.h index b63cf3af2dc2..c467ad502cac 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *); */ extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc); extern void inode_add_lru(struct inode *inode); -extern int dentry_needs_remove_privs(struct dentry *dentry); +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file); extern bool __atime_needs_update(const struct path *, struct inode *, bool); static inline bool atime_needs_update_rcu(const struct path *path, diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index c4889655d32b..db6efd940ac0 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode, } } - if (file && should_remove_suid(file->f_path.dentry)) { + if (file && should_remove_suid(file->f_path.dentry, file)) { ret = __ocfs2_write_remove_suid(inode, di_bh); if (ret) { mlog_errno(ret); @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file, * inode. There's also the dinode i_size state which * can be lost via setattr during extending writes (we * set inode->i_size at the end of a write. */ - if (should_remove_suid(dentry)) { + if (should_remove_suid(dentry, file)) { if (meta_level == 0) { ocfs2_inode_unlock(inode, meta_level); meta_level = 1; diff --git a/fs/open.c b/fs/open.c index d3ed8171e8e0..8f54f34d1e3e 100644 --- a/fs/open.c +++ b/fs/open.c @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, } /* Remove suid, sgid, and file capabilities on truncate too */ - ret = dentry_needs_remove_privs(dentry); + ret = dentry_needs_remove_privs(dentry, filp); if (ret < 0) return ret; if (ret) diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba074328894..87654fb21158 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *); extern struct inode *new_inode_pseudo(struct super_block *sb); extern struct inode *new_inode(struct super_block *sb); extern void free_inode_nonrcu(struct inode *inode); -extern int should_remove_suid(struct dentry *); +extern int should_remove_suid(struct dentry *, struct file *); extern int file_remove_privs(struct file *); extern void __insert_inode_hash(struct inode *, unsigned long hashval); -- 2.9.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() @ 2017-01-25 21:06 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski, stable If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit. Fix it by checking f_cred instead of current's creds whenever a struct file is involved. I don't know why we check capabilities at all, and we could probably get away with clearing the setgid bit regardless of capabilities, but this change should be less likely to break some weird program. This mitigates exploits that take advantage of world-writable setgid files or directories. Cc: stable@vger.kernel.org Signed-off-by: Andy Lutomirski <luto@kernel.org> --- fs/inode.c | 16 +++++++++++----- fs/internal.h | 2 +- fs/ocfs2/file.c | 4 ++-- fs/open.c | 2 +- include/linux/fs.h | 2 +- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/fs/inode.c b/fs/inode.c index 88110fd0b282..f7029c40cfbd 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime); * * if suid or (sgid and xgrp) * remove privs + * + * If a file is provided, we assume that this is ftruncate() or similar + * on that file. If a file is not provided, we assume that no file + * descriptor is involved. */ -int should_remove_suid(struct dentry *dentry) +int should_remove_suid(struct dentry *dentry, struct file *file) { umode_t mode = d_inode(dentry)->i_mode; int kill = 0; @@ -1750,7 +1754,9 @@ int should_remove_suid(struct dentry *dentry) if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) kill |= ATTR_KILL_SGID; - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) + if (unlikely(kill && S_ISREG(mode) && + !(file ? file_ns_capable(file, &init_user_ns, CAP_FSETID) : + capable(CAP_FSETID)))) return kill; return 0; @@ -1762,7 +1768,7 @@ EXPORT_SYMBOL(should_remove_suid); * response to write or truncate. Return 0 if nothing has to be changed. * Negative value on error (change should be denied). */ -int dentry_needs_remove_privs(struct dentry *dentry) +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file) { struct inode *inode = d_inode(dentry); int mask = 0; @@ -1771,7 +1777,7 @@ int dentry_needs_remove_privs(struct dentry *dentry) if (IS_NOSEC(inode)) return 0; - mask = should_remove_suid(dentry); + mask = should_remove_suid(dentry, file); ret = security_inode_need_killpriv(dentry); if (ret < 0) return ret; @@ -1807,7 +1813,7 @@ int file_remove_privs(struct file *file) if (IS_NOSEC(inode)) return 0; - kill = dentry_needs_remove_privs(dentry); + kill = dentry_needs_remove_privs(dentry, file); if (kill < 0) return kill; if (kill) diff --git a/fs/internal.h b/fs/internal.h index b63cf3af2dc2..c467ad502cac 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *); */ extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc); extern void inode_add_lru(struct inode *inode); -extern int dentry_needs_remove_privs(struct dentry *dentry); +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file); extern bool __atime_needs_update(const struct path *, struct inode *, bool); static inline bool atime_needs_update_rcu(const struct path *path, diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index c4889655d32b..db6efd940ac0 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode, } } - if (file && should_remove_suid(file->f_path.dentry)) { + if (file && should_remove_suid(file->f_path.dentry, file)) { ret = __ocfs2_write_remove_suid(inode, di_bh); if (ret) { mlog_errno(ret); @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file, * inode. There's also the dinode i_size state which * can be lost via setattr during extending writes (we * set inode->i_size at the end of a write. */ - if (should_remove_suid(dentry)) { + if (should_remove_suid(dentry, file)) { if (meta_level == 0) { ocfs2_inode_unlock(inode, meta_level); meta_level = 1; diff --git a/fs/open.c b/fs/open.c index d3ed8171e8e0..8f54f34d1e3e 100644 --- a/fs/open.c +++ b/fs/open.c @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, } /* Remove suid, sgid, and file capabilities on truncate too */ - ret = dentry_needs_remove_privs(dentry); + ret = dentry_needs_remove_privs(dentry, filp); if (ret < 0) return ret; if (ret) diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba074328894..87654fb21158 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *); extern struct inode *new_inode_pseudo(struct super_block *sb); extern struct inode *new_inode(struct super_block *sb); extern void free_inode_nonrcu(struct inode *inode); -extern int should_remove_suid(struct dentry *); +extern int should_remove_suid(struct dentry *, struct file *); extern int file_remove_privs(struct file *); extern void __insert_inode_hash(struct inode *, unsigned long hashval); -- 2.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() 2017-01-25 21:06 ` Andy Lutomirski (?) @ 2017-01-25 21:43 ` Ben Hutchings 2017-01-25 21:48 ` Andy Lutomirski 2017-01-26 0:12 ` Kees Cook -1 siblings, 2 replies; 24+ messages in thread From: Ben Hutchings @ 2017-01-25 21:43 UTC (permalink / raw) To: Andy Lutomirski, security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, stable [-- Attachment #1: Type: text/plain, Size: 1099 bytes --] On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > If an unprivileged program opens a setgid file for write and passes > the fd to a privileged program and the privileged program writes to > it, we currently fail to clear the setgid bit. Fix it by checking > f_cred instead of current's creds whenever a struct file is > involved. [...] What if, instead, a privileged program passes the fd to an un unprivileged program? It sounds like a bad idea to start with, but at least currently the unprivileged program is going to clear the setgid bit when it writes. This change would make that behaviour more dangerous. Perhaps there should be a capability check on both the current credentials and file credentials? (I realise that we've considered file credential checks to be sufficient elsewhere, but those cases involved virtual files with special semantics, where it's clearer that a privileged process should not pass them to an unprivileged process.) Ben. -- Ben Hutchings It is easier to write an incorrect program than to understand a correct one. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() 2017-01-25 21:43 ` Ben Hutchings @ 2017-01-25 21:48 ` Andy Lutomirski 2017-01-26 0:12 ` Kees Cook 1 sibling, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:48 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, stable On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> If an unprivileged program opens a setgid file for write and passes >> the fd to a privileged program and the privileged program writes to >> it, we currently fail to clear the setgid bit. Fix it by checking >> f_cred instead of current's creds whenever a struct file is >> involved. > [...] > > What if, instead, a privileged program passes the fd to an un > unprivileged program? It sounds like a bad idea to start with, but at > least currently the unprivileged program is going to clear the setgid > bit when it writes. This change would make that behaviour more > dangerous. Hmm. Although, if a privileged program does something like: (sudo -u nobody echo blah) >setuid_program presumably it wanted to make the change. > > Perhaps there should be a capability check on both the current > credentials and file credentials? (I realise that we've considered > file credential checks to be sufficient elsewhere, but those cases > involved virtual files with special semantics, where it's clearer that > a privileged process should not pass them to an unprivileged process.) > I could go either way. What I really want to do is to write a third patch that isn't for -stable that just removes the capable() check entirely. I'm reasonably confident it won't break things for a silly reason: because it's capable() and not ns_capable(), anything it would break would also be broken in an unprivileged container, and I haven't seen any reports of package managers or similar breaking for this reason. --Andy ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() @ 2017-01-25 21:48 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:48 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, stable On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> If an unprivileged program opens a setgid file for write and passes >> the fd to a privileged program and the privileged program writes to >> it, we currently fail to clear the setgid bit. Fix it by checking >> f_cred instead of current's creds whenever a struct file is >> involved. > [...] > > What if, instead, a privileged program passes the fd to an un > unprivileged program? It sounds like a bad idea to start with, but at > least currently the unprivileged program is going to clear the setgid > bit when it writes. This change would make that behaviour more > dangerous. Hmm. Although, if a privileged program does something like: (sudo -u nobody echo blah) >setuid_program presumably it wanted to make the change. > > Perhaps there should be a capability check on both the current > credentials and file credentials? (I realise that we've considered > file credential checks to be sufficient elsewhere, but those cases > involved virtual files with special semantics, where it's clearer that > a privileged process should not pass them to an unprivileged process.) > I could go either way. What I really want to do is to write a third patch that isn't for -stable that just removes the capable() check entirely. I'm reasonably confident it won't break things for a silly reason: because it's capable() and not ns_capable(), anything it would break would also be broken in an unprivileged container, and I haven't seen any reports of package managers or similar breaking for this reason. --Andy -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() 2017-01-25 21:48 ` Andy Lutomirski (?) @ 2017-01-25 23:15 ` Frank Filz -1 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:15 UTC (permalink / raw) To: 'Andy Lutomirski', 'Ben Hutchings' Cc: 'Andy Lutomirski', security, 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel', 'stable' > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> > wrote: > > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > >> If an unprivileged program opens a setgid file for write and passes > >> the fd to a privileged program and the privileged program writes to > >> it, we currently fail to clear the setgid bit. Fix it by checking > >> f_cred instead of current's creds whenever a struct file is involved. > > [...] > > > > What if, instead, a privileged program passes the fd to an un > > unprivileged program? It sounds like a bad idea to start with, but at > > least currently the unprivileged program is going to clear the setgid > > bit when it writes. This change would make that behaviour more > > dangerous. > > Hmm. Although, if a privileged program does something like: > > (sudo -u nobody echo blah) >setuid_program > > presumably it wanted to make the change. I'm not following all the intricacies here, though I need to... What about a privileged program that drops privilege for certain operations? Specifically the Ganesha user space NFS server runs as root, but sets fsuid/fsgid for specific threads performing I/O operations on behalf of NFS clients. I want to make sure setgid bit handling is proper for these cases. Ganesha does some permission checking, but this is one area I want to defer to the underlying filesystem because it's not easy for Ganesha to get it right. > > Perhaps there should be a capability check on both the current > > credentials and file credentials? (I realise that we've considered > > file credential checks to be sufficient elsewhere, but those cases > > involved virtual files with special semantics, where it's clearer that > > a privileged process should not pass them to an unprivileged process.) > > > > I could go either way. > > What I really want to do is to write a third patch that isn't for -stable that just > removes the capable() check entirely. I'm reasonably confident it won't > break things for a silly reason: because it's capable() and not ns_capable(), > anything it would break would also be broken in an unprivileged container, > and I haven't seen any reports of package managers or similar breaking for > this reason. Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() @ 2017-01-25 23:15 ` Frank Filz 0 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:15 UTC (permalink / raw) To: 'Andy Lutomirski', 'Ben Hutchings' Cc: 'Andy Lutomirski', security, 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel', 'stable' > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> > wrote: > > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > >> If an unprivileged program opens a setgid file for write and passes > >> the fd to a privileged program and the privileged program writes to > >> it, we currently fail to clear the setgid bit. Fix it by checking > >> f_cred instead of current's creds whenever a struct file is involved. > > [...] > > > > What if, instead, a privileged program passes the fd to an un > > unprivileged program? It sounds like a bad idea to start with, but at > > least currently the unprivileged program is going to clear the setgid > > bit when it writes. This change would make that behaviour more > > dangerous. > > Hmm. Although, if a privileged program does something like: > > (sudo -u nobody echo blah) >setuid_program > > presumably it wanted to make the change. I'm not following all the intricacies here, though I need to... What about a privileged program that drops privilege for certain operations? Specifically the Ganesha user space NFS server runs as root, but sets fsuid/fsgid for specific threads performing I/O operations on behalf of NFS clients. I want to make sure setgid bit handling is proper for these cases. Ganesha does some permission checking, but this is one area I want to defer to the underlying filesystem because it's not easy for Ganesha to get it right. > > Perhaps there should be a capability check on both the current > > credentials and file credentials? (I realise that we've considered > > file credential checks to be sufficient elsewhere, but those cases > > involved virtual files with special semantics, where it's clearer that > > a privileged process should not pass them to an unprivileged process.) > > > > I could go either way. > > What I really want to do is to write a third patch that isn't for -stable that just > removes the capable() check entirely. I'm reasonably confident it won't > break things for a silly reason: because it's capable() and not ns_capable(), > anything it would break would also be broken in an unprivileged container, > and I haven't seen any reports of package managers or similar breaking for > this reason. Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() @ 2017-01-25 23:15 ` Frank Filz 0 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:15 UTC (permalink / raw) To: 'Andy Lutomirski', 'Ben Hutchings' Cc: 'Andy Lutomirski', security, 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel', 'stable' > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> > wrote: > > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > >> If an unprivileged program opens a setgid file for write and passes > >> the fd to a privileged program and the privileged program writes to > >> it, we currently fail to clear the setgid bit. Fix it by checking > >> f_cred instead of current's creds whenever a struct file is involved. > > [...] > > > > What if, instead, a privileged program passes the fd to an un > > unprivileged program? It sounds like a bad idea to start with, but at > > least currently the unprivileged program is going to clear the setgid > > bit when it writes. This change would make that behaviour more > > dangerous. > > Hmm. Although, if a privileged program does something like: > > (sudo -u nobody echo blah) >setuid_program > > presumably it wanted to make the change. I'm not following all the intricacies here, though I need to... What about a privileged program that drops privilege for certain operations? Specifically the Ganesha user space NFS server runs as root, but sets fsuid/fsgid for specific threads performing I/O operations on behalf of NFS clients. I want to make sure setgid bit handling is proper for these cases. Ganesha does some permission checking, but this is one area I want to defer to the underlying filesystem because it's not easy for Ganesha to get it right. > > Perhaps there should be a capability check on both the current > > credentials and file credentials? (I realise that we've considered > > file credential checks to be sufficient elsewhere, but those cases > > involved virtual files with special semantics, where it's clearer that > > a privileged process should not pass them to an unprivileged process.) > > > > I could go either way. > > What I really want to do is to write a third patch that isn't for -stable that just > removes the capable() check entirely. I'm reasonably confident it won't > break things for a silly reason: because it's capable() and not ns_capable(), > anything it would break would also be broken in an unprivileged container, > and I haven't seen any reports of package managers or similar breaking for > this reason. Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() 2017-01-25 21:43 ` Ben Hutchings @ 2017-01-26 0:12 ` Kees Cook 2017-01-26 0:12 ` Kees Cook 1 sibling, 0 replies; 24+ messages in thread From: Kees Cook @ 2017-01-26 0:12 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, # 3.4.x On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> If an unprivileged program opens a setgid file for write and passes >> the fd to a privileged program and the privileged program writes to >> it, we currently fail to clear the setgid bit. Fix it by checking >> f_cred instead of current's creds whenever a struct file is >> involved. > [...] > > What if, instead, a privileged program passes the fd to an un > unprivileged program? It sounds like a bad idea to start with, but at > least currently the unprivileged program is going to clear the setgid > bit when it writes. This change would make that behaviour more > dangerous. > > Perhaps there should be a capability check on both the current > credentials and file credentials? (I realise that we've considered > file credential checks to be sufficient elsewhere, but those cases > involved virtual files with special semantics, where it's clearer that > a privileged process should not pass them to an unprivileged process.) We need a set of self-tests for this whole area. :( There are so many corner cases. We still have an unfixed corner case with mmap writes not clearing set*id bits that I tried to solve last year... -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() @ 2017-01-26 0:12 ` Kees Cook 0 siblings, 0 replies; 24+ messages in thread From: Kees Cook @ 2017-01-26 0:12 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, # 3.4.x On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> If an unprivileged program opens a setgid file for write and passes >> the fd to a privileged program and the privileged program writes to >> it, we currently fail to clear the setgid bit. Fix it by checking >> f_cred instead of current's creds whenever a struct file is >> involved. > [...] > > What if, instead, a privileged program passes the fd to an un > unprivileged program? It sounds like a bad idea to start with, but at > least currently the unprivileged program is going to clear the setgid > bit when it writes. This change would make that behaviour more > dangerous. > > Perhaps there should be a capability check on both the current > credentials and file credentials? (I realise that we've considered > file credential checks to be sufficient elsewhere, but those cases > involved virtual files with special semantics, where it's clearer that > a privileged process should not pass them to an unprivileged process.) We need a set of self-tests for this whole area. :( There are so many corner cases. We still have an unfixed corner case with mmap writes not clearing set*id bits that I tried to solve last year... -Kees -- Kees Cook Nexus Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory 2017-01-25 21:06 ` Andy Lutomirski @ 2017-01-25 21:06 ` Andy Lutomirski -1 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's fsgid, you end up with an SGID executable that is owned by the directory's GID. This is a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous. Harden against this type of attack by detecting this particular corner case (unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- fs/inode.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/fs/inode.c b/fs/inode.c index f7029c40cfbd..d7e4b80470dd 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, { inode->i_uid = current_fsuid(); if (dir && dir->i_mode & S_ISGID) { + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); + inode->i_gid = dir->i_gid; - if (S_ISDIR(mode)) + if (S_ISDIR(mode)) { mode |= S_ISGID; - } else + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) + && S_ISREG(mode) && changing_gid + && !capable(CAP_FSETID)) { + /* + * Whoa there! An unprivileged program just + * tried to create a new executable with SGID + * set in a directory with SGID set that belongs + * to a different group. Don't let this program + * create a SGID executable that ends up owned + * by the wrong group. + */ + mode &= ~S_ISGID; + } + + } else { inode->i_gid = current_fsgid(); + } inode->i_mode = mode; } EXPORT_SYMBOL(inode_init_owner); -- 2.9.3 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory @ 2017-01-25 21:06 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:06 UTC (permalink / raw) To: security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Andy Lutomirski Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's fsgid, you end up with an SGID executable that is owned by the directory's GID. This is a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous. Harden against this type of attack by detecting this particular corner case (unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- fs/inode.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/fs/inode.c b/fs/inode.c index f7029c40cfbd..d7e4b80470dd 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, { inode->i_uid = current_fsuid(); if (dir && dir->i_mode & S_ISGID) { + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); + inode->i_gid = dir->i_gid; - if (S_ISDIR(mode)) + if (S_ISDIR(mode)) { mode |= S_ISGID; - } else + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) + && S_ISREG(mode) && changing_gid + && !capable(CAP_FSETID)) { + /* + * Whoa there! An unprivileged program just + * tried to create a new executable with SGID + * set in a directory with SGID set that belongs + * to a different group. Don't let this program + * create a SGID executable that ends up owned + * by the wrong group. + */ + mode &= ~S_ISGID; + } + + } else { inode->i_gid = current_fsgid(); + } inode->i_mode = mode; } EXPORT_SYMBOL(inode_init_owner); -- 2.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory 2017-01-25 21:06 ` Andy Lutomirski (?) @ 2017-01-25 21:31 ` Ben Hutchings 2017-01-25 21:44 ` Andy Lutomirski -1 siblings, 1 reply; 24+ messages in thread From: Ben Hutchings @ 2017-01-25 21:31 UTC (permalink / raw) To: Andy Lutomirski, security Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel [-- Attachment #1: Type: text/plain, Size: 1498 bytes --] On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's > fsgid, you end up with an SGID executable that is owned by the > directory's GID. This is a Bad Thing (tm). Exploiting this is > nontrivial because most ways of creating a new file create an empty > file and empty executables aren't particularly interesting, but this > is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular > corner case (unprivileged program creates SGID executable inode in > SGID directory owned by a different GID) and clearing the new > inode's SGID bit. > > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > fs/inode.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index f7029c40cfbd..d7e4b80470dd 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, > { > inode->i_uid = current_fsuid(); > if (dir && dir->i_mode & S_ISGID) { > + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); [...] inode->i_gid hasn't been initialised yet. This should compare with current_fsgid(), shouldn't it? Ben. -- Ben Hutchings It is easier to write an incorrect program than to understand a correct one. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory 2017-01-25 21:31 ` Ben Hutchings @ 2017-01-25 21:44 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:44 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 1:31 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a >> directory that is setgid and owned by a different gid than current's >> fsgid, you end up with an SGID executable that is owned by the >> directory's GID. This is a Bad Thing (tm). Exploiting this is >> nontrivial because most ways of creating a new file create an empty >> file and empty executables aren't particularly interesting, but this >> is nevertheless quite dangerous. >> >> Harden against this type of attack by detecting this particular >> corner case (unprivileged program creates SGID executable inode in >> SGID directory owned by a different GID) and clearing the new >> inode's SGID bit. >> >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> --- >> fs/inode.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index f7029c40cfbd..d7e4b80470dd 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, >> { >> inode->i_uid = current_fsuid(); >> if (dir && dir->i_mode & S_ISGID) { >> + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); > [...] > > inode->i_gid hasn't been initialised yet. This should compare with > current_fsgid(), shouldn't it? Whoops. In v2, I'll fix it by inode->i_gid first -- that'll simplify the control flow. > > Ben. > > -- > Ben Hutchings > It is easier to write an incorrect program than to understand a correct > one. > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory @ 2017-01-25 21:44 ` Andy Lutomirski 0 siblings, 0 replies; 24+ messages in thread From: Andy Lutomirski @ 2017-01-25 21:44 UTC (permalink / raw) To: Ben Hutchings Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 1:31 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a >> directory that is setgid and owned by a different gid than current's >> fsgid, you end up with an SGID executable that is owned by the >> directory's GID. This is a Bad Thing (tm). Exploiting this is >> nontrivial because most ways of creating a new file create an empty >> file and empty executables aren't particularly interesting, but this >> is nevertheless quite dangerous. >> >> Harden against this type of attack by detecting this particular >> corner case (unprivileged program creates SGID executable inode in >> SGID directory owned by a different GID) and clearing the new >> inode's SGID bit. >> >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> --- >> fs/inode.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index f7029c40cfbd..d7e4b80470dd 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, >> { >> inode->i_uid = current_fsuid(); >> if (dir && dir->i_mode & S_ISGID) { >> + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); > [...] > > inode->i_gid hasn't been initialised yet. This should compare with > current_fsgid(), shouldn't it? Whoops. In v2, I'll fix it by inode->i_gid first -- that'll simplify the control flow. > > Ben. > > -- > Ben Hutchings > It is easier to write an incorrect program than to understand a correct > one. > -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory 2017-01-25 21:06 ` Andy Lutomirski (?) @ 2017-01-25 23:17 ` Frank Filz -1 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:17 UTC (permalink / raw) To: 'Andy Lutomirski', security Cc: 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel' > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's fsgid, you > end up with an SGID executable that is owned by the directory's GID. This is > a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a > new file create an empty file and empty executables aren't particularly > interesting, but this is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular corner case > (unprivileged program creates SGID executable inode in SGID directory > owned by a different GID) and clearing the new inode's SGID bit. Nasty. I'd love to see a test for this in xfstests and/or pjdfstests... Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory @ 2017-01-25 23:17 ` Frank Filz 0 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:17 UTC (permalink / raw) To: 'Andy Lutomirski', security Cc: 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel' > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's fsgid, you > end up with an SGID executable that is owned by the directory's GID. This is > a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a > new file create an empty file and empty executables aren't particularly > interesting, but this is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular corner case > (unprivileged program creates SGID executable inode in SGID directory > owned by a different GID) and clearing the new inode's SGID bit. Nasty. I'd love to see a test for this in xfstests and/or pjdfstests... Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory @ 2017-01-25 23:17 ` Frank Filz 0 siblings, 0 replies; 24+ messages in thread From: Frank Filz @ 2017-01-25 23:17 UTC (permalink / raw) To: 'Andy Lutomirski', security Cc: 'Konstantin Khlebnikov', 'Alexander Viro', 'Kees Cook', 'Willy Tarreau', linux-mm, 'Andrew Morton', 'yalin wang', 'Linux Kernel Mailing List', 'Jan Kara', 'Linux FS Devel' > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's fsgid, you > end up with an SGID executable that is owned by the directory's GID. This is > a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a > new file create an empty file and empty executables aren't particularly > interesting, but this is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular corner case > (unprivileged program creates SGID executable inode in SGID directory > owned by a different GID) and clearing the new inode's SGID bit. Nasty. I'd love to see a test for this in xfstests and/or pjdfstests... Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory 2017-01-25 21:06 ` Andy Lutomirski @ 2017-01-25 23:50 ` Willy Tarreau -1 siblings, 0 replies; 24+ messages in thread From: Willy Tarreau @ 2017-01-25 23:50 UTC (permalink / raw) To: Andy Lutomirski Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 01:06:52PM -0800, Andy Lutomirski wrote: > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's > fsgid, you end up with an SGID executable that is owned by the > directory's GID. This is a Bad Thing (tm). Exploiting this is > nontrivial because most ways of creating a new file create an empty > file and empty executables aren't particularly interesting, but this > is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular > corner case (unprivileged program creates SGID executable inode in > SGID directory owned by a different GID) and clearing the new > inode's SGID bit. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > fs/inode.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index f7029c40cfbd..d7e4b80470dd 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, > { > inode->i_uid = current_fsuid(); > if (dir && dir->i_mode & S_ISGID) { > + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); > + > inode->i_gid = dir->i_gid; > - if (S_ISDIR(mode)) > + if (S_ISDIR(mode)) { > mode |= S_ISGID; > - } else > + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) > + && S_ISREG(mode) && changing_gid > + && !capable(CAP_FSETID)) { > + /* > + * Whoa there! An unprivileged program just > + * tried to create a new executable with SGID > + * set in a directory with SGID set that belongs > + * to a different group. Don't let this program > + * create a SGID executable that ends up owned > + * by the wrong group. > + */ > + mode &= ~S_ISGID; > + } > + > + } else { > inode->i_gid = current_fsgid(); > + } > inode->i_mode = mode; > } It seems to me like you're leaving inode->i_gid uninitialized when you take the Woah branch here. Or at least it's not obvious to me. I'd rather adjust it like this to make it easier to read (patched edited by hand, sorry for the bad formating) and it also covers the case where the gid_eq() check was apparently performed on something uninitialized : { inode->i_uid = current_fsuid(); + inode->i_gid = current_fsgid(); if (dir && dir->i_mode & S_ISGID) { + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); + inode->i_gid = dir->i_gid; - if (S_ISDIR(mode)) + if (S_ISDIR(mode)) { mode |= S_ISGID; - } else + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) + && S_ISREG(mode) && changing_gid + && !capable(CAP_FSETID)) { + /* + * Whoa there! An unprivileged program just + * tried to create a new executable with SGID + * set in a directory with SGID set that belongs + * to a different group. Don't let this program + * create a SGID executable that ends up owned + * by the wrong group. + */ + mode &= ~S_ISGID; + } + } inode->i_mode = mode; } Please ignore all this if I missed something. Willy ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory @ 2017-01-25 23:50 ` Willy Tarreau 0 siblings, 0 replies; 24+ messages in thread From: Willy Tarreau @ 2017-01-25 23:50 UTC (permalink / raw) To: Andy Lutomirski Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 01:06:52PM -0800, Andy Lutomirski wrote: > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's > fsgid, you end up with an SGID executable that is owned by the > directory's GID. This is a Bad Thing (tm). Exploiting this is > nontrivial because most ways of creating a new file create an empty > file and empty executables aren't particularly interesting, but this > is nevertheless quite dangerous. > > Harden against this type of attack by detecting this particular > corner case (unprivileged program creates SGID executable inode in > SGID directory owned by a different GID) and clearing the new > inode's SGID bit. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > fs/inode.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index f7029c40cfbd..d7e4b80470dd 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, > { > inode->i_uid = current_fsuid(); > if (dir && dir->i_mode & S_ISGID) { > + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); > + > inode->i_gid = dir->i_gid; > - if (S_ISDIR(mode)) > + if (S_ISDIR(mode)) { > mode |= S_ISGID; > - } else > + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) > + && S_ISREG(mode) && changing_gid > + && !capable(CAP_FSETID)) { > + /* > + * Whoa there! An unprivileged program just > + * tried to create a new executable with SGID > + * set in a directory with SGID set that belongs > + * to a different group. Don't let this program > + * create a SGID executable that ends up owned > + * by the wrong group. > + */ > + mode &= ~S_ISGID; > + } > + > + } else { > inode->i_gid = current_fsgid(); > + } > inode->i_mode = mode; > } It seems to me like you're leaving inode->i_gid uninitialized when you take the Woah branch here. Or at least it's not obvious to me. I'd rather adjust it like this to make it easier to read (patched edited by hand, sorry for the bad formating) and it also covers the case where the gid_eq() check was apparently performed on something uninitialized : { inode->i_uid = current_fsuid(); + inode->i_gid = current_fsgid(); if (dir && dir->i_mode & S_ISGID) { + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); + inode->i_gid = dir->i_gid; - if (S_ISDIR(mode)) + if (S_ISDIR(mode)) { mode |= S_ISGID; - } else + } else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) + && S_ISREG(mode) && changing_gid + && !capable(CAP_FSETID)) { + /* + * Whoa there! An unprivileged program just + * tried to create a new executable with SGID + * set in a directory with SGID set that belongs + * to a different group. Don't let this program + * create a SGID executable that ends up owned + * by the wrong group. + */ + mode &= ~S_ISGID; + } + } inode->i_mode = mode; } Please ignore all this if I missed something. Willy -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2] setgid hardening 2017-01-25 21:06 ` Andy Lutomirski @ 2017-01-25 23:59 ` Willy Tarreau -1 siblings, 0 replies; 24+ messages in thread From: Willy Tarreau @ 2017-01-25 23:59 UTC (permalink / raw) To: Andy Lutomirski Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 01:06:50PM -0800, Andy Lutomirski wrote: > The kernel has some dangerous behavior involving the creation and > modification of setgid executables. These issues aren't kernel > security bugs per se, but they have been used to turn various > filesystem permission oddities into reliably privilege escalation > exploits. > > See http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/ > for a nice writeup. > > Let's fix them for real. BTW I like this. I vaguely remember having played with this when I was a student 2 decades ago on a system where /var/spool/mail was 3777 (yes, setgid+sticky) and the mail files were 660. You could deposit a shell there, then execute it with mail's permissions and access any mailbox. That was quite odd as a design choice. The impacts are often limited unless you find other ways to escalate but generally it's not really clean. Willy ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2] setgid hardening @ 2017-01-25 23:59 ` Willy Tarreau 0 siblings, 0 replies; 24+ messages in thread From: Willy Tarreau @ 2017-01-25 23:59 UTC (permalink / raw) To: Andy Lutomirski Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook, linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List, Jan Kara, Linux FS Devel On Wed, Jan 25, 2017 at 01:06:50PM -0800, Andy Lutomirski wrote: > The kernel has some dangerous behavior involving the creation and > modification of setgid executables. These issues aren't kernel > security bugs per se, but they have been used to turn various > filesystem permission oddities into reliably privilege escalation > exploits. > > See http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/ > for a nice writeup. > > Let's fix them for real. BTW I like this. I vaguely remember having played with this when I was a student 2 decades ago on a system where /var/spool/mail was 3777 (yes, setgid+sticky) and the mail files were 660. You could deposit a shell there, then execute it with mail's permissions and access any mailbox. That was quite odd as a design choice. The impacts are often limited unless you find other ways to escalate but generally it's not really clean. Willy -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-01-26 0:12 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-25 21:06 [PATCH 0/2] setgid hardening Andy Lutomirski 2017-01-25 21:06 ` Andy Lutomirski 2017-01-25 21:06 ` [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() Andy Lutomirski 2017-01-25 21:06 ` Andy Lutomirski 2017-01-25 21:43 ` Ben Hutchings 2017-01-25 21:48 ` Andy Lutomirski 2017-01-25 21:48 ` Andy Lutomirski 2017-01-25 23:15 ` Frank Filz 2017-01-25 23:15 ` Frank Filz 2017-01-25 23:15 ` Frank Filz 2017-01-26 0:12 ` Kees Cook 2017-01-26 0:12 ` Kees Cook 2017-01-25 21:06 ` [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory Andy Lutomirski 2017-01-25 21:06 ` Andy Lutomirski 2017-01-25 21:31 ` Ben Hutchings 2017-01-25 21:44 ` Andy Lutomirski 2017-01-25 21:44 ` Andy Lutomirski 2017-01-25 23:17 ` Frank Filz 2017-01-25 23:17 ` Frank Filz 2017-01-25 23:17 ` Frank Filz 2017-01-25 23:50 ` Willy Tarreau 2017-01-25 23:50 ` Willy Tarreau 2017-01-25 23:59 ` [PATCH 0/2] setgid hardening Willy Tarreau 2017-01-25 23:59 ` Willy Tarreau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.