linux-kernel - [PATCH v4] fs: clear file privilege bits when mmap writing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151208232818.GA29887@www.outflux.net>
Date:	Tue, 8 Dec 2015 15:28:18 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Jan Kara <jack@...e.cz>, yalin wang <yalin.wang2010@...il.com>,
	Willy Tarreau <w@....eu>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Chen Gang <gang.chen.5i5j@...il.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH v4] fs: clear file privilege bits when mmap writing

Normally, when a user can modify a file that has setuid or setgid bits,
those bits are cleared when they are not the file owner or a member
of the group. This is enforced when using write and truncate but not
when writing to a shared mmap on the file. This could allow the file
writer to gain privileges by changing a binary without losing the
setuid/setgid/caps bits.

Changing the bits requires holding inode->i_mutex, so it cannot be done
during the page fault (due to mmap_sem being held during the fault). We
could do this during vm_mmap_pgoff, but that would need coverage in
mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
again.

Instead, detect the need to clear the bits during the page fault, and
actually remove the bits during final fput. Since the file was open for
writing, it wouldn't have been possible to execute it yet.

Signed-off-by: Kees Cook <keescook@...omium.org>
---
Here's another way? I wonder which of these will actually work. I
wish we could reject writes if file_remove_privs() fails.

v4:
- delay removal instead of still needing mmap_sem for mprotect, yalin
v3:
- move outside of mmap_sem for real now, fengguang
- check return code of file_remove_privs, akpm
v2:
- move to mmap from fault handler, jack
---
 fs/file_table.c    | 8 ++++++++
 include/linux/fs.h | 1 +
 mm/memory.c        | 1 +
 3 files changed, 10 insertions(+)

diff --git a/fs/file_table.c b/fs/file_table.c
index ad17e05ebf95..abb537ef4344 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -191,6 +191,14 @@ static void __fput(struct file *file)
 
 	might_sleep();
 
+	/*
+	 * XXX: While avoiding mmap_sem, we've already been written to.
+	 * We must ignore the return value, since we can't reject the
+	 * write.
+	 */
+	if (unlikely(file->f_remove_privs))
+		file_remove_privs(file);
+
 	fsnotify_close(file);
 	/*
 	 * The function eventpoll_release() should be the first called
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3aa514254161..409bd7047e7e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -872,6 +872,7 @@ struct file {
 	struct list_head	f_tfile_llink;
 #endif /* #ifdef CONFIG_EPOLL */
 	struct address_space	*f_mapping;
+	bool			f_remove_privs;
 } __attribute__((aligned(4)));	/* lest something weird decides that 2 is OK */
 
 struct file_handle {
diff --git a/mm/memory.c b/mm/memory.c
index c387430f06c3..08a77e0cf65f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
 
 		if (!page_mkwrite)
 			file_update_time(vma->vm_file);
+		vma->vm_file->f_remove_privs = true;
 	}
 
 	return VM_FAULT_WRITE;
-- 
1.9.1


-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/