diff mbox series

[SRU,T/X/A/B/C] Fix up non-directory creation in SGID directories

Message ID 1531767035-5276-2-git-send-email-tyhicks@canonical.com
State New
Headers show
Series [SRU,T/X/A/B/C] Fix up non-directory creation in SGID directories | expand

Commit Message

Tyler Hicks July 16, 2018, 6:50 p.m. UTC
From: Linus Torvalds <torvalds@linux-foundation.org>

BugLink: https://launchpad.net/bugs/1779923
CVE-2018-13405

sgid directories have special semantics, making newly created files in
the directory belong to the group of the directory, and newly created
subdirectories will also become sgid.  This is historically used for
group-shared directories.

But group directories writable by non-group members should not imply
that such non-group members can magically join the group, so make sure
to clear the sgid bit on non-directories for non-members (but remember
that sgid without group execute means "mandatory locking", just to
confuse things even more).

Reported-by: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry picked from commit 0fa3ecd87848c9c93c2c828ef4c3a8ca36ce46c7)
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 fs/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Stefan Bader July 19, 2018, 12:19 p.m. UTC | #1
On 16.07.2018 20:50, Tyler Hicks wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> BugLink: https://launchpad.net/bugs/1779923
> CVE-2018-13405
> 
> sgid directories have special semantics, making newly created files in
> the directory belong to the group of the directory, and newly created
> subdirectories will also become sgid.  This is historically used for
> group-shared directories.
> 
> But group directories writable by non-group members should not imply
> that such non-group members can magically join the group, so make sure
> to clear the sgid bit on non-directories for non-members (but remember
> that sgid without group execute means "mandatory locking", just to
> confuse things even more).
> 
> Reported-by: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit 0fa3ecd87848c9c93c2c828ef4c3a8ca36ce46c7)
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  fs/inode.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 5c1138e9cac0..797b4cb3d20b 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2008,8 +2008,14 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  	inode->i_uid = current_fsuid();
>  	if (dir && dir->i_mode & S_ISGID) {
>  		inode->i_gid = dir->i_gid;
> +
> +		/* Directories are special, and always inherit S_ISGID */
>  		if (S_ISDIR(mode))
>  			mode |= S_ISGID;
> +		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> +			 !in_group_p(inode->i_gid) &&
> +			 !capable_wrt_inode_uidgid(dir, CAP_FSETID))
> +			mode &= ~S_ISGID;
>  	} else
>  		inode->i_gid = current_fsgid();
>  	inode->i_mode = mode;
>
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 5c1138e9cac0..797b4cb3d20b 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2008,8 +2008,14 @@  void inode_init_owner(struct inode *inode, const struct inode *dir,
 	inode->i_uid = current_fsuid();
 	if (dir && dir->i_mode & S_ISGID) {
 		inode->i_gid = dir->i_gid;
+
+		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
+		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
+			 !in_group_p(inode->i_gid) &&
+			 !capable_wrt_inode_uidgid(dir, CAP_FSETID))
+			mode &= ~S_ISGID;
 	} else
 		inode->i_gid = current_fsgid();
 	inode->i_mode = mode;