From patchwork Tue Dec 8 23:28:18 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 7802781 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 1EB13BEEE1 for ; Tue, 8 Dec 2015 23:28:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 408C120434 for ; Tue, 8 Dec 2015 23:28:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 57FD62042C for ; Tue, 8 Dec 2015 23:28:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751442AbbLHX2X (ORCPT ); Tue, 8 Dec 2015 18:28:23 -0500 Received: from mail-pa0-f52.google.com ([209.85.220.52]:33597 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751214AbbLHX2W (ORCPT ); Tue, 8 Dec 2015 18:28:22 -0500 Received: by pabur14 with SMTP id ur14so19377596pab.0 for ; Tue, 08 Dec 2015 15:28:21 -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=IGrBWroAugB4BuQnHwIN/vnGTtFyKS6/YTZ/ub5hTCU=; b=bQMS/FefT0BPYpbzjmyEGOUdt5T3XKBBUIhyILgY5YjzKdIabAWmiO+YCnnnQwlE7S uRXVLi/RhWi7upX5wREOjziAWAR6mEoUzdMbv7qIIs6UAPIQS+FBHj04Rk9UGZbV5tZd JA35m2l58LKujKVw38Axp7Bgc/RrTx9qzKtBY= 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=IGrBWroAugB4BuQnHwIN/vnGTtFyKS6/YTZ/ub5hTCU=; b=JaCsbb8FLQKZz9S38YjGCEnCc3ZVEwq6ZBUPbBUOfD4In6+oulZmtjd7SP2Iyrha6O LS3d8/lNE6RPvZ81Y5JpN6oXrQHWNnMhM5h6ymejlitoLYnmEwLf0O5BKVeSgVLG8IbQ v+P2Ftk6/AydF341fPnDkkxGJoZoCGvjm03HHhM+1wZ0NWWoE25dOCMFrwxqasAGiqbe yARsIcGxeG/2rJPVWlAmnqbAaGlYVOrpAjw85hGgnJ1z+Oy99zxOlgGBhUb8oOcQTMHy Zkc0ws4DQ7bawU4KgtYmereMdRh60kKbbJGMC0ekf9fHv7JeBge1P+wAgE+FJLkDtPiV R0Ig== X-Gm-Message-State: ALoCoQn9/YoKiYpzQeaWRGoBVYYDK2vp/VznByXNhTmRMwdlCxLo5+bPOrX5TYPGgNhHLc3q1OzDbviluJaKmH88PVq7694z1Q== X-Received: by 10.66.227.231 with SMTP id sd7mr3805031pac.60.1449617301679; Tue, 08 Dec 2015 15:28:21 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id 24sm6972530pfm.75.2015.12.08.15.28.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Dec 2015 15:28:20 -0800 (PST) Date: Tue, 8 Dec 2015 15:28:18 -0800 From: Kees Cook To: Andrew Morton Cc: Jan Kara , yalin wang , Willy Tarreau , "Eric W. Biederman" , "Kirill A. Shutemov" , Oleg Nesterov , Rik van Riel , Chen Gang , Davidlohr Bueso , Andrea Arcangeli , Alexander Viro , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: [PATCH v4] fs: clear file privilege bits when mmap writing Message-ID: <20151208232818.GA29887@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. 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 --- 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;