From patchwork Thu Dec 10 22:33:14 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 7822671 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 4A36C9FB32 for ; Thu, 10 Dec 2015 22:33:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E5D49203E9 for ; Thu, 10 Dec 2015 22:33:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DAB9C20555 for ; Thu, 10 Dec 2015 22:33:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750835AbbLJWdU (ORCPT ); Thu, 10 Dec 2015 17:33:20 -0500 Received: from mail-pa0-f42.google.com ([209.85.220.42]:34087 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751875AbbLJWdT (ORCPT ); Thu, 10 Dec 2015 17:33:19 -0500 Received: by pacwq6 with SMTP id wq6so54081501pac.1 for ; Thu, 10 Dec 2015 14:33:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:mime-version:content-type :content-disposition; bh=U+6vcGKktnNCYD9iJWH3qAVnudgnHI7k/P2uuZeyndA=; b=ngaBGkp+AHBqYgZl1X2OHoeoNSjA01h3pKHKJ/87fIR7Je7BkEcrGc/aRXE1jCpBHt afiM3oSaqhq0J8soNpk4wX96qdMxb5phchM7rHHgUJz9fw+iTGMGE+xqGrCN/IxlexTU MWRaIvxbeXnbAqjYubmA3z+8jGxMcWjJBMyDs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-type:content-disposition; bh=U+6vcGKktnNCYD9iJWH3qAVnudgnHI7k/P2uuZeyndA=; b=YV5jOV/N2tGQqUlr+9Trri3fUMHzUcze/971X0WbmEX9b5vrPVbBEAQpXDQaZel34H mUPDxnrCcmL/WBOJWgpo7wDJIaVqMXVHJSefIOUfeir3OJg62EQigUIm0d6Qz2TRoroY htoclIeEfUq3qVNnxjJ3P68lccphUakO0sm6B2pyqPZmyJznOhIiQLgk8tZVf69veDtd HUWN79jGXtTrVX4jXdyDYAenbPM2v5lkCFvhyIUf39BNabVdpNXUutZHhBPz4jiNGgoH YqHMxxnY1cbZQLXO5RIAx5b8CxbRMpY2v+rucoimgstj+YSiYZLuZr5fgNRMgq5sf/wy AssQ== X-Gm-Message-State: ALoCoQm0SnpxvpGHachFHand49CmBypslEtIkP/h964p1Rpniw3v1NzU1dAHtbRF9MBoZIUtaojU9wgE4rE1lZ7SZqNLZpYlzQ== X-Received: by 10.66.240.4 with SMTP id vw4mr20410818pac.9.1449786797366; Thu, 10 Dec 2015 14:33:17 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id pb9sm20949376pac.38.2015.12.10.14.33.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Dec 2015 14:33:16 -0800 (PST) Date: Thu, 10 Dec 2015 14:33:14 -0800 From: Kees Cook To: Alexander Viro Cc: Jan Kara , yalin wang , Willy Tarreau , Andrew Morton , linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org Subject: [PATCH v5] fs: clear file privilege bits when mmap writing Message-ID: <20151210223314.GA14512@www.outflux.net> MIME-Version: 1.0 Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. We could clear at open() time, but it's possible things are accidentally opening with O_RDWR and only reading. Better to clear on close and error failures (i.e. an improvement over now, which is not clearing at all). 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 --- v5: - add to f_flags instead, viro - add i_mutex during __fput, jack 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 | 11 +++++++++++ fs/open.c | 2 +- include/uapi/asm-generic/fcntl.h | 4 ++++ mm/memory.c | 5 +++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/fs/file_table.c b/fs/file_table.c index ad17e05ebf95..4a8b0b4553e9 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -191,6 +191,17 @@ 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_flags & O_REMOVEPRIV)) { + mutex_lock(&inode->i_mutex); + file_remove_privs(file); + mutex_unlock(&inode->i_mutex); + } + fsnotify_close(file); /* * The function eventpoll_release() should be the first called diff --git a/fs/open.c b/fs/open.c index b6f1e96a7c0b..89069d16ca80 100644 --- a/fs/open.c +++ b/fs/open.c @@ -895,7 +895,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o op->mode = 0; /* Must never be set by userspace */ - flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC; + flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC & ~O_REMOVEPRIV; /* * O_SYNC is implemented as __O_SYNC|O_DSYNC. As many places only diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h index e063effe0cc1..096c4b3afe6a 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -88,6 +88,10 @@ #define __O_TMPFILE 020000000 #endif +#ifndef O_REMOVEPRIV +#define O_REMOVEPRIV 040000000 +#endif + /* a horrid kludge trying to make sure that this will fail on old kernels */ #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY) #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT) diff --git a/mm/memory.c b/mm/memory.c index c387430f06c3..ad4188a8f279 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2036,6 +2036,11 @@ static inline int wp_page_reuse(struct mm_struct *mm, if (!page_mkwrite) file_update_time(vma->vm_file); + if (unlikely((vma->vm_file->f_flags & O_REMOVEPRIV) == 0)) { + spin_lock(&vma->vm_file->f_lock); + vma->vm_file->f_flags |= O_REMOVEPRIV; + spin_unlock(&vma->vm_file->f_lock); + } } return VM_FAULT_WRITE;