[PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Nicolas Belouin]

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 <nicolas-6zwZCx3K5ONGWvitb5QawA@public.gmane.org>
---
 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);
 	u16 count = same->dest_count;
 	struct file *dst_file;
 	loff_t dst_off;
-- 
2.14.2

Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Nick Kralevich]

On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin <nicolas-6zwZCx3K5ONGWvitb5QawA@public.gmane.org> 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 <nicolas-6zwZCx3K5ONGWvitb5QawA@public.gmane.org>
> ---
>  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.

>         u16 count = same->dest_count;
>         struct file *dst_file;
>         loff_t dst_off;
> --
> 2.14.2
>



-- 
Nick Kralevich | Android Security | nnk-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org | 650.214.4037

Re: [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Nicolas Belouin]

On October 21, 2017 4:08:31 PM GMT+02:00, Nick Kralevich <nnk@google.com> wrote:
>On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin <nicolas@belouin.fr>
>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 <nicolas@belouin.fr>
>> ---
>>  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.

>
>>         u16 count = same->dest_count;
>>         struct file *dst_file;
>>         loff_t dst_off;
>> --
>> 2.14.2
>>

Nicolas

Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Andy Lutomirski]

On Sat, Oct 21, 2017 at 12:40 PM, Nicolas Belouin <nicolas@belouin.fr> wrote:
>
>
> On October 21, 2017 4:08:31 PM GMT+02:00, Nick Kralevich <nnk@google.com> wrote:
>>On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin <nicolas@belouin.fr>
>>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 <nicolas@belouin.fr>
>>> ---
>>>  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