Quantcast

[PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Ralph Böhme-2
Hi!

Attached is a patch for bug
https://bugzilla.samba.org/show_bug.cgi?id=12562

The fix for bug #12181 included a change that should ensure filesystem
permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
"acl_xattr:ignore system acls = yes".

At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
contain executable rights files. This should really by "create mask = 0777"
instead.

Please review & push if happy. Thanks!

Cheerio!
-slow

bug12562-master.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Uri Simchoni-4
On 02/06/2017 02:19 PM, Ralph Böhme wrote:

> Hi!
>
> Attached is a patch for bug
> https://bugzilla.samba.org/show_bug.cgi?id=12562
>
> The fix for bug #12181 included a change that should ensure filesystem
> permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> "acl_xattr:ignore system acls = yes".
>
> At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> contain executable rights files. This should really by "create mask = 0777"
> instead.
>
> Please review & push if happy. Thanks!
>
> Cheerio!
> -slow
>
Well, what if I want files created to be 0666? Is there a way to make
the create mask 0777 only if the current create mask is not
world-writable (i.e. only as a fix for an incompatible conf)?

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Ralph Böhme-2
On Mon, Feb 06, 2017 at 02:47:08PM +0200, Uri Simchoni wrote:

> On 02/06/2017 02:19 PM, Ralph Böhme wrote:
> > Hi!
> >
> > Attached is a patch for bug
> > https://bugzilla.samba.org/show_bug.cgi?id=12562
> >
> > The fix for bug #12181 included a change that should ensure filesystem
> > permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> > "acl_xattr:ignore system acls = yes".
> >
> > At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> > contain executable rights files. This should really by "create mask = 0777"
> > instead.
> >
> > Please review & push if happy. Thanks!
> >
> > Cheerio!
> > -slow
> >
> Well, what if I want files created to be 0666?

huh, why would you? You've explicitly requested

  acl_xattr:ignore system acls = yes

whose behaviour is

  When set to yes, a best effort mapping from/to the POSIX ACL layer will not be
  done by this module.

I know it says "POSIX ACL", but you can't seperate the POSIX mode from the ACL
from a functional perspective. We must ensure filesytem permissions are
completely open and permission checking is based entirely on the ACL blob from
the xattr, not on some unpredictable mix of blob and fs.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Uri Simchoni-4
On 02/06/2017 03:04 PM, Ralph Böhme wrote:

> On Mon, Feb 06, 2017 at 02:47:08PM +0200, Uri Simchoni wrote:
>> On 02/06/2017 02:19 PM, Ralph Böhme wrote:
>>> Hi!
>>>
>>> Attached is a patch for bug
>>> https://bugzilla.samba.org/show_bug.cgi?id=12562
>>>
>>> The fix for bug #12181 included a change that should ensure filesystem
>>> permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
>>> "acl_xattr:ignore system acls = yes".
>>>
>>> At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
>>> contain executable rights files. This should really by "create mask = 0777"
>>> instead.
>>>
>>> Please review & push if happy. Thanks!
>>>
>>> Cheerio!
>>> -slow
>>>
>> Well, what if I want files created to be 0666?
>
> huh, why would you? You've explicitly requested
>
>   acl_xattr:ignore system acls = yes
>
> whose behaviour is
>
>   When set to yes, a best effort mapping from/to the POSIX ACL layer will not be
>   done by this module.
>
> I know it says "POSIX ACL", but you can't seperate the POSIX mode from the ACL
> from a functional perspective. We must ensure filesytem permissions are
> completely open and permission checking is based entirely on the ACL blob from
> the xattr, not on some unpredictable mix of blob and fs.
>
> Cheerio!
> -slow
>

I just think 0777 increases the attack surface if the admin doesn't wish
files stored on that share to be locally executed, so there has to be a
way to avoid x bit.

My thinking is that if a file is world-readable and world-writable,
there are no restrictions imposed by POSIX.

Uri.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Uri Simchoni-4
On 02/06/2017 08:30 PM, Uri Simchoni wrote:

>>> Well, what if I want files created to be 0666?
>>
>> huh, why would you? You've explicitly requested
>>
>>   acl_xattr:ignore system acls = yes
>>
>> whose behaviour is
>>
>>   When set to yes, a best effort mapping from/to the POSIX ACL layer will not be
>>   done by this module.
>>
>> I know it says "POSIX ACL", but you can't seperate the POSIX mode from the ACL
>> from a functional perspective. We must ensure filesytem permissions are
>> completely open and permission checking is based entirely on the ACL blob from
>> the xattr, not on some unpredictable mix of blob and fs.
>>
>> Cheerio!
>> -slow
>>
>
> I just think 0777 increases the attack surface if the admin doesn't wish
> files stored on that share to be locally executed, so there has to be a
> way to avoid x bit.
>
> My thinking is that if a file is world-readable and world-writable,
> there are no restrictions imposed by POSIX.
>
> Uri.
>
... so the idea is that if I explicitly set create mask to 0666, it
doesn't become 0777.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Jeremy Allison
In reply to this post by Ralph Böhme-2
On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:

> Hi!
>
> Attached is a patch for bug
> https://bugzilla.samba.org/show_bug.cgi?id=12562
>
> The fix for bug #12181 included a change that should ensure filesystem
> permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> "acl_xattr:ignore system acls = yes".
>
> At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> contain executable rights files. This should really by "create mask = 0777"
> instead.
>
> Please review & push if happy. Thanks!

Hi Ralph,

Can you explain the customer scenario that instigated
this fix ?

It's *probably* right, but I think Uri is asking the
right questions about defauling files to 'x' access
and I want to understand the exact failure case before
I OK this :-).

Cheers,

        Jeremy.


> From f49942e3eb0a2cb67d461cdb3d670fc3cfb51059 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Mon, 6 Feb 2017 12:47:41 +0100
> Subject: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if
>  ignore_system_acls is set
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12562
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/modules/vfs_acl_tdb.c   | 4 ++--
>  source3/modules/vfs_acl_xattr.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_tdb.c b/source3/modules/vfs_acl_tdb.c
> index 174affe..802162a 100644
> --- a/source3/modules/vfs_acl_tdb.c
> +++ b/source3/modules/vfs_acl_tdb.c
> @@ -342,12 +342,12 @@ static int connect_acl_tdb(struct vfs_handle_struct *handle,
>   return -1);
>  
>   if (config->ignore_system_acls) {
> - DBG_NOTICE("setting 'create mask = 0666', "
> + DBG_NOTICE("setting 'create mask = 0777', "
>     "'directory mask = 0777', "
>     "'store dos attributes = yes' and all "
>     "'map ...' options to 'no'\n");
>  
> - lp_do_parameter(SNUM(handle->conn), "create mask", "0666");
> + lp_do_parameter(SNUM(handle->conn), "create mask", "0777");
>   lp_do_parameter(SNUM(handle->conn), "directory mask", "0777");
>   lp_do_parameter(SNUM(handle->conn), "map archive", "no");
>   lp_do_parameter(SNUM(handle->conn), "map hidden", "no");
> diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
> index e1f90ff..9cbc0cc 100644
> --- a/source3/modules/vfs_acl_xattr.c
> +++ b/source3/modules/vfs_acl_xattr.c
> @@ -209,12 +209,12 @@ static int connect_acl_xattr(struct vfs_handle_struct *handle,
>   return -1);
>  
>   if (config->ignore_system_acls) {
> - DBG_NOTICE("setting 'create mask = 0666', "
> + DBG_NOTICE("setting 'create mask = 0777', "
>     "'directory mask = 0777', "
>     "'store dos attributes = yes' and all "
>     "'map ...' options to 'no'\n");
>  
> - lp_do_parameter(SNUM(handle->conn), "create mask", "0666");
> + lp_do_parameter(SNUM(handle->conn), "create mask", "0777");
>   lp_do_parameter(SNUM(handle->conn), "directory mask", "0777");
>   lp_do_parameter(SNUM(handle->conn), "map archive", "no");
>   lp_do_parameter(SNUM(handle->conn), "map hidden", "no");
> --
> 2.9.3
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Jeremy Allison
On Thu, Feb 09, 2017 at 11:03:21AM -0800, Jeremy Allison wrote:

> On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:
> > Hi!
> >
> > Attached is a patch for bug
> > https://bugzilla.samba.org/show_bug.cgi?id=12562
> >
> > The fix for bug #12181 included a change that should ensure filesystem
> > permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> > "acl_xattr:ignore system acls = yes".
> >
> > At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> > contain executable rights files. This should really by "create mask = 0777"
> > instead.
> >
> > Please review & push if happy. Thanks!
>
> Hi Ralph,
>
> Can you explain the customer scenario that instigated
> this fix ?
>
> It's *probably* right, but I think Uri is asking the
> right questions about defauling files to 'x' access
> and I want to understand the exact failure case before
> I OK this :-).

Ping Ralph, I'd love to get this sorted asap.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Ralph Böhme-2
On Fri, Feb 10, 2017 at 11:31:38AM -0800, Jeremy Allison wrote:

> On Thu, Feb 09, 2017 at 11:03:21AM -0800, Jeremy Allison wrote:
> > On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:
> > > Hi!
> > >
> > > Attached is a patch for bug
> > > https://bugzilla.samba.org/show_bug.cgi?id=12562
> > >
> > > The fix for bug #12181 included a change that should ensure filesystem
> > > permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> > > "acl_xattr:ignore system acls = yes".
> > >
> > > At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> > > contain executable rights files. This should really by "create mask = 0777"
> > > instead.
> > >
> > > Please review & push if happy. Thanks!
> >
> > Hi Ralph,
> >
> > Can you explain the customer scenario that instigated
> > this fix ?
> >
> > It's *probably* right, but I think Uri is asking the
> > right questions about defauling files to 'x' access
> > and I want to understand the exact failure case before
> > I OK this :-).
>
> Ping Ralph, I'd love to get this sorted asap.

sorry for keeping you waiting, but I am strill struggling with struggling with
adding name translation caching to catia for handle based VFS ops as requested
by Uri. Part of the large fruit patchset review... I believe I finally have a
version that is works and is safe. More on that later.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
In reply to this post by Jeremy Allison
On Fri, Feb 10, 2017 at 11:31:38AM -0800, Jeremy Allison wrote:

> On Thu, Feb 09, 2017 at 11:03:21AM -0800, Jeremy Allison wrote:
> > On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:
> > > Hi!
> > >
> > > Attached is a patch for bug
> > > https://bugzilla.samba.org/show_bug.cgi?id=12562
> > >
> > > The fix for bug #12181 included a change that should ensure filesystem
> > > permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> > > "acl_xattr:ignore system acls = yes".
> > >
> > > At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> > > contain executable rights files. This should really by "create mask = 0777"
> > > instead.
> > >
> > > Please review & push if happy. Thanks!
> >
> > Hi Ralph,
> >
> > Can you explain the customer scenario that instigated
> > this fix ?
> >
> > It's *probably* right, but I think Uri is asking the
> > right questions about defauling files to 'x' access
> > and I want to understand the exact failure case before
> > I OK this :-).
>
> Ping Ralph, I'd love to get this sorted asap.
well, the customer scenario is "support *some* legacy scenario", I don't have
more details. :)

But I have a rewored patch that should work for all of us: it ensures "create
mask" is *at least* 0666. Customer can set "create mask = 0777" and be happy, we
keep the default 0666, Uri is happy. :)

Ok?

Cheerio!
-slow

bug12562-master.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
You need an else case to set and display the create mask with DBG_NOTICE
if (create_mask & 0666) == 0666.
Or, change the if block to only add 0666 to create_mask and always
format/print/set create_mask_str from create_mask.

On 4/19/2017 5:57 AM, Ralph Böhme via samba-technical wrote:
> - DBG_NOTICE("setting 'create mask = 0666', "
> -   "'directory mask = 0777', "
> + mode_t create_mask = lp_create_mask(SNUM(handle->conn));
> + char *create_mask_str = NULL;
> +
> + if ((create_mask & 0666) != 0666) {


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 09:13:11AM -0400, jim via samba-technical wrote:
> You need an else case to set and display the create mask with DBG_NOTICE if
> (create_mask & 0666) == 0666.

er, why? If the create mask is exactly 0666 I *want* to leave it as it and *not*
change it and *not* log it. But maybe I'm just overly stupid and am still
missing the point. If that's the case, please spell it out in large letters so I
can understand. :)

Thanks!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
Your test should be (create_mask & 0777) != 0666 to see if create_mask
is not exactly 0666.
You only test the 0666 bits which isn't the same thing.
create_mask could be 0767 and pass your test.

On 4/19/2017 10:08 AM, Ralph Böhme wrote:

> On Wed, Apr 19, 2017 at 09:13:11AM -0400, jim via samba-technical wrote:
>> You need an else case to set and display the create mask with DBG_NOTICE if
>> (create_mask & 0666) == 0666.
> er, why? If the create mask is exactly 0666 I *want* to leave it as it and *not*
> change it and *not* log it. But maybe I'm just overly stupid and am still
> missing the point. If that's the case, please spell it out in large letters so I
> can understand. :)
>
> Thanks!
> -slow


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 10:22:39AM -0400, jim wrote:
> Your test should be (create_mask & 0777) != 0666 to see if create_mask is
> not exactly 0666.

oh, no. I just want it to be *at least* 0666, not exactly 0666.

> You only test the 0666 bits which isn't the same thing.
> create_mask could be 0767 and pass your test.

Which is what I want. :)

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 04/19/2017 12:57 PM, Ralph Böhme via samba-technical wrote:

> On Fri, Feb 10, 2017 at 11:31:38AM -0800, Jeremy Allison wrote:
>> On Thu, Feb 09, 2017 at 11:03:21AM -0800, Jeremy Allison wrote:
>>> On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:
>>>> Hi!
>>>>
>>>> Attached is a patch for bug
>>>> https://bugzilla.samba.org/show_bug.cgi?id=12562
>>>>
>>>> The fix for bug #12181 included a change that should ensure filesystem
>>>> permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
>>>> "acl_xattr:ignore system acls = yes".
>>>>
>>>> At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
>>>> contain executable rights files. This should really by "create mask = 0777"
>>>> instead.
>>>>
>>>> Please review & push if happy. Thanks!
>>>
>>> Hi Ralph,
>>>
>>> Can you explain the customer scenario that instigated
>>> this fix ?
>>>
>>> It's *probably* right, but I think Uri is asking the
>>> right questions about defauling files to 'x' access
>>> and I want to understand the exact failure case before
>>> I OK this :-).
>>
>> Ping Ralph, I'd love to get this sorted asap.
>
> well, the customer scenario is "support *some* legacy scenario", I don't have
> more details. :)
>
> But I have a rewored patch that should work for all of us: it ensures "create
> mask" is *at least* 0666. Customer can set "create mask = 0777" and be happy, we
> keep the default 0666, Uri is happy. :)
>
> Ok?
>
> Cheerio!
> -slow
>

Happy :)
RB+ me. I don't have time right now for push logistics, will push later
if none does.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] vfs_acl_xattr|tdb: set create mask to 0777 if ignore_system_acls is set

Samba - samba-technical mailing list
On Thu, Apr 20, 2017 at 11:42:44AM +0300, Uri Simchoni via samba-technical wrote:

> On 04/19/2017 12:57 PM, Ralph Böhme via samba-technical wrote:
> > On Fri, Feb 10, 2017 at 11:31:38AM -0800, Jeremy Allison wrote:
> >> On Thu, Feb 09, 2017 at 11:03:21AM -0800, Jeremy Allison wrote:
> >>> On Mon, Feb 06, 2017 at 01:19:48PM +0100, Ralph Böhme wrote:
> >>>> Hi!
> >>>>
> >>>> Attached is a patch for bug
> >>>> https://bugzilla.samba.org/show_bug.cgi?id=12562
> >>>>
> >>>> The fix for bug #12181 included a change that should ensure filesystem
> >>>> permissions are out of the way when using VFS modules acl_xattr or acl_tdb with
> >>>> "acl_xattr:ignore system acls = yes".
> >>>>
> >>>> At runtime, when the module is loaded, we set "create mask = 0666" which doesn't
> >>>> contain executable rights files. This should really by "create mask = 0777"
> >>>> instead.
> >>>>
> >>>> Please review & push if happy. Thanks!
> >>>
> >>> Hi Ralph,
> >>>
> >>> Can you explain the customer scenario that instigated
> >>> this fix ?
> >>>
> >>> It's *probably* right, but I think Uri is asking the
> >>> right questions about defauling files to 'x' access
> >>> and I want to understand the exact failure case before
> >>> I OK this :-).
> >>
> >> Ping Ralph, I'd love to get this sorted asap.
> >
> > well, the customer scenario is "support *some* legacy scenario", I don't have
> > more details. :)
> >
> > But I have a rewored patch that should work for all of us: it ensures "create
> > mask" is *at least* 0666. Customer can set "create mask = 0777" and be happy, we
> > keep the default 0666, Uri is happy. :)
> >
> > Ok?
> >
> > Cheerio!
> > -slow
> >
>
> Happy :)
> RB+ me. I don't have time right now for push logistics, will push later
> if none does.

thanks, pushed.

-slow

Loading...