From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]:35652 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752059AbdJZHzE (ORCPT ); Thu, 26 Oct 2017 03:55:04 -0400 Received: from mail-it0-f41.google.com (mail-it0-f41.google.com [209.85.214.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 2252C21939 for ; Thu, 26 Oct 2017 07:55:04 +0000 (UTC) Received: by mail-it0-f41.google.com with SMTP id o135so4007346itb.0 for ; Thu, 26 Oct 2017 00:55:04 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20171021132806.18086-1-nicolas@belouin.fr> From: Andy Lutomirski Date: Thu, 26 Oct 2017 00:54:42 -0700 Message-ID: Subject: Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe To: Nicolas Belouin Cc: Nick Kralevich , Alexander Viro , Linux FS Devel , lkml , Linux API , "kernel-hardening@lists.openwall.com" Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sat, Oct 21, 2017 at 12:40 PM, Nicolas Belouin wrote: > > > On October 21, 2017 4:08:31 PM GMT+02:00, Nick Kralevich wrote: >>On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin >>wrote: >>> In its current implementation the check is against CAP_SYS_ADMIN, >>> however this capability is bloated and inapropriate for this use. >>> Indeed the check aims to avoid dedupe against non writable files, >>> falling directly in the use case of CAP_DAC_OVERRIDE. >>> >>> Signed-off-by: Nicolas Belouin >>> --- >>> fs/read_write.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/read_write.c b/fs/read_write.c >>> index f0d4b16873e8..43cc7e84e29e 100644 >>> --- a/fs/read_write.c >>> +++ b/fs/read_write.c >>> @@ -1965,7 +1965,7 @@ int vfs_dedupe_file_range(struct file *file, >>struct file_dedupe_range *same) >>> u64 len; >>> int i; >>> int ret; >>> - bool is_admin = capable(CAP_SYS_ADMIN); >>> + bool is_admin = capable(CAP_SYS_ADMIN) || >>capable(CAP_DAC_OVERRIDE); >> >>Can you please reverse the order of the checks? In particular, on an >>SELinux based system, a capable() call generates an SELinux denial, >>and people often instinctively allow the first operation performed. >>Reordering the elements will ensure that the CAP_DAC_OVERRIDE denial >>(least permissive) is generated first. > > Will do in the v2 of every concerned patch. > That's still a bit wrong because of how audit works. What you really want is: bool have_either_global_cap(int cap1, int cap2); where, if neither cap is available, the audit message references cap1 and not cap2. Ditto for have_either_ns_cap(). --Andy