* [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe @ 2017-10-21 13:28 Nicolas Belouin 2017-10-21 14:08 ` [kernel-hardening] " Nick Kralevich 0 siblings, 1 reply; 4+ messages in thread From: Nicolas Belouin @ 2017-10-21 13:28 UTC (permalink / raw) To: Alexander Viro, linux-fsdevel, linux-kernel, linux-api, kernel-hardening Cc: 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@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); u16 count = same->dest_count; struct file *dst_file; loff_t dst_off; -- 2.14.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe 2017-10-21 13:28 [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe Nicolas Belouin @ 2017-10-21 14:08 ` Nick Kralevich 2017-10-21 19:40 ` Nicolas Belouin 0 siblings, 1 reply; 4+ messages in thread From: Nick Kralevich @ 2017-10-21 14:08 UTC (permalink / raw) To: Nicolas Belouin Cc: Alexander Viro, linux-fsdevel, lkml, linux-api, kernel-hardening 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. > u16 count = same->dest_count; > struct file *dst_file; > loff_t dst_off; > -- > 2.14.2 > -- Nick Kralevich | Android Security | nnk@google.com | 650.214.4037 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe 2017-10-21 14:08 ` [kernel-hardening] " Nick Kralevich @ 2017-10-21 19:40 ` Nicolas Belouin 2017-10-26 7:54 ` Andy Lutomirski 0 siblings, 1 reply; 4+ messages in thread From: Nicolas Belouin @ 2017-10-21 19:40 UTC (permalink / raw) To: Nick Kralevich Cc: Alexander Viro, linux-fsdevel, lkml, linux-api, kernel-hardening 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe 2017-10-21 19:40 ` Nicolas Belouin @ 2017-10-26 7:54 ` Andy Lutomirski 0 siblings, 0 replies; 4+ messages in thread From: Andy Lutomirski @ 2017-10-26 7:54 UTC (permalink / raw) To: Nicolas Belouin Cc: Nick Kralevich, Alexander Viro, Linux FS Devel, lkml, Linux API, kernel-hardening@lists.openwall.com 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-26 7:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-21 13:28 [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe Nicolas Belouin 2017-10-21 14:08 ` [kernel-hardening] " Nick Kralevich 2017-10-21 19:40 ` Nicolas Belouin 2017-10-26 7:54 ` Andy Lutomirski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).