[Samba] Issue with acl_xattr:ignore system acls in 4.5rc2

[Eric Eastman via samba]

I was testing Samba 4.5rc2 with an existing smb.conf file that I have been
using since Samba 4.1.x and found that I cannot access data in the share on
Windows 2012 (my AD server), a Windows 2008 client or on a Ubuntu 16.04
client. I built a new version of Samba 4.4.5 using the same procedure as I
did for building 4.5rc2 and the 4.4.5 Samba version worked with my smb.conf
file. I then created a very basic smb.conf file and slowly built it up,
and by doing testing between 4.4.5 and 4.5rc2, I finally found the line
that caused the problem. Here is my cut down smb.conf file:

[global]
security = ads
realm = ERIC.LOCAL
workgroup = ERIC
netbios name = C1-GW03-T5
idmap uid = 500-10000000
idmap gid = 500-10000000
winbind use default domain = Yes
winbind nested groups = Yes
map acl inherit = yes
vfs objects = acl_xattr streams_xattr
acl_xattr:ignore system acls = yes
load printers = no
printcap name = /dev/null

[zzz]
path = /zzz
writeable = yes
browseable = yes


The line causing the problem with 4.5rc2 is:
acl_xattr:ignore system acls = yes

For my tests the /zzz directory was created clean each time before starting
the Samba processes with:
# rm -rf /zzz
# mkdir /zzz
# chmod 777 /zzz

The test server is a Ubuntu 14.04 VM using an ext4 file system and a 4.7.0
kernel:
# uname -a
Linux ede-c1-gw03 4.7.0-4.7.0-k #1 SMP Mon Jul 25 10:54:31 EDT 2016 x86_64
x86_64 x86_64 GNU/Linux

I tested that extended attributes worked on the ext4 file system by using
the setfattr command.

My Samba 4.5rc2 the version shows:
# smbd --version
Version 4.5.0rc2

Both the 4.4.5 and 4.5rc2 Samba builds were done with:
$ ./configure --prefix=/usr/keeper --without-systemd --without-lttng
$ make
$ sudo make install

The compiler verion is:
$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4

Using Samba 4.5rc2 and using the Windows 2012 AD, when I tried to map the
zzz export, it pops up an error window that says:

Location is not available
Z:\ is not accessible.
The handle is invalid.

On my Ubuntu 16.04 system, the mount works, but ls -la on the mount point
gives the error:

# mount -v /zzz
mount.cifs kernel mount options: ip=10.14.2.33,unc=\\
ede-c1-gw03.keepertech.com
\zzz,vers=3.0,user=ERIC,,domain=ERIC.LOCAL,pass=********
# ls -la /zzz
ls: reading directory '/zzz': Permission denied
total 0

The Ubuntu client /etc/fstab entry is:
//ede-c1-gw03.XXXX.com/zzz /zzz cifs
rw,username=ERIC,password=XXXX,domain=ERIC.LOCAL,vers=3.0 0 0

The same smb.conf file works fine on Samba 4.4.5 and I have no issues
accessing the the zzz share with Ubuntu or Windows.

On Samba 4.5rc2 if I stop all the Samba processes, comment out the
following line in the smb.conf file:

# acl_xattr:ignore system acls = yes

and restart the Samba processes, I no longer have the error when I mount
the share on Windows of Ubuntu.

I am not sure why this one line is causing the issue with 4.5rc2.

Regards,

Eric
--
To unsubscribe from this list go to the following URL and read the
instructions: https://lists.samba.org/mailman/options/samba

[Ralph Böhme via samba]

Hi Eric,

this change was introduced in
<https://bugzilla.samba.org/show_bug.cgi?id=12028>

Before explaining the gory details, one question: why are you setting
this option?

With the patches from the bugreport the behaviour of vfs_acl_xattr
changed when "acl_xattr:ignore system acls" is set to yes for the case
a file or directory lacks the NT ACL xattr (eg when creating files and
directories on the server).

Before the change, we would query the system POSIX permissions and map
them to a NT ACL.

After the change we sythesize a default NT ACL with just

ACL:$uid:ALLOWED/0x0/$mapped_mode
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL

As this severly impacts existing setups, we have three options to
address this:

1. Revert it,
2. Document it, or
3. Do it differently

1. Revert it

Brings back the original problem: not behaving as a Windows server and
in certain situations unexpectedly exposing system POSIX permissions
as described in the above bug.

2. Document it

One could argue that this works as designed, so just add a big note to
the release notes so people are aware of this change. As everybody
reads release notes, there'll be no surprise. :)

When acl_xattr:ignore system acls is set to yes, we now completely
ignore the system POSIX permissions. Since bug 11806 we already ignore
the the system POSIX permissions when setting an NT ACL, we now ignore
them as well when querying them. So this can be seen as improvement
because previously our behaviour was inconsistent.

In some sense "acl_xattr:ignore system acls = yes" works like a
fictional option "acl_xattr:ignore system permissions = yes".

Any filesytem object without NT ACL xattr will get default permissions
matching Windows:

ACL:$uid:ALLOWED/0x0/$mapped_mode
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL

This affects directories created on the server, like in Eric'c case,
or when an SMB client creates a file or directory where the parent
directory has no inheritable ACEs, as described in bug 12028.

3. Do it differently

Implement the fictional option from 2 "acl_xattr:ignore system
permissions" and only ignore system POSIX permissions when querying
permissions if this is set. Sing: one more option before you go, to
the valley below.

*scratching head* Thoughts?

Cheerio!
-slow

[Uri Simchoni via samba]

Ralph,

I think the current behavior is correct (also tested it against Windows
for comparison).

However, if it breaks things for users, we need a way out for them, and
the only way out that's guaranteed to work in all cases is a
compatibility switch - generate the "old" DACL as default, wrong or
surprising as it may be, to maintain backward compatibility. So that's
an additional "acl_xattr:legacy default acl = yes" param IMHO.

Thanks,
Uri.

[Michael Adam via samba]

From reading the original post, I get the impression
that the problem is associated to the share root directory.
There is of cours a chicken-and-egg problem here.

And my wild guess is that this could be fixed with setting
a proper share acl. (use the sharesec command).
Maybe we must also/alternatively put a different xattr-acl for
the share root.

Otherwise I fully agree that the new behaviour seems to be
the correct one to me.

Michael

[Ralph Böhme via samba]

> I think the current behavior is correct (also tested it against Windows
> for comparison).

thanks for confirming.

> However, if it breaks things for users, we need a way out for them, and
> the only way out that's guaranteed to work in all cases is a
> compatibility switch - generate the "old" DACL as default, wrong or
> surprising as it may be, to maintain backward compatibility. So that's
> an additional "acl_xattr:legacy default acl = yes" param IMHO.

well, if we conclude we need a new option that

a) preserves existing behaviour, and

b) allows enabling full Windows compatibility by completely ignoring
system POSIX permissions,

then my suggestion would be

o restore the previous behaviour of "acl_xattr:ignore system acls =
yes", in order not to break exisiting setups, and

o add a new option that would tell us to completely ignore system
POSIX permissions when querying and setting, defaulting to off:

acl_xattr:ignore system permissions = yes | no (default: no)

"acl_xattr:ignore system permissions = yes" implicitly sets

"acl_xattr:ignore system acls = yes"

The name "ignore system permissions" also better reflects the fact
that this option is not only about ACLs, but about system permissions
including POSIX mode.

We should then mark "acl_xattr:ignore system acls" as legacy and
explain the broken behaviour (system permisions ignored when setting,
but sometimes (when the filesystem object lacks an NT ACL xattr) still
used when querying).

*still scratching head* :) Thoughts?

Cheerio!
-slow

[Ralph Böhme via samba]

Hi Michael,

On Thu, Aug 18, 2016 at 11:42:12AM +0200, Michael Adam wrote:
> From reading the original post, I get the impression
> that the problem is associated to the share root directory.

nope. It will happen with just any directory or file created from a
non SMB client, eg mkdir|touch (or NFS, ...) on the server.

It is also associated with files or directories created from Windows
client where the parent directory for some reason lacks inheritable
ACEs (because eg the admin relied on POSIX mode for some basic
permissions).

> There is of cours a chicken-and-egg problem here.
>
> And my wild guess is that this could be fixed with setting
> a proper share acl. (use the sharesec command).
> Maybe we must also/alternatively put a different xattr-acl for
> the share root.

Just adding inheritable (NT ACL xattr) ACEs to the share root
directory indeed fixes the problem for SMB clients, but not for stuff
created on the server.

Cheerio!
-slow

[Michael Adam via samba]

On 2016-08-18 at 12:40 +0200, Ralph Böhme wrote:
> Hi Michael,
>
> On Thu, Aug 18, 2016 at 11:42:12AM +0200, Michael Adam wrote:
> > From reading the original post, I get the impression
> > that the problem is associated to the share root directory.
>
> nope. It will happen with just any directory or file created from a
> non SMB client, eg mkdir|touch (or NFS, ...) on the server.

Sure. But that is works-as-designed, imho.
Creating should only be done via smb in this case.
All else is invalid for this config and expected
to disturb behavior.

The base directory has to be created externally, though,
and in order to give initial access, there needs to
be an ACL (xattr/share) on the base dir.
Hence this is different.

> It is also associated with files or directories created from Windows
> client where the parent directory for some reason lacks inheritable
> ACEs (because eg the admin relied on POSIX mode for some basic
> permissions).

Relying on posix mode is plain wrong usage when
'ignore system acls = yes'.

> > There is of cours a chicken-and-egg problem here.
> >
> > And my wild guess is that this could be fixed with setting
> > a proper share acl. (use the sharesec command).
> > Maybe we must also/alternatively put a different xattr-acl for
> > the share root.
>
> Just adding inheritable (NT ACL xattr) ACEs to the share root
> directory indeed fixes the problem for SMB clients, but not for stuff
> created on the server.

I can only repeat: Creating stuff with other means than SMB
is invalid usage when 'inherit system acls = true', imho.

We may discuss a 'strict' and 'non-strict' mode to that.
In order to offer backwards compatibility to old broken
or at least inconsistent behavior?

Cheers - Michael

[Ralph Böhme via samba]

On Thu, Aug 18, 2016 at 12:59:15PM +0200, Michael Adam wrote:
> On 2016-08-18 at 12:40 +0200, Ralph Böhme wrote:
> > Hi Michael,
> >
> > On Thu, Aug 18, 2016 at 11:42:12AM +0200, Michael Adam wrote:
> > > From reading the original post, I get the impression
> > > that the problem is associated to the share root directory.
> >
> > nope. It will happen with just any directory or file created from a
> > non SMB client, eg mkdir|touch (or NFS, ...) on the server.
>
> Sure. But that is works-as-designed, imho.

I agree, but it did work that way and with 4.5 now
workinging-as-designed we will break existing setups.

> Creating should only be done via smb in this case.
> All else is invalid for this config and expected
> to disturb behavior.

Well yes, we even document it in the manpage:

If you only access the data via Samba you might set this to yes to
achieve better NT ACL compatibility.

> > It is also associated with files or directories created from Windows
> > client where the parent directory for some reason lacks inheritable
> > ACEs (because eg the admin relied on POSIX mode for some basic
> > permissions).
>
> Relying on posix mode is plain wrong usage when
> 'ignore system acls = yes'.

agreed, but it worked with previous versions, it will suddenly stop
working with 4.5.

> > > There is of cours a chicken-and-egg problem here.
> > >
> > > And my wild guess is that this could be fixed with setting
> > > a proper share acl. (use the sharesec command).
> > > Maybe we must also/alternatively put a different xattr-acl for
> > > the share root.
> >
> > Just adding inheritable (NT ACL xattr) ACEs to the share root
> > directory indeed fixes the problem for SMB clients, but not for stuff
> > created on the server.
>
> I can only repeat: Creating stuff with other means than SMB
> is invalid usage when 'inherit system acls = true', imho.

indeed.

So leave as is and add better documentation of this change to the
release notes?

[Michael Adam via samba]

That sounds reasonable to me.
It's essentially the strict and non-strict modes
that I mentioned, just better.

Michael

[Eric Eastman via samba]

Hi Ralph,

>> The line causing the problem with 4.5rc2 is:
>> acl_xattr:ignore system acls = yes
>
> this change was introduced in
> <https://bugzilla.samba.org/show_bug.cgi?id=12028>
>
> Before explaining the gory details, one question: why are you setting
> this option?

I am setting this option per the vfs_acl_xattr.8 man page
recommendations. Using a Windows system I setup a Home directory under
the root directory, /zzz/Home in this case, and that directory gets
the needed NT ACLs when it is created. Not having access to /zzz on
my Windows AD was a surprise when I started testing 4.5, as this has
worked for me since 4.1.x. Other then creating /zzz, all access to the
/zzz/Home tree is done using shared SMB mounts from Linux and Windows.

> As this severly impacts existing setups, we have three options to
> address this:
>
> 1. Revert it,
> 2. Document it, or
> 3. Do it differently
>
> 1. Revert it
>
> Brings back the original problem: not behaving as a Windows server and
> in certain situations unexpectedly exposing system POSIX permissions
> as described in the above bug.

I would not revert it, but per other recommendations, having a legacy
option would be nice.

> 2. Document it
>
> One could argue that this works as designed, so just add a big note to
> the release notes so people are aware of this change. As everybody
> reads release notes, there'll be no surprise. :)

This would have been very helpful. I read the release notes before
starting my 4.5 testing, and re-read them as soon as I hit this issue.
A note in the man page that states how this function changed in 4.5
would also be helpful.

> 3. Do it differently

Now that I understand what is going on, I have no problems with the
change. It was just a surprise that cost me some time to figure it
out.

Thank you for the detailed information.

Eric

[Ralph Böhme via samba]

Hi Eric,

On Thu, Aug 18, 2016 at 07:57:36AM -0600, Eric Eastman wrote:
> >> The line causing the problem with 4.5rc2 is:
> >> acl_xattr:ignore system acls = yes
> >
> > this change was introduced in
> > <https://bugzilla.samba.org/show_bug.cgi?id=12028>
> >
> > Before explaining the gory details, one question: why are you setting
> > this option?
>
> I am setting this option per the vfs_acl_xattr.8 man page
> recommendations. Using a Windows system I setup a Home directory under
> the root directory, /zzz/Home in this case, and that directory gets
> the needed NT ACLs when it is created. Not having access to /zzz on
> my Windows AD was a surprise when I started testing 4.5, as this has
> worked for me since 4.1.x. Other then creating /zzz, all access to the
> /zzz/Home tree is done using shared SMB mounts from Linux and Windows.

ok, thanks for that.

> > As this severly impacts existing setups, we have three options to
> > address this:
> >
> > 1. Revert it,
> > 2. Document it, or
> > 3. Do it differently
> >
> > 1. Revert it
> >
> > Brings back the original problem: not behaving as a Windows server and
> > in certain situations unexpectedly exposing system POSIX permissions
> > as described in the above bug.
>
> I would not revert it, but per other recommendations, having a legacy
> option would be nice.

Yeah, as much as I'd like to avoid adding a new option, I guess we
have to do something about it, my latest take on this is

acl_xattr:default acl style = [posix|windows]

This parameter determines the type of ACL that is
synthesized in case a file or directory lacks an
security.NTACL xattr.

When set to posix, an ACL will be synthesized based on the
POSIX mode permissions for user, group and others, with an
additional ACE for NT Authority\SYSTEM will full rights..

When set to windows, an ACL is synthesized the same way
Windows does it, only inclusing permissions for the owner
and NT Authority\SYSTEM

The default for this option is posix.

tldr: this reverts behaviour to what it was before #12028 and make the
behaviour introduced by #12028 optional.

Plan? Michael? Uri? Jeremy?

Cheerio!
-slow

[Jeremy Allison via samba]

On Wed, Aug 24, 2016 at 04:06:42PM +0200, Ralph Böhme via samba wrote:
>
> Yeah, as much as I'd like to avoid adding a new option, I guess we
> have to do something about it, my latest take on this is
>
> acl_xattr:default acl style = [posix|windows]
>
> This parameter determines the type of ACL that is
> synthesized in case a file or directory lacks an
> security.NTACL xattr.
>
> When set to posix, an ACL will be synthesized based on the
> POSIX mode permissions for user, group and others, with an
> additional ACE for NT Authority\SYSTEM will full rights..
>
> When set to windows, an ACL is synthesized the same way
> Windows does it, only inclusing permissions for the owner
> and NT Authority\SYSTEM
>
> The default for this option is posix.
>
> tldr: this reverts behaviour to what it was before #12028 and make the
> behaviour introduced by #12028 optional.
>
> Plan? Michael? Uri? Jeremy?

I like this. Puts the tweak in the right place IMHO.

Jeremy.

[Ralph Böhme via samba]

On Thu, Aug 25, 2016 at 12:14:00PM -0700, Jeremy Allison wrote:
> On Wed, Aug 24, 2016 at 04:06:42PM +0200, Ralph Böhme via samba wrote:
> >
> > Yeah, as much as I'd like to avoid adding a new option, I guess we
> > have to do something about it, my latest take on this is
> >
> > acl_xattr:default acl style = [posix|windows]
> >
> > This parameter determines the type of ACL that is
> > synthesized in case a file or directory lacks an
> > security.NTACL xattr.
> >
> > When set to posix, an ACL will be synthesized based on the
> > POSIX mode permissions for user, group and others, with an
> > additional ACE for NT Authority\SYSTEM will full rights..
> >
> > When set to windows, an ACL is synthesized the same way
> > Windows does it, only inclusing permissions for the owner
> > and NT Authority\SYSTEM
> >
> > The default for this option is posix.
> >
> > tldr: this reverts behaviour to what it was before #12028 and make the
> > behaviour introduced by #12028 optional.
> >
> > Plan? Michael? Uri? Jeremy?
>
> I like this. Puts the tweak in the right place IMHO.

ok, final patchset attached. Bonus: it has a test.

Please review & comment & then let's have a final discussion which
default ACL style should be the default, posix or windows. Thanks!

Cheerio!
-slow

[Jeremy Allison via samba]

On Fri, Aug 26, 2016 at 06:33:26PM +0200, Ralph Böhme via samba wrote:
> On Thu, Aug 25, 2016 at 12:14:00PM -0700, Jeremy Allison wrote:
> > On Wed, Aug 24, 2016 at 04:06:42PM +0200, Ralph Böhme via samba wrote:
> > >
> > > Yeah, as much as I'd like to avoid adding a new option, I guess we
> > > have to do something about it, my latest take on this is
> > >
> > > acl_xattr:default acl style = [posix|windows]
> > >
> > > This parameter determines the type of ACL that is
> > > synthesized in case a file or directory lacks an
> > > security.NTACL xattr.
> > >
> > > When set to posix, an ACL will be synthesized based on the
> > > POSIX mode permissions for user, group and others, with an
> > > additional ACE for NT Authority\SYSTEM will full rights..
> > >
> > > When set to windows, an ACL is synthesized the same way
> > > Windows does it, only inclusing permissions for the owner
> > > and NT Authority\SYSTEM
> > >
> > > The default for this option is posix.
> > >
> > > tldr: this reverts behaviour to what it was before #12028 and make the
> > > behaviour introduced by #12028 optional.
> > >
> > > Plan? Michael? Uri? Jeremy?
> >
> > I like this. Puts the tweak in the right place IMHO.
>
> ok, final patchset attached. Bonus: it has a test.

ENOPATCH :-).

[Ralph Böhme via samba]

*gnah* :)

Cheerio!
-slow

[Jeremy Allison via samba]

On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote:
>
> Cheerio!
> -slow

Still reviewing this - but a few things that will need changing:

When adding the validate_nt_acl_blob() function in
[PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function

this makes some of the existing function names in debug statements
incorrect.

Eg. validate_nt_acl_blob() has debug statements:

688 DEBUG(10, ("get_nt_acl_internal: ACL blob revision "
689 "mismatch (%u) for file %s\n",
690 (unsigned int)hash_type,
691 smb_fname->base_name));
692 TALLOC_FREE(psd_blob);
693 return NT_STATUS_OK;
694 }
695
696 /* determine which type of xattr we got */
697 if (hash_type != XATTR_SD_HASH_TYPE_SHA256) {
698 DEBUG(10, ("get_nt_acl_internal: ACL blob hash type "
699 "(%u) unexpected for file %s\n",
700 (unsigned int)hash_type,
701 smb_fname->base_name));
702 TALLOC_FREE(psd_blob);

768 if (!NT_STATUS_IS_OK(status)) {
769 DEBUG(10, ("get_nt_acl_internal: get_next_acl for file %s "
770 "returned %s\n",
771 smb_fname->base_name,
772 nt_errstr(status)));
773 goto fail;

784 if (memcmp(&hash[0], &hash_tmp[0], XATTR_SD_HASH_SIZE) == 0) {
785 /* Hash matches, return blob sd. */
786 DEBUG(10, ("get_nt_acl_internal: blob hash "
787 "matches for file %s\n",
788 smb_fname->base_name ));
789 *ppsd = psd_blob;

794 DEBUG(10, ("get_nt_acl_internal: blob hash "
795 "does not match for file %s - returning "
796 "file system SD mapping.\n",
797 smb_fname->base_name ));
798
799 if (DEBUGLEVEL >= 10) {
800 DEBUG(10,("get_nt_acl_internal: acl for blob hash for %s is:\n",
801 smb_fname->base_name ));
802 NDR_PRINT_DEBUG(security_descriptor, psd_fs);

These are gonna need fixing to remove the function names
from the debugs (they are automatically added now).

Still reviewing...

> From dd4665a7930fad64ff649110cb087b69b08464f4 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:04:24 +0200
> Subject: [PATCH 01/12] Revert "vfs_acl_xattr: objects without NT ACL xattr"
>
> This reverts commit 961c4b591bb102751079d9cc92d7aa1c37f1958c.
>
> Subsequent commits will add the same functionality as an optional
> feature.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 43 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 2fda938e..a287945 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -379,10 +379,12 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> gid_to_sid(&group_sid, psbuf->st_ex_gid);
>
> /*
> - * We provide 2 ACEs:
> - * - Owner
> - * - NT System
> - */
> + We provide up to 4 ACEs
> + - Owner
> + - Group
> + - Everyone
> + - NT System
> + */
>
> if (mode & S_IRUSR) {
> if (mode & S_IWUSR) {
> @@ -402,6 +404,39 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> 0);
> idx++;
>
> + access_mask = 0;
> + if (mode & S_IRGRP) {
> + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
> + }
> + if (mode & S_IWGRP) {
> + /* note that delete is not granted - this matches posix behaviour */
> + access_mask |= SEC_RIGHTS_FILE_WRITE;
> + }
> + if (access_mask) {
> + init_sec_ace(&aces[idx],
> + &group_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + access_mask,
> + 0);
> + idx++;
> + }
> +
> + access_mask = 0;
> + if (mode & S_IROTH) {
> + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
> + }
> + if (mode & S_IWOTH) {
> + access_mask |= SEC_RIGHTS_FILE_WRITE;
> + }
> + if (access_mask) {
> + init_sec_ace(&aces[idx],
> + &global_sid_World,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + access_mask,
> + 0);
> + idx++;
> + }
> +
> init_sec_ace(&aces[idx],
> &global_sid_System,
> SEC_ACE_TYPE_ACCESS_ALLOWED,
> --
> 2.7.4
>
>
> From ac9828d7fcc48af51f59f7b271210a6328abed0f Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:08:12 +0200
> Subject: [PATCH 02/12] vfs_acl_common: rename psd to psd_blob in
> get_nt_acl_internal()
>
> This makes it explicit where the SD is originating from. No change in
> behaviour.
>
> This just paves the way for a later change that will simplify the whole
> logic and talloc hierarchy, therefor this also strictly renames the
> occurences after the out label.
>
> Logically, behind the out label, we're dealing with a variable that
> points to what we're going to return, so the name psd_blob is
> misleading, but I'm desperately trying to avoid logic changes in this
> commit and therefor I'm just strictly renaming.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 58 ++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index a287945..1adc875 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -488,7 +488,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> uint8_t sys_acl_hash[XATTR_SD_HASH_SIZE];
> uint8_t hash_tmp[XATTR_SD_HASH_SIZE];
> uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE];
> - struct security_descriptor *psd = NULL;
> + struct security_descriptor *psd_blob = NULL;
> struct security_descriptor *pdesc_next = NULL;
> const struct smb_filename *smb_fname = NULL;
> bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> @@ -509,25 +509,25 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (!NT_STATUS_IS_OK(status)) {
> DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n",
> nt_errstr(status)));
> - psd = NULL;
> + psd_blob = NULL;
> goto out;
> } else {
> - status = parse_acl_blob(&blob, mem_ctx, &psd,
> + status = parse_acl_blob(&blob, mem_ctx, &psd_blob,
> &hash_type, &xattr_version, &hash[0], &sys_acl_hash[0]);
> if (!NT_STATUS_IS_OK(status)) {
> DEBUG(10, ("parse_acl_blob returned %s\n",
> nt_errstr(status)));
> - psd = NULL;
> + psd_blob = NULL;
> goto out;
> }
> }
>
> - /* Ensure we don't leak psd if we don't choose it.
> + /* Ensure we don't leak psd_blob if we don't choose it.
> *
> * We don't allocate it onto frame as it is preferred not to
> * steal from a talloc pool.
> */
> - talloc_steal(frame, psd);
> + talloc_steal(frame, psd_blob);
>
> /* determine which type of xattr we got */
> switch (xattr_version) {
> @@ -550,8 +550,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> "mismatch (%u) for file %s\n",
> (unsigned int)hash_type,
> smb_fname->base_name));
> - TALLOC_FREE(psd);
> - psd = NULL;
> + TALLOC_FREE(psd_blob);
> + psd_blob = NULL;
> goto out;
> }
>
> @@ -561,8 +561,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> "(%u) unexpected for file %s\n",
> (unsigned int)hash_type,
> smb_fname->base_name));
> - TALLOC_FREE(psd);
> - psd = NULL;
> + TALLOC_FREE(psd_blob);
> + psd_blob = NULL;
> goto out;
> }
>
> @@ -645,8 +645,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
>
> status = hash_sd_sha256(pdesc_next, hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(psd);
> - psd = pdesc_next;
> + TALLOC_FREE(psd_blob);
> + psd_blob = pdesc_next;
> goto out;
> }
>
> @@ -670,12 +670,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> NDR_PRINT_DEBUG(security_descriptor, pdesc_next);
> }
>
> - TALLOC_FREE(psd);
> - psd = pdesc_next;
> + TALLOC_FREE(psd_blob);
> + psd_blob = pdesc_next;
> }
> out:
>
> - if (psd == NULL) {
> + if (psd_blob == NULL) {
> /* Get the full underlying sd, as we failed to get the
> * blob for the hash, or the revision/hash type wasn't
> * known */
> @@ -708,10 +708,10 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> * steal from a talloc pool.
> */
> talloc_steal(frame, pdesc_next);
> - psd = pdesc_next;
> + psd_blob = pdesc_next;
> }
>
> - if (psd != pdesc_next) {
> + if (psd_blob != pdesc_next) {
> /* We're returning the blob, throw
> * away the filesystem SD. */
> TALLOC_FREE(pdesc_next);
> @@ -764,20 +764,20 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> status = make_default_filesystem_acl(mem_ctx,
> smb_fname->base_name,
> psbuf,
> - &psd);
> + &psd_blob);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> }
> } else {
> if (is_directory &&
> - !sd_has_inheritable_components(psd,
> + !sd_has_inheritable_components(psd_blob,
> true)) {
> status = add_directory_inheritable_components(
> handle,
> smb_fname->base_name,
> psbuf,
> - psd);
> + psd_blob);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> @@ -787,23 +787,23 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> the ~SEC_DESC_DACL_PROTECTED bit, as ACLs
> can't be inherited in this way under POSIX.
> Remove it for Windows-style ACLs. */
> - psd->type &= ~SEC_DESC_DACL_PROTECTED;
> + psd_blob->type &= ~SEC_DESC_DACL_PROTECTED;
> }
> }
>
> if (!(security_info & SECINFO_OWNER)) {
> - psd->owner_sid = NULL;
> + psd_blob->owner_sid = NULL;
> }
> if (!(security_info & SECINFO_GROUP)) {
> - psd->group_sid = NULL;
> + psd_blob->group_sid = NULL;
> }
> if (!(security_info & SECINFO_DACL)) {
> - psd->type &= ~SEC_DESC_DACL_PRESENT;
> - psd->dacl = NULL;
> + psd_blob->type &= ~SEC_DESC_DACL_PRESENT;
> + psd_blob->dacl = NULL;
> }
> if (!(security_info & SECINFO_SACL)) {
> - psd->type &= ~SEC_DESC_SACL_PRESENT;
> - psd->sacl = NULL;
> + psd_blob->type &= ~SEC_DESC_SACL_PRESENT;
> + psd_blob->sacl = NULL;
> }
>
> TALLOC_FREE(blob.data);
> @@ -811,11 +811,11 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (DEBUGLEVEL >= 10) {
> DEBUG(10,("get_nt_acl_internal: returning acl for %s is:\n",
> smb_fname->base_name ));
> - NDR_PRINT_DEBUG(security_descriptor, psd);
> + NDR_PRINT_DEBUG(security_descriptor, psd_blob);
> }
>
> /* The VFS API is that the ACL is expected to be on mem_ctx */
> - *ppdesc = talloc_move(mem_ctx, &psd);
> + *ppdesc = talloc_move(mem_ctx, &psd_blob);
>
> TALLOC_FREE(frame);
> return NT_STATUS_OK;
> --
> 2.7.4
>
>
> From bf5cd7cb4aac0733c8634b62ad29c128a9b3f451 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:11:24 +0200
> Subject: [PATCH 03/12] vfs_acl_common: rename pdesc_next to psd_fs
>
> In most realistic cases the "next" VFS op will return the permissions
> from the filesystem. This rename makes it explicit where the SD is
> originating from. No change in behaviour.
>
> This just paves the way for a later change that will simplify the whole
> logic and talloc hierarchy.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 1adc875..fb01bf4 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -489,7 +489,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> uint8_t hash_tmp[XATTR_SD_HASH_SIZE];
> uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE];
> struct security_descriptor *psd_blob = NULL;
> - struct security_descriptor *pdesc_next = NULL;
> + struct security_descriptor *psd_fs = NULL;
> const struct smb_filename *smb_fname = NULL;
> bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> ACL_MODULE_NAME,
> @@ -618,13 +618,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> fsp,
> HASH_SECURITY_INFO,
> mem_ctx,
> - &pdesc_next);
> + &psd_fs);
> } else {
> status = SMB_VFS_NEXT_GET_NT_ACL(handle,
> smb_fname,
> HASH_SECURITY_INFO,
> mem_ctx,
> - &pdesc_next);
> + &psd_fs);
> }
>
> if (!NT_STATUS_IS_OK(status)) {
> @@ -636,17 +636,17 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> return status;
> }
>
> - /* Ensure we don't leak psd_next if we don't choose it.
> + /* Ensure we don't leak psd_fs if we don't choose it.
> *
> * We don't allocate it onto frame as it is preferred not to
> * steal from a talloc pool.
> */
> - talloc_steal(frame, pdesc_next);
> + talloc_steal(frame, psd_fs);
>
> - status = hash_sd_sha256(pdesc_next, hash_tmp);
> + status = hash_sd_sha256(psd_fs, hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(psd_blob);
> - psd_blob = pdesc_next;
> + psd_blob = psd_fs;
> goto out;
> }
>
> @@ -667,11 +667,11 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (DEBUGLEVEL >= 10) {
> DEBUG(10,("get_nt_acl_internal: acl for blob hash for %s is:\n",
> smb_fname->base_name ));
> - NDR_PRINT_DEBUG(security_descriptor, pdesc_next);
> + NDR_PRINT_DEBUG(security_descriptor, psd_fs);
> }
>
> TALLOC_FREE(psd_blob);
> - psd_blob = pdesc_next;
> + psd_blob = psd_fs;
> }
> out:
>
> @@ -684,13 +684,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> fsp,
> security_info,
> mem_ctx,
> - &pdesc_next);
> + &psd_fs);
> } else {
> status = SMB_VFS_NEXT_GET_NT_ACL(handle,
> smb_fname,
> security_info,
> mem_ctx,
> - &pdesc_next);
> + &psd_fs);
> }
>
> if (!NT_STATUS_IS_OK(status)) {
> @@ -702,19 +702,19 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> return status;
> }
>
> - /* Ensure we don't leak psd_next if we don't choose it.
> + /* Ensure we don't leak psd_fs if we don't choose it.
> *
> * We don't allocate it onto frame as it is preferred not to
> * steal from a talloc pool.
> */
> - talloc_steal(frame, pdesc_next);
> - psd_blob = pdesc_next;
> + talloc_steal(frame, psd_fs);
> + psd_blob = psd_fs;
> }
>
> - if (psd_blob != pdesc_next) {
> + if (psd_blob != psd_fs) {
> /* We're returning the blob, throw
> * away the filesystem SD. */
> - TALLOC_FREE(pdesc_next);
> + TALLOC_FREE(psd_fs);
> } else {
> SMB_STRUCT_STAT sbuf;
> SMB_STRUCT_STAT *psbuf = &sbuf;
> @@ -760,7 +760,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> is_directory = S_ISDIR(psbuf->st_ex_mode);
>
> if (ignore_file_system_acl) {
> - TALLOC_FREE(pdesc_next);
> + TALLOC_FREE(psd_fs);
> status = make_default_filesystem_acl(mem_ctx,
> smb_fname->base_name,
> psbuf,
> --
> 2.7.4
>
>
> From b77ee317be4b59aaffe13aaddd3262ec64ecf914 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:14:50 +0200
> Subject: [PATCH 04/12] vfs_acl_common: remove redundant NULL assignment
>
> The variables are already set to NULL by TALLOC_FREE.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index fb01bf4..0c40f37 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -551,7 +551,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - psd_blob = NULL;
> goto out;
> }
>
> @@ -562,7 +561,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - psd_blob = NULL;
> goto out;
> }
>
> --
> 2.7.4
>
>
> From f199343ccafc8ff3c620be730d7a320559733018 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 17:07:20 +0200
> Subject: [PATCH 05/12] vfs_acl_common: simplify ACL logic, cleanup and talloc
> hierarchy
>
> No change in behaviour (hopefully! :-). This paves the way for moving
> the ACL blob validation to a helper function in the next commit.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 131 ++++++++++++++++++---------------------
> 1 file changed, 61 insertions(+), 70 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 0c40f37..66c58e7 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -488,6 +488,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> uint8_t sys_acl_hash[XATTR_SD_HASH_SIZE];
> uint8_t hash_tmp[XATTR_SD_HASH_SIZE];
> uint8_t sys_acl_hash_tmp[XATTR_SD_HASH_SIZE];
> + struct security_descriptor *psd = NULL;
> struct security_descriptor *psd_blob = NULL;
> struct security_descriptor *psd_fs = NULL;
> const struct smb_filename *smb_fname = NULL;
> @@ -495,7 +496,9 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> ACL_MODULE_NAME,
> "ignore system acls",
> false);
> - TALLOC_CTX *frame = talloc_stackframe();
> + char *sys_acl_blob_description = NULL;
> + DATA_BLOB sys_acl_blob = { 0 };
> + bool psd_is_from_fs = false;
>
> if (fsp && smb_fname_in == NULL) {
> smb_fname = fsp->fsp_name;
> @@ -505,11 +508,10 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
>
> DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name));
>
> - status = get_acl_blob(frame, handle, fsp, smb_fname, &blob);
> + status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob);
> if (!NT_STATUS_IS_OK(status)) {
> DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n",
> nt_errstr(status)));
> - psd_blob = NULL;
> goto out;
> } else {
> status = parse_acl_blob(&blob, mem_ctx, &psd_blob,
> @@ -517,17 +519,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (!NT_STATUS_IS_OK(status)) {
> DEBUG(10, ("parse_acl_blob returned %s\n",
> nt_errstr(status)));
> - psd_blob = NULL;
> + TALLOC_FREE(blob.data);
> goto out;
> }
> }
>
> - /* Ensure we don't leak psd_blob if we don't choose it.
> - *
> - * We don't allocate it onto frame as it is preferred not to
> - * steal from a talloc pool.
> - */
> - talloc_steal(frame, psd_blob);
> + TALLOC_FREE(blob.data);
>
> /* determine which type of xattr we got */
> switch (xattr_version) {
> @@ -537,10 +534,14 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> * require confirmation of the hash. In particular,
> * the NTVFS file server uses version 1, but
> * 'samba-tool ntacl' can set these as well */
> + psd = psd_blob;
> + psd_blob = NULL;
> goto out;
> case 3:
> case 4:
> if (ignore_file_system_acl) {
> + psd = psd_blob;
> + psd_blob = NULL;
> goto out;
> }
>
> @@ -569,20 +570,18 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> case 4:
> {
> int ret;
> - char *sys_acl_blob_description;
> - DATA_BLOB sys_acl_blob;
> if (fsp) {
> /* Get the full underlying sd, then hash. */
> ret = SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FD(handle,
> fsp,
> - frame,
> + mem_ctx,
> &sys_acl_blob_description,
> &sys_acl_blob);
> } else {
> /* Get the full underlying sd, then hash. */
> ret = SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FILE(handle,
> smb_fname->base_name,
> - frame,
> + mem_ctx,
> &sys_acl_blob_description,
> &sys_acl_blob);
> }
> @@ -592,16 +591,20 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (ret == 0) {
> status = hash_blob_sha256(sys_acl_blob, sys_acl_hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
>
> + TALLOC_FREE(sys_acl_blob_description);
> + TALLOC_FREE(sys_acl_blob.data);
> +
> if (memcmp(&sys_acl_hash[0], &sys_acl_hash_tmp[0],
> XATTR_SD_HASH_SIZE) == 0) {
> /* Hash matches, return blob sd. */
> DEBUG(10, ("get_nt_acl_internal: blob hash "
> "matches for file %s\n",
> smb_fname->base_name ));
> + psd = psd_blob;
> + psd_blob = NULL;
> goto out;
> }
> }
> @@ -630,21 +633,15 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> "returned %s\n",
> smb_fname->base_name,
> nt_errstr(status)));
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
>
> - /* Ensure we don't leak psd_fs if we don't choose it.
> - *
> - * We don't allocate it onto frame as it is preferred not to
> - * steal from a talloc pool.
> - */
> - talloc_steal(frame, psd_fs);
> -
> status = hash_sd_sha256(psd_fs, hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(psd_blob);
> - psd_blob = psd_fs;
> + psd = psd_fs;
> + psd_fs = NULL;
> + psd_is_from_fs = true;
> goto out;
> }
>
> @@ -653,6 +650,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> DEBUG(10, ("get_nt_acl_internal: blob hash "
> "matches for file %s\n",
> smb_fname->base_name ));
> + psd = psd_blob;
> + psd_blob = NULL;
> goto out;
> }
>
> @@ -669,11 +668,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> }
>
> TALLOC_FREE(psd_blob);
> - psd_blob = psd_fs;
> + psd = psd_fs;
> + psd_fs = NULL;
> + psd_is_from_fs = true;
> }
> - out:
>
> - if (psd_blob == NULL) {
> +out:
> + if (psd == NULL) {
> /* Get the full underlying sd, as we failed to get the
> * blob for the hash, or the revision/hash type wasn't
> * known */
> @@ -682,13 +683,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> fsp,
> security_info,
> mem_ctx,
> - &psd_fs);
> + &psd);
> } else {
> status = SMB_VFS_NEXT_GET_NT_ACL(handle,
> smb_fname,
> security_info,
> mem_ctx,
> - &psd_fs);
> + &psd);
> }
>
> if (!NT_STATUS_IS_OK(status)) {
> @@ -696,24 +697,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> "returned %s\n",
> smb_fname->base_name,
> nt_errstr(status)));
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
>
> - /* Ensure we don't leak psd_fs if we don't choose it.
> - *
> - * We don't allocate it onto frame as it is preferred not to
> - * steal from a talloc pool.
> - */
> - talloc_steal(frame, psd_fs);
> - psd_blob = psd_fs;
> + psd_is_from_fs = true;
> }
>
> - if (psd_blob != psd_fs) {
> - /* We're returning the blob, throw
> - * away the filesystem SD. */
> - TALLOC_FREE(psd_fs);
> - } else {
> + if (psd_is_from_fs) {
> SMB_STRUCT_STAT sbuf;
> SMB_STRUCT_STAT *psbuf = &sbuf;
> bool is_directory = false;
> @@ -725,8 +715,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> if (fsp) {
> status = vfs_stat_fsp(fsp);
> if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
> psbuf = &fsp->fsp_name->st;
> } else {
> @@ -751,72 +740,74 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> smb_fname,
> &sbuf);
> if (ret == -1) {
> - TALLOC_FREE(frame);
> - return map_nt_error_from_unix(errno);
> + status = map_nt_error_from_unix(errno);
> + goto fail;
> }
> }
> is_directory = S_ISDIR(psbuf->st_ex_mode);
>
> if (ignore_file_system_acl) {
> - TALLOC_FREE(psd_fs);
> + TALLOC_FREE(psd);
> status = make_default_filesystem_acl(mem_ctx,
> smb_fname->base_name,
> psbuf,
> - &psd_blob);
> + &psd);
> if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
> } else {
> if (is_directory &&
> - !sd_has_inheritable_components(psd_blob,
> + !sd_has_inheritable_components(psd,
> true)) {
> status = add_directory_inheritable_components(
> handle,
> smb_fname->base_name,
> psbuf,
> - psd_blob);
> + psd);
> if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(frame);
> - return status;
> + goto fail;
> }
> }
> /* The underlying POSIX module always sets
> the ~SEC_DESC_DACL_PROTECTED bit, as ACLs
> can't be inherited in this way under POSIX.
> Remove it for Windows-style ACLs. */
> - psd_blob->type &= ~SEC_DESC_DACL_PROTECTED;
> + psd->type &= ~SEC_DESC_DACL_PROTECTED;
> }
> }
>
> if (!(security_info & SECINFO_OWNER)) {
> - psd_blob->owner_sid = NULL;
> + psd->owner_sid = NULL;
> }
> if (!(security_info & SECINFO_GROUP)) {
> - psd_blob->group_sid = NULL;
> + psd->group_sid = NULL;
> }
> if (!(security_info & SECINFO_DACL)) {
> - psd_blob->type &= ~SEC_DESC_DACL_PRESENT;
> - psd_blob->dacl = NULL;
> + psd->type &= ~SEC_DESC_DACL_PRESENT;
> + psd->dacl = NULL;
> }
> if (!(security_info & SECINFO_SACL)) {
> - psd_blob->type &= ~SEC_DESC_SACL_PRESENT;
> - psd_blob->sacl = NULL;
> + psd->type &= ~SEC_DESC_SACL_PRESENT;
> + psd->sacl = NULL;
> }
>
> - TALLOC_FREE(blob.data);
> -
> if (DEBUGLEVEL >= 10) {
> DEBUG(10,("get_nt_acl_internal: returning acl for %s is:\n",
> smb_fname->base_name ));
> - NDR_PRINT_DEBUG(security_descriptor, psd_blob);
> + NDR_PRINT_DEBUG(security_descriptor, psd);
> }
>
> - /* The VFS API is that the ACL is expected to be on mem_ctx */
> - *ppdesc = talloc_move(mem_ctx, &psd_blob);
> + *ppdesc = psd;
>
> - TALLOC_FREE(frame);
> return NT_STATUS_OK;
> +
> +fail:
> + TALLOC_FREE(psd);
> + TALLOC_FREE(psd_blob);
> + TALLOC_FREE(psd_fs);
> + TALLOC_FREE(sys_acl_blob_description);
> + TALLOC_FREE(sys_acl_blob.data);
> + return status;
> }
>
> /*********************************************************************
> --
> 2.7.4
>
>
> From a063153eeb506a018c84a34801eaddcc416eced9 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 22:32:57 +0200
> Subject: [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a
> helper function
>
> No change in behaviour.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 177 +++++++++++++++++++++++++--------------
> 1 file changed, 112 insertions(+), 65 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 66c58e7..2cf5012 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -467,20 +467,32 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> return NT_STATUS_OK;
> }
>
> -/*******************************************************************
> - Pull a DATA_BLOB from an xattr given a pathname.
> - If the hash doesn't match, or doesn't exist - return the underlying
> - filesystem sd.
> -*******************************************************************/
> -
> -static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> - files_struct *fsp,
> - const struct smb_filename *smb_fname_in,
> - uint32_t security_info,
> - TALLOC_CTX *mem_ctx,
> - struct security_descriptor **ppdesc)
> +/**
> + * Validate an ACL blob
> + *
> + * This validates an ACL blob against the underlying filesystem ACL. If this
> + * function returns NT_STATUS_OK ppsd can be
> + *
> + * 1. the ACL from the blob (psd_from_fs=false), or
> + * 2. the ACL from the fs (psd_from_fs=true), or
> + * 3. NULL (!)
> + *
> + * If the return value is anything else then NT_STATUS_OK, ppsd is set to NULL
> + * and psd_from_fs set to false.
> + *
> + * Returning the underlying filesystem ACL in case no. 2 is really just an
> + * optimisation, because some validations have to fetch the filesytem ACL as
> + * part of the validation, so we already have it available and callers might
> + * need it as well.
> + **/
> +static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx,
> + vfs_handle_struct *handle,
> + files_struct *fsp,
> + const struct smb_filename *smb_fname,
> + DATA_BLOB *blob,
> + struct security_descriptor **ppsd,
> + bool *psd_is_from_fs)
> {
> - DATA_BLOB blob = data_blob_null;
> NTSTATUS status;
> uint16_t hash_type = XATTR_SD_HASH_TYPE_NONE;
> uint16_t xattr_version = 0;
> @@ -491,41 +503,28 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> struct security_descriptor *psd = NULL;
> struct security_descriptor *psd_blob = NULL;
> struct security_descriptor *psd_fs = NULL;
> - const struct smb_filename *smb_fname = NULL;
> + char *sys_acl_blob_description = NULL;
> + DATA_BLOB sys_acl_blob = { 0 };
> bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> ACL_MODULE_NAME,
> "ignore system acls",
> false);
> - char *sys_acl_blob_description = NULL;
> - DATA_BLOB sys_acl_blob = { 0 };
> - bool psd_is_from_fs = false;
>
> - if (fsp && smb_fname_in == NULL) {
> - smb_fname = fsp->fsp_name;
> - } else {
> - smb_fname = smb_fname_in;
> - }
> + *ppsd = NULL;
> + *psd_is_from_fs = false;
>
> - DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name));
> -
> - status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob);
> + status = parse_acl_blob(blob,
> + mem_ctx,
> + &psd_blob,
> + &hash_type,
> + &xattr_version,
> + &hash[0],
> + &sys_acl_hash[0]);
> if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("get_nt_acl_internal: get_acl_blob returned %s\n",
> - nt_errstr(status)));
> - goto out;
> - } else {
> - status = parse_acl_blob(&blob, mem_ctx, &psd_blob,
> - &hash_type, &xattr_version, &hash[0], &sys_acl_hash[0]);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("parse_acl_blob returned %s\n",
> - nt_errstr(status)));
> - TALLOC_FREE(blob.data);
> - goto out;
> - }
> + DBG_DEBUG("parse_acl_blob returned %s\n", nt_errstr(status));
> + goto fail;
> }
>
> - TALLOC_FREE(blob.data);
> -
> /* determine which type of xattr we got */
> switch (xattr_version) {
> case 1:
> @@ -534,15 +533,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> * require confirmation of the hash. In particular,
> * the NTVFS file server uses version 1, but
> * 'samba-tool ntacl' can set these as well */
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> case 3:
> case 4:
> if (ignore_file_system_acl) {
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
>
> break;
> @@ -552,7 +549,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - goto out;
> + return NT_STATUS_OK;
> }
>
> /* determine which type of xattr we got */
> @@ -562,7 +559,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - goto out;
> + return NT_STATUS_OK;
> }
>
> /* determine which type of xattr we got */
> @@ -603,9 +600,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> DEBUG(10, ("get_nt_acl_internal: blob hash "
> "matches for file %s\n",
> smb_fname->base_name ));
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
> }
>
> @@ -639,10 +635,9 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> status = hash_sd_sha256(psd_fs, hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(psd_blob);
> - psd = psd_fs;
> - psd_fs = NULL;
> - psd_is_from_fs = true;
> - goto out;
> + *ppsd = psd_fs;
> + *psd_is_from_fs = true;
> + return NT_STATUS_OK;
> }
>
> if (memcmp(&hash[0], &hash_tmp[0], XATTR_SD_HASH_SIZE) == 0) {
> @@ -650,9 +645,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> DEBUG(10, ("get_nt_acl_internal: blob hash "
> "matches for file %s\n",
> smb_fname->base_name ));
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
>
> /* Hash doesn't match, return underlying sd. */
> @@ -668,12 +662,69 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> }
>
> TALLOC_FREE(psd_blob);
> - psd = psd_fs;
> - psd_fs = NULL;
> - psd_is_from_fs = true;
> + *ppsd = psd_fs;
> + *psd_is_from_fs = true;
> + }
> +
> + return NT_STATUS_OK;
> +
> +fail:
> + TALLOC_FREE(psd);
> + TALLOC_FREE(psd_blob);
> + TALLOC_FREE(psd_fs);
> + TALLOC_FREE(sys_acl_blob_description);
> + TALLOC_FREE(sys_acl_blob.data);
> + return status;
> +}
> +
> +/*******************************************************************
> + Pull a DATA_BLOB from an xattr given a pathname.
> + If the hash doesn't match, or doesn't exist - return the underlying
> + filesystem sd.
> +*******************************************************************/
> +
> +static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> + files_struct *fsp,
> + const struct smb_filename *smb_fname_in,
> + uint32_t security_info,
> + TALLOC_CTX *mem_ctx,
> + struct security_descriptor **ppdesc)
> +{
> + DATA_BLOB blob = data_blob_null;
> + NTSTATUS status;
> + struct security_descriptor *psd = NULL;
> + const struct smb_filename *smb_fname = NULL;
> + bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> + ACL_MODULE_NAME,
> + "ignore system acls",
> + false);
> + bool psd_is_from_fs = false;
> +
> + if (fsp && smb_fname_in == NULL) {
> + smb_fname = fsp->fsp_name;
> + } else {
> + smb_fname = smb_fname_in;
> + }
> +
> + DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name));
> +
> + status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob);
> + if (NT_STATUS_IS_OK(status)) {
> + status = validate_nt_acl_blob(mem_ctx,
> + handle,
> + fsp,
> + smb_fname,
> + &blob,
> + &psd,
> + &psd_is_from_fs);
> + TALLOC_FREE(blob.data);
> + if (!NT_STATUS_IS_OK(status)) {
> + DBG_DEBUG("ACL validation for [%s] failed\n",
> + smb_fname->base_name);
> + goto fail;
> + }
> }
>
> -out:
> if (psd == NULL) {
> /* Get the full underlying sd, as we failed to get the
> * blob for the hash, or the revision/hash type wasn't
> @@ -803,10 +854,6 @@ out:
>
> fail:
> TALLOC_FREE(psd);
> - TALLOC_FREE(psd_blob);
> - TALLOC_FREE(psd_fs);
> - TALLOC_FREE(sys_acl_blob_description);
> - TALLOC_FREE(sys_acl_blob.data);
> return status;
> }
>
> --
> 2.7.4
>
>
> From 9a9795133dfc713dd5008eb8705d5b7f72ae1f91 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:01:17 +0200
> Subject: [PATCH 07/12] vfs_acl_tdb|xattr: use a config handle
>
> Better for performance and a subsequent commit will add one more option
> where this will pay off.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 50 ++++++++++++++++++++++++++++++++--------
> source3/modules/vfs_acl_tdb.c | 7 ++++++
> source3/modules/vfs_acl_xattr.c | 7 ++++++
> 3 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 2cf5012..fe631e3 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -46,6 +46,34 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *handle,
> SECINFO_DACL | \
> SECINFO_SACL)
>
> +struct acl_common_config {
> + bool ignore_system_acls;
> +};
> +
> +static bool init_acl_common_config(vfs_handle_struct *handle)
> +{
> + struct acl_common_config *config = NULL;
> +
> + config = talloc_zero(handle->conn, struct acl_common_config);
> + if (config == NULL) {
> + DBG_ERR("talloc_zero() failed\n");
> + errno = ENOMEM;
> + return false;
> + }
> +
> + config->ignore_system_acls = lp_parm_bool(SNUM(handle->conn),
> + ACL_MODULE_NAME,
> + "ignore system acls",
> + false);
> +
> + SMB_VFS_HANDLE_SET_DATA(handle, config, NULL,
> + struct acl_common_config,
> + return false);
> +
> + return true;
> +}
> +
> +
> /*******************************************************************
> Hash a security descriptor.
> *******************************************************************/
> @@ -505,14 +533,15 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx,
> struct security_descriptor *psd_fs = NULL;
> char *sys_acl_blob_description = NULL;
> DATA_BLOB sys_acl_blob = { 0 };
> - bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> - ACL_MODULE_NAME,
> - "ignore system acls",
> - false);
> + struct acl_common_config *config = NULL;
>
> *ppsd = NULL;
> *psd_is_from_fs = false;
>
> + SMB_VFS_HANDLE_GET_DATA(handle, config,
> + struct acl_common_config,
> + return NT_STATUS_UNSUCCESSFUL);
> +
> status = parse_acl_blob(blob,
> mem_ctx,
> &psd_blob,
> @@ -537,7 +566,7 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx,
> return NT_STATUS_OK;
> case 3:
> case 4:
> - if (ignore_file_system_acl) {
> + if (config->ignore_system_acls) {
> *ppsd = psd_blob;
> return NT_STATUS_OK;
> }
> @@ -694,11 +723,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> NTSTATUS status;
> struct security_descriptor *psd = NULL;
> const struct smb_filename *smb_fname = NULL;
> - bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> - ACL_MODULE_NAME,
> - "ignore system acls",
> - false);
> bool psd_is_from_fs = false;
> + struct acl_common_config *config = NULL;
> +
> + SMB_VFS_HANDLE_GET_DATA(handle, config,
> + struct acl_common_config,
> + return NT_STATUS_UNSUCCESSFUL);
>
> if (fsp && smb_fname_in == NULL) {
> smb_fname = fsp->fsp_name;
> @@ -797,7 +827,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> }
> is_directory = S_ISDIR(psbuf->st_ex_mode);
>
> - if (ignore_file_system_acl) {
> + if (config->ignore_system_acls) {
> TALLOC_FREE(psd);
> status = make_default_filesystem_acl(mem_ctx,
> smb_fname->base_name,
> diff --git a/source3/modules/vfs_acl_tdb.c b/source3/modules/vfs_acl_tdb.c
> index e4c8462..0c92b72 100644
> --- a/source3/modules/vfs_acl_tdb.c
> +++ b/source3/modules/vfs_acl_tdb.c
> @@ -308,6 +308,7 @@ static int connect_acl_tdb(struct vfs_handle_struct *handle,
> const char *user)
> {
> int ret = SMB_VFS_NEXT_CONNECT(handle, service, user);
> + bool ok;
>
> if (ret < 0) {
> return ret;
> @@ -318,6 +319,12 @@ static int connect_acl_tdb(struct vfs_handle_struct *handle,
> return -1;
> }
>
> + ok = init_acl_common_config(handle);
> + if (!ok) {
> + DBG_ERR("init_acl_common_config failed\n");
> + return -1;
> + }
> +
> /* Ensure we have the parameters correct if we're
> * using this module. */
> DEBUG(2,("connect_acl_tdb: setting 'inherit acls = true' "
> diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
> index d311c57..307ab6a 100644
> --- a/source3/modules/vfs_acl_xattr.c
> +++ b/source3/modules/vfs_acl_xattr.c
> @@ -180,11 +180,18 @@ static int connect_acl_xattr(struct vfs_handle_struct *handle,
> const char *user)
> {
> int ret = SMB_VFS_NEXT_CONNECT(handle, service, user);
> + bool ok;
>
> if (ret < 0) {
> return ret;
> }
>
> + ok = init_acl_common_config(handle);
> + if (!ok) {
> + DBG_ERR("init_acl_common_config failed\n");
> + return -1;
> + }
> +
> /* Ensure we have the parameters correct if we're
> * using this module. */
> DEBUG(2,("connect_acl_xattr: setting 'inherit acls = true' "
> --
> 2.7.4
>
>
> From 8a645460b093d8c61a3507ebb6e3337a3873b10a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:30:15 +0200
> Subject: [PATCH 08/12] vfs_acl_common: move stat stuff to a helper function
>
> Will be reused in the next commit when moving the
> make_default_filesystem_acl() stuff to a different place.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 79 ++++++++++++++++++++++++----------------
> 1 file changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index fe631e3..d7fe258 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -706,6 +706,48 @@ fail:
> return status;
> }
>
> +static NTSTATUS stat_fsp_or_smb_fname(vfs_handle_struct *handle,
> + files_struct *fsp,
> + const struct smb_filename *smb_fname,
> + SMB_STRUCT_STAT *sbuf,
> + SMB_STRUCT_STAT **psbuf)
> +{
> + NTSTATUS status;
> + int ret;
> +
> + if (fsp) {
> + status = vfs_stat_fsp(fsp);
> + if (!NT_STATUS_IS_OK(status)) {
> + return status;
> + }
> + *psbuf = &fsp->fsp_name->st;
> + } else {
> + /*
> + * https://bugzilla.samba.org/show_bug.cgi?id=11249
> + *
> + * We are currently guaranteed that 'name' here is a
> + * smb_fname->base_name, which *cannot* contain a stream name
> + * (':'). vfs_stat_smb_fname() splits a name into a base name +
> + * stream name, which when we get here we know we've already
> + * done. So we have to call the stat or lstat VFS calls
> + * directly here. Else, a base_name that contains a ':' (from a
> + * demangled name) will get split again.
> + *
> + * FIXME.
> + * This uglyness will go away once smb_fname is fully plumbed
> + * through the VFS.
> + */
> + ret = vfs_stat_smb_basename(handle->conn,
> + smb_fname,
> + sbuf);
> + if (ret == -1) {
> + return map_nt_error_from_unix(errno);
> + }
> + }
> +
> + return NT_STATUS_OK;
> +}
> +
> /*******************************************************************
> Pull a DATA_BLOB from an xattr given a pathname.
> If the hash doesn't match, or doesn't exist - return the underlying
> @@ -793,38 +835,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> * filesystem. If it's a directory, and has no
> * inheritable ACE entries we have to fake them.
> */
> - if (fsp) {
> - status = vfs_stat_fsp(fsp);
> - if (!NT_STATUS_IS_OK(status)) {
> - goto fail;
> - }
> - psbuf = &fsp->fsp_name->st;
> - } else {
> - /*
> - * https://bugzilla.samba.org/show_bug.cgi?id=11249
> - *
> - * We are currently guaranteed that 'name' here is
> - * a smb_fname->base_name, which *cannot* contain
> - * a stream name (':'). vfs_stat_smb_fname() splits
> - * a name into a base name + stream name, which
> - * when we get here we know we've already done.
> - * So we have to call the stat or lstat VFS
> - * calls directly here. Else, a base_name that
> - * contains a ':' (from a demangled name) will
> - * get split again.
> - *
> - * FIXME.
> - * This uglyness will go away once smb_fname
> - * is fully plumbed through the VFS.
> - */
> - int ret = vfs_stat_smb_basename(handle->conn,
> - smb_fname,
> - &sbuf);
> - if (ret == -1) {
> - status = map_nt_error_from_unix(errno);
> - goto fail;
> - }
> +
> + status = stat_fsp_or_smb_fname(handle, fsp, smb_fname,
> + &sbuf, &psbuf);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto fail;
> }
> +
> is_directory = S_ISDIR(psbuf->st_ex_mode);
>
> if (config->ignore_system_acls) {
> --
> 2.7.4
>
>
> From 33980bcae5a907f9b8f67c5852a365bd22afc53c Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:43:47 +0200
> Subject: [PATCH 09/12] vfs_acl_common: check for ignore_system_acls before
> fetching filesystem ACL
>
> If ignore_system_acls is set and we're synthesizing a default ACL, we
> were fetching the filesystem ACL just to free it again. This change
> avoids this.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 99 ++++++++++++++++++++++------------------
> 1 file changed, 55 insertions(+), 44 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index d7fe258..9071775 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -801,35 +801,57 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> /* Get the full underlying sd, as we failed to get the
> * blob for the hash, or the revision/hash type wasn't
> * known */
> - if (fsp) {
> - status = SMB_VFS_NEXT_FGET_NT_ACL(handle,
> - fsp,
> - security_info,
> - mem_ctx,
> - &psd);
> +
> + if (config->ignore_system_acls) {
> + SMB_STRUCT_STAT sbuf;
> + SMB_STRUCT_STAT *psbuf = &sbuf;
> +
> + status = stat_fsp_or_smb_fname(handle, fsp, smb_fname,
> + &sbuf, &psbuf);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto fail;
> + }
> +
> + status = make_default_filesystem_acl(
> + mem_ctx,
> + smb_fname->base_name,
> + psbuf,
> + &psd);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto fail;
> + }
> } else {
> - status = SMB_VFS_NEXT_GET_NT_ACL(handle,
> - smb_fname,
> - security_info,
> - mem_ctx,
> - &psd);
> - }
> + if (fsp) {
> + status = SMB_VFS_NEXT_FGET_NT_ACL(handle,
> + fsp,
> + security_info,
> + mem_ctx,
> + &psd);
> + } else {
> + status = SMB_VFS_NEXT_GET_NT_ACL(handle,
> + smb_fname,
> + security_info,
> + mem_ctx,
> + &psd);
> + }
>
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("get_nt_acl_internal: get_next_acl for file %s "
> - "returned %s\n",
> - smb_fname->base_name,
> - nt_errstr(status)));
> - goto fail;
> - }
> + if (!NT_STATUS_IS_OK(status)) {
> + DBG_DEBUG("get_next_acl for file %s "
> + "returned %s\n",
> + smb_fname->base_name,
> + nt_errstr(status));
> + goto fail;
> + }
>
> - psd_is_from_fs = true;
> + psd_is_from_fs = true;
> + }
> }
>
> if (psd_is_from_fs) {
> SMB_STRUCT_STAT sbuf;
> SMB_STRUCT_STAT *psbuf = &sbuf;
> bool is_directory = false;
> +
> /*
> * We're returning the underlying ACL from the
> * filesystem. If it's a directory, and has no
> @@ -844,34 +866,23 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
>
> is_directory = S_ISDIR(psbuf->st_ex_mode);
>
> - if (config->ignore_system_acls) {
> - TALLOC_FREE(psd);
> - status = make_default_filesystem_acl(mem_ctx,
> - smb_fname->base_name,
> - psbuf,
> - &psd);
> + if (is_directory && !sd_has_inheritable_components(psd, true)) {
> + status = add_directory_inheritable_components(
> + handle,
> + smb_fname->base_name,
> + psbuf,
> + psd);
> if (!NT_STATUS_IS_OK(status)) {
> goto fail;
> }
> - } else {
> - if (is_directory &&
> - !sd_has_inheritable_components(psd,
> - true)) {
> - status = add_directory_inheritable_components(
> - handle,
> - smb_fname->base_name,
> - psbuf,
> - psd);
> - if (!NT_STATUS_IS_OK(status)) {
> - goto fail;
> - }
> - }
> - /* The underlying POSIX module always sets
> - the ~SEC_DESC_DACL_PROTECTED bit, as ACLs
> - can't be inherited in this way under POSIX.
> - Remove it for Windows-style ACLs. */
> - psd->type &= ~SEC_DESC_DACL_PROTECTED;
> }
> +
> + /*
> + * The underlying POSIX module always sets the
> + * ~SEC_DESC_DACL_PROTECTED bit, as ACLs can't be inherited in
> + * this way under POSIX. Remove it for Windows-style ACLs.
> + */
> + psd->type &= ~SEC_DESC_DACL_PROTECTED;
> }
>
> if (!(security_info & SECINFO_OWNER)) {
> --
> 2.7.4
>
>
> From 1dafd89f8ffa4825d8593fe27069b4d8dcb1ae80 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 20:31:00 +0200
> Subject: [PATCH 10/12] vfs_acl_xattr|tdb: add option to control default ACL
> style
>
> Existing behaviour is "posix" style. Next commit will (re)add the
> "windows" style. This commit doesn't change behaviour in any way.
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> docs-xml/manpages/vfs_acl_tdb.8.xml | 25 +++++++++++++++++++
> docs-xml/manpages/vfs_acl_xattr.8.xml | 25 +++++++++++++++++++
> source3/modules/vfs_acl_common.c | 45 +++++++++++++++++++++++++++++++----
> 3 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/docs-xml/manpages/vfs_acl_tdb.8.xml b/docs-xml/manpages/vfs_acl_tdb.8.xml
> index 640cec0..607e344 100644
> --- a/docs-xml/manpages/vfs_acl_tdb.8.xml
> +++ b/docs-xml/manpages/vfs_acl_tdb.8.xml
> @@ -63,6 +63,31 @@
> </para>
> </listitem>
> </varlistentry>
> +
> + <varlistentry>
> + <term>acl_tdb:default acl style = [posix|windows]</term>
> + <listitem>
> + <para>
> + This parameter determines the type of ACL that is synthesized in
> + case a file or directory lacks an
> + <emphasis>security.NTACL</emphasis> xattr.
> + </para>
> + <para>
> + When set to <emphasis>posix</emphasis>, an ACL will be
> + synthesized based on the POSIX mode permissions for user, group
> + and others, with an additional ACE for <emphasis>NT
> + Authority\SYSTEM</emphasis> will full rights.
> + </para>
> + <para>
> + When set to <emphasis>windows</emphasis>, an ACL is synthesized
> + the same way Windows does it, only including permissions for the
> + owner and <emphasis>NT Authority\SYSTEM</emphasis>.
> + </para>
> + <para>
> + The default for this option is <emphasis>posix</emphasis>.
> + </para>
> + </listitem>
> + </varlistentry>
> </variablelist>
>
> </refsect1>
> diff --git a/docs-xml/manpages/vfs_acl_xattr.8.xml b/docs-xml/manpages/vfs_acl_xattr.8.xml
> index 60a1c2d..8da73e0 100644
> --- a/docs-xml/manpages/vfs_acl_xattr.8.xml
> +++ b/docs-xml/manpages/vfs_acl_xattr.8.xml
> @@ -67,6 +67,31 @@
> </para>
> </listitem>
> </varlistentry>
> +
> + <varlistentry>
> + <term>acl_xattr:default acl style = [posix|windows]</term>
> + <listitem>
> + <para>
> + This parameter determines the type of ACL that is synthesized in
> + case a file or directory lacks an
> + <emphasis>security.NTACL</emphasis> xattr.
> + </para>
> + <para>
> + When set to <emphasis>posix</emphasis>, an ACL will be
> + synthesized based on the POSIX mode permissions for user, group
> + and others, with an additional ACE for <emphasis>NT
> + Authority\SYSTEM</emphasis> will full rights.
> + </para>
> + <para>
> + When set to <emphasis>windows</emphasis>, an ACL is synthesized
> + the same way Windows does it, only including permissions for the
> + owner and <emphasis>NT Authority\SYSTEM</emphasis>.
> + </para>
> + <para>
> + The default for this option is <emphasis>posix</emphasis>.
> + </para>
> + </listitem>
> + </varlistentry>
> </variablelist>
>
> </refsect1>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 9071775..919a095 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -46,8 +46,16 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *handle,
> SECINFO_DACL | \
> SECINFO_SACL)
>
> +enum default_acl_style {DEFAULT_ACL_POSIX, DEFAULT_ACL_WINDOWS};
> +
> +static const struct enum_list default_acl_style[] = {
> + {DEFAULT_ACL_POSIX, "posix"},
> + {DEFAULT_ACL_WINDOWS, "windows"}
> +};
> +
> struct acl_common_config {
> bool ignore_system_acls;
> + enum default_acl_style default_acl_style;
> };
>
> static bool init_acl_common_config(vfs_handle_struct *handle)
> @@ -65,6 +73,11 @@ static bool init_acl_common_config(vfs_handle_struct *handle)
> ACL_MODULE_NAME,
> "ignore system acls",
> false);
> + config->default_acl_style = lp_parm_enum(SNUM(handle->conn),
> + ACL_MODULE_NAME,
> + "default acl style",
> + default_acl_style,
> + DEFAULT_ACL_POSIX);
>
> SMB_VFS_HANDLE_SET_DATA(handle, config, NULL,
> struct acl_common_config,
> @@ -387,10 +400,10 @@ static NTSTATUS add_directory_inheritable_components(vfs_handle_struct *handle,
> return NT_STATUS_OK;
> }
>
> -static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> - const char *name,
> - SMB_STRUCT_STAT *psbuf,
> - struct security_descriptor **ppdesc)
> +static NTSTATUS make_default_acl_posix(TALLOC_CTX *ctx,
> + const char *name,
> + SMB_STRUCT_STAT *psbuf,
> + struct security_descriptor **ppdesc)
> {
> struct dom_sid owner_sid, group_sid;
> size_t size = 0;
> @@ -495,6 +508,29 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> return NT_STATUS_OK;
> }
>
> +static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> + struct acl_common_config *config,
> + const char *name,
> + SMB_STRUCT_STAT *psbuf,
> + struct security_descriptor **ppdesc)
> +{
> + NTSTATUS status;
> +
> + switch (config->default_acl_style) {
> +
> + case DEFAULT_ACL_POSIX:
> + status = make_default_acl_posix(ctx, name, psbuf, ppdesc);
> + break;
> +
> + default:
> + DBG_ERR("unknown acl style %d", config->default_acl_style);
> + status = NT_STATUS_INTERNAL_ERROR;
> + break;
> + }
> +
> + return status;
> +}
> +
> /**
> * Validate an ACL blob
> *
> @@ -814,6 +850,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
>
> status = make_default_filesystem_acl(
> mem_ctx,
> + config,
> smb_fname->base_name,
> psbuf,
> &psd);
> --
> 2.7.4
>
>
> From 54dbe78c2ec49dff927bd120e25db1f2a9b10e65 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Thu, 25 Aug 2016 07:45:34 +0200
> Subject: [PATCH 11/12] vfs_acl_common: Windows style default ACL
>
> Reintroduce Windows style default ACL, but this time as an optional
> feature, not changing default behaviour.
>
> Original bugreport that got reverted because it changed the default
> behaviour: https://bugzilla.samba.org/show_bug.cgi?id=12028
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 76 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 919a095..a3f1e3d 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -508,6 +508,78 @@ static NTSTATUS make_default_acl_posix(TALLOC_CTX *ctx,
> return NT_STATUS_OK;
> }
>
> +static NTSTATUS make_default_acl_windows(TALLOC_CTX *ctx,
> + const char *name,
> + SMB_STRUCT_STAT *psbuf,
> + struct security_descriptor **ppdesc)
> +{
> + struct dom_sid owner_sid, group_sid;
> + size_t size = 0;
> + struct security_ace aces[4];
> + uint32_t access_mask = 0;
> + mode_t mode = psbuf->st_ex_mode;
> + struct security_acl *new_dacl = NULL;
> + int idx = 0;
> +
> + DBG_DEBUG("file [%s] mode [0%o]\n", name, (int)mode);
> +
> + uid_to_sid(&owner_sid, psbuf->st_ex_uid);
> + gid_to_sid(&group_sid, psbuf->st_ex_gid);
> +
> + /*
> + * We provide 2 ACEs:
> + * - Owner
> + * - NT System
> + */
> +
> + if (mode & S_IRUSR) {
> + if (mode & S_IWUSR) {
> + access_mask |= SEC_RIGHTS_FILE_ALL;
> + } else {
> + access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
> + }
> + }
> + if (mode & S_IWUSR) {
> + access_mask |= SEC_RIGHTS_FILE_WRITE | SEC_STD_DELETE;
> + }
> +
> + init_sec_ace(&aces[idx],
> + &owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + access_mask,
> + 0);
> + idx++;
> +
> + init_sec_ace(&aces[idx],
> + &global_sid_System,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_RIGHTS_FILE_ALL,
> + 0);
> + idx++;
> +
> + new_dacl = make_sec_acl(ctx,
> + NT4_ACL_REVISION,
> + idx,
> + aces);
> +
> + if (!new_dacl) {
> + return NT_STATUS_NO_MEMORY;
> + }
> +
> + *ppdesc = make_sec_desc(ctx,
> + SECURITY_DESCRIPTOR_REVISION_1,
> + SEC_DESC_SELF_RELATIVE|SEC_DESC_DACL_PRESENT,
> + &owner_sid,
> + &group_sid,
> + NULL,
> + new_dacl,
> + &size);
> + if (!*ppdesc) {
> + return NT_STATUS_NO_MEMORY;
> + }
> + return NT_STATUS_OK;
> +}
> +
> static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> struct acl_common_config *config,
> const char *name,
> @@ -522,6 +594,10 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
> status = make_default_acl_posix(ctx, name, psbuf, ppdesc);
> break;
>
> + case DEFAULT_ACL_WINDOWS:
> + status = make_default_acl_windows(ctx, name, psbuf, ppdesc);
> + break;
> +
> default:
> DBG_ERR("unknown acl style %d", config->default_acl_style);
> status = NT_STATUS_INTERNAL_ERROR;
> --
> 2.7.4
>
>
> From 521fda43f24d35b8af4ed9fd503b1d42f30e3d03 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Thu, 25 Aug 2016 16:30:24 +0200
> Subject: [PATCH 12/12] s4/torture: tests for vfs_acl_xattr default ACL styles
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> selftest/target/Samba3.pm | 8 +
> source3/selftest/tests.py | 4 +-
> source4/torture/vfs/acl_xattr.c | 314 ++++++++++++++++++++++++++++++++++++++++
> source4/torture/vfs/vfs.c | 1 +
> source4/torture/wscript_build | 2 +-
> 5 files changed, 327 insertions(+), 2 deletions(-)
> create mode 100644 source4/torture/vfs/acl_xattr.c
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 8fc3204..f68d7de 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -1792,6 +1792,14 @@ sub provision($$$$$$$$)
> vfs objects = acl_xattr fake_acls xattr_tdb fake_dfq
> inherit owner = yes
> include = $dfqconffile
> +[acl_xattr_ign_sysacl_posix]
> + copy = tmp
> + acl_xattr:ignore system acls = yes
> + acl_xattr:default acl style = posix
> +[acl_xattr_ign_sysacl_windows]
> + copy = tmp
> + acl_xattr:ignore system acls = yes
> + acl_xattr:default acl style = windows
> ";
> close(CONF);
>
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 7d0dcc1..e4c31c6 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -322,7 +322,7 @@ nbt = ["nbt.dgram" ]
>
> libsmbclient = ["libsmbclient"]
>
> -vfs = ["vfs.fruit"]
> +vfs = ["vfs.fruit", "vfs.acl_xattr"]
>
> tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap + vfs
>
> @@ -418,6 +418,8 @@ for t in tests:
> plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD --signing=required')
> elif t == "smb2.dosmode":
> plansmbtorture4testsuite(t, "simpleserver", '//$SERVER/dosmode -U$USERNAME%$PASSWORD')
> + elif t == "vfs.acl_xattr":
> + plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> else:
> plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> diff --git a/source4/torture/vfs/acl_xattr.c b/source4/torture/vfs/acl_xattr.c
> new file mode 100644
> index 0000000..7fd10d0
> --- /dev/null
> +++ b/source4/torture/vfs/acl_xattr.c
> @@ -0,0 +1,314 @@
> +/*
> + Unix SMB/CIFS implementation.
> +
> + Copyright (C) Ralph Boehme 2016
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include "includes.h"
> +#include "lib/cmdline/popt_common.h"
> +#include "libcli/smb2/smb2.h"
> +#include "libcli/smb2/smb2_calls.h"
> +#include "libcli/smb/smbXcli_base.h"
> +#include "torture/torture.h"
> +#include "torture/vfs/proto.h"
> +#include "libcli/resolve/resolve.h"
> +#include "torture/util.h"
> +#include "torture/smb2/proto.h"
> +#include "libcli/security/security.h"
> +#include "librpc/gen_ndr/ndr_security.h"
> +#include "lib/param/param.h"
> +
> +#define BASEDIR "smb2-testsd"
> +
> +#define CHECK_SECURITY_DESCRIPTOR(_sd1, _sd2) do { \
> + if (!security_descriptor_equal(_sd1, _sd2)) { \
> + torture_warning(tctx, "security descriptors don't match!\n"); \
> + torture_warning(tctx, "got:\n"); \
> + NDR_PRINT_DEBUG(security_descriptor, _sd1); \
> + torture_warning(tctx, "expected:\n"); \
> + NDR_PRINT_DEBUG(security_descriptor, _sd2); \
> + torture_result(tctx, TORTURE_FAIL, \
> + "%s: security descriptors don't match!\n", \
> + __location__); \
> + ret = false; \
> + } \
> +} while (0)
> +
> +/**
> + * SMB2 connect with explicit share
> + **/
> +static bool torture_smb2_con_share(struct torture_context *tctx,
> + const char *share,
> + struct smb2_tree **tree)
> +{
> + struct smbcli_options options;
> + NTSTATUS status;
> + const char *host = torture_setting_string(tctx, "host", NULL);
> + struct cli_credentials *credentials = cmdline_credentials;
> +
> + lpcfg_smbcli_options(tctx->lp_ctx, &options);
> +
> + status = smb2_connect_ext(tctx,
> + host,
> + lpcfg_smb_ports(tctx->lp_ctx),
> + share,
> + lpcfg_resolve_context(tctx->lp_ctx),
> + credentials,
> + 0,
> + tree,
> + tctx->ev,
> + &options,
> + lpcfg_socket_options(tctx->lp_ctx),
> + lpcfg_gensec_settings(tctx, tctx->lp_ctx)
> + );
> + if (!NT_STATUS_IS_OK(status)) {
> + printf("Failed to connect to SMB2 share \\\\%s\\%s - %s\n",
> + host, share, nt_errstr(status));
> + return false;
> + }
> + return true;
> +}
> +
> +static bool test_default_acl_posix(struct torture_context *tctx,
> + struct smb2_tree *tree_unused)
> +{
> + struct smb2_tree *tree = NULL;
> + NTSTATUS status;
> + bool ok;
> + bool ret = true;
> + const char *dname = BASEDIR "\\testdir";
> + const char *fname = BASEDIR "\\testdir\\testfile";
> + struct smb2_handle fhandle, dhandle;
> + union smb_fileinfo q;
> + union smb_setfileinfo set;
> + struct security_descriptor *sd = NULL;
> + struct security_descriptor *exp_sd = NULL;
> + char *owner_sid = NULL;
> + char *group_sid = NULL;
> +
> + ok = torture_smb2_con_share(tctx, "acl_xattr_ign_sysacl_posix", &tree);
> + torture_assert_goto(tctx, ok == true, ret, done,
> + "Unable to connect to 'acl_xattr_ign_sysacl_posix'\n");
> +
> + ok = smb2_util_setup_dir(tctx, tree, BASEDIR);
> + torture_assert_goto(tctx, ok == true, ret, done, "Unable to setup testdir\n");
> +
> + ZERO_STRUCT(dhandle);
> + status = torture_smb2_testdir(tree, dname, &dhandle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "torture_smb2_testdir\n");
> +
> + torture_comment(tctx, "Get the original sd\n");
> +
> + ZERO_STRUCT(q);
> + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
> + q.query_secdesc.in.file.handle = dhandle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n");
> +
> + sd = q.query_secdesc.out.sd;
> + owner_sid = dom_sid_string(tctx, sd->owner_sid);
> + group_sid = dom_sid_string(tctx, sd->group_sid);
> + torture_comment(tctx, "owner [%s] group [%s]\n", owner_sid, group_sid);
> +
> + torture_comment(tctx, "Set ACL with no inheritable ACE\n");
> +
> + sd = security_descriptor_dacl_create(tctx,
> + 0, NULL, NULL,
> + owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_RIGHTS_DIR_ALL,
> + 0,
> + NULL);
> +
> + ZERO_STRUCT(set);
> + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
> + set.set_secdesc.in.file.handle = dhandle;
> + set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
> + set.set_secdesc.in.sd = sd;
> + status = smb2_setinfo_file(tree, &set);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file\n");
> +
> + TALLOC_FREE(sd);
> + smb2_util_close(tree, dhandle);
> +
> + torture_comment(tctx, "Create file\n");
> +
> + ZERO_STRUCT(fhandle);
> + status = torture_smb2_testfile(tree, fname, &fhandle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_create_complex_file\n");
> +
> + torture_comment(tctx, "Query file SD\n");
> +
> + ZERO_STRUCT(q);
> + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
> + q.query_secdesc.in.file.handle = fhandle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n");
> + sd = q.query_secdesc.out.sd;
> +
> + smb2_util_close(tree, fhandle);
> + ZERO_STRUCT(fhandle);
> +
> + torture_comment(tctx, "Checking actual file SD against expected SD\n");
> +
> + exp_sd = security_descriptor_dacl_create(
> + tctx, 0, owner_sid, group_sid,
> + owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0,
> + group_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE, 0,
> + SID_WORLD, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE, 0,
> + SID_NT_SYSTEM, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0,
> + NULL);
> +
> + CHECK_SECURITY_DESCRIPTOR(sd, exp_sd);
> +
> +done:
> + if (!smb2_util_handle_empty(fhandle)) {
> + smb2_util_close(tree, fhandle);
> + }
> + if (!smb2_util_handle_empty(dhandle)) {
> + smb2_util_close(tree, dhandle);
> + }
> + if (tree != NULL) {
> + smb2_deltree(tree, BASEDIR);
> + smb2_tdis(tree);
> + }
> +
> + return ret;
> +}
> +
> +static bool test_default_acl_win(struct torture_context *tctx,
> + struct smb2_tree *tree_unused)
> +{
> + struct smb2_tree *tree = NULL;
> + NTSTATUS status;
> + bool ok;
> + bool ret = true;
> + const char *dname = BASEDIR "\\testdir";
> + const char *fname = BASEDIR "\\testdir\\testfile";
> + struct smb2_handle fhandle, dhandle;
> + union smb_fileinfo q;
> + union smb_setfileinfo set;
> + struct security_descriptor *sd = NULL;
> + struct security_descriptor *exp_sd = NULL;
> + char *owner_sid = NULL;
> + char *group_sid = NULL;
> +
> + ok = torture_smb2_con_share(tctx, "acl_xattr_ign_sysacl_windows", &tree);
> + torture_assert_goto(tctx, ok == true, ret, done,
> + "Unable to connect to 'acl_xattr_ign_sysacl_windows'\n");
> +
> + ok = smb2_util_setup_dir(tctx, tree, BASEDIR);
> + torture_assert_goto(tctx, ok == true, ret, done, "Unable to setup testdir\n");
> +
> + ZERO_STRUCT(dhandle);
> + status = torture_smb2_testdir(tree, dname, &dhandle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "torture_smb2_testdir\n");
> +
> + torture_comment(tctx, "Get the original sd\n");
> +
> + ZERO_STRUCT(q);
> + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
> + q.query_secdesc.in.file.handle = dhandle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n");
> +
> + sd = q.query_secdesc.out.sd;
> + owner_sid = dom_sid_string(tctx, sd->owner_sid);
> + group_sid = dom_sid_string(tctx, sd->group_sid);
> + torture_comment(tctx, "owner [%s] group [%s]\n", owner_sid, group_sid);
> +
> + torture_comment(tctx, "Set ACL with no inheritable ACE\n");
> +
> + sd = security_descriptor_dacl_create(tctx,
> + 0, NULL, NULL,
> + owner_sid,
> + SEC_ACE_TYPE_ACCESS_ALLOWED,
> + SEC_RIGHTS_DIR_ALL,
> + 0,
> + NULL);
> +
> + ZERO_STRUCT(set);
> + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
> + set.set_secdesc.in.file.handle = dhandle;
> + set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
> + set.set_secdesc.in.sd = sd;
> + status = smb2_setinfo_file(tree, &set);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_setinfo_file\n");
> +
> + TALLOC_FREE(sd);
> + smb2_util_close(tree, dhandle);
> +
> + torture_comment(tctx, "Create file\n");
> +
> + ZERO_STRUCT(fhandle);
> + status = torture_smb2_testfile(tree, fname, &fhandle);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_create_complex_file\n");
> +
> + torture_comment(tctx, "Query file SD\n");
> +
> + ZERO_STRUCT(q);
> + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
> + q.query_secdesc.in.file.handle = fhandle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER | SECINFO_GROUP;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_getinfo_file\n");
> + sd = q.query_secdesc.out.sd;
> +
> + smb2_util_close(tree, fhandle);
> + ZERO_STRUCT(fhandle);
> +
> + torture_comment(tctx, "Checking actual file SD against expected SD\n");
> +
> + exp_sd = security_descriptor_dacl_create(
> + tctx, 0, owner_sid, group_sid,
> + owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0,
> + SID_NT_SYSTEM, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0,
> + NULL);
> +
> + CHECK_SECURITY_DESCRIPTOR(sd, exp_sd);
> +
> +done:
> + if (!smb2_util_handle_empty(fhandle)) {
> + smb2_util_close(tree, fhandle);
> + }
> + if (!smb2_util_handle_empty(dhandle)) {
> + smb2_util_close(tree, dhandle);
> + }
> + if (tree != NULL) {
> + smb2_deltree(tree, BASEDIR);
> + smb2_tdis(tree);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + basic testing of vfs_acl_xattr
> +*/
> +struct torture_suite *torture_acl_xattr(void)
> +{
> + struct torture_suite *suite = torture_suite_create(talloc_autofree_context(), "acl_xattr");
> +
> + torture_suite_add_1smb2_test(suite, "default-acl-style-posix", test_default_acl_posix);
> + torture_suite_add_1smb2_test(suite, "default-acl-style-windows", test_default_acl_win);
> +
> + suite->description = talloc_strdup(suite, "vfs_acl_xattr tests");
> +
> + return suite;
> +}
> diff --git a/source4/torture/vfs/vfs.c b/source4/torture/vfs/vfs.c
> index f3ce447..7f805f4 100644
> --- a/source4/torture/vfs/vfs.c
> +++ b/source4/torture/vfs/vfs.c
> @@ -107,6 +107,7 @@ NTSTATUS torture_vfs_init(void)
> suite->description = talloc_strdup(suite, "VFS modules tests");
>
> torture_suite_add_suite(suite, torture_vfs_fruit());
> + torture_suite_add_suite(suite, torture_acl_xattr());
>
> torture_register_suite(suite);
>
> diff --git a/source4/torture/wscript_build b/source4/torture/wscript_build
> index 382e2c6..ff79c3d 100755
> --- a/source4/torture/wscript_build
> +++ b/source4/torture/wscript_build
> @@ -271,7 +271,7 @@ bld.SAMBA_MODULE('TORTURE_NTP',
> )
>
> bld.SAMBA_MODULE('TORTURE_VFS',
> - source='vfs/vfs.c vfs/fruit.c',
> + source='vfs/vfs.c vfs/fruit.c vfs/acl_xattr.c',
> subsystem='smbtorture',
> deps='LIBCLI_SMB POPT_CREDENTIALS TORTURE_UTIL smbclient-raw TORTURE_RAW',
> internal_module=True,
> --
> 2.7.4

[Jeremy Allison via samba]

On Fri, Aug 26, 2016 at 02:46:19PM -0700, Jeremy Allison via samba wrote:
> On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote:
> >
> > Cheerio!
> > -slow
>
> Still reviewing this - but a few things that will need changing:
>
> When adding the validate_nt_acl_blob() function in
> [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function
>
> this makes some of the existing function names in debug statements
> incorrect.
>
> Eg. validate_nt_acl_blob() has debug statements:

How about adding this on top - or squashing into it ?

[Ralph Böhme via samba]

On Fri, Aug 26, 2016 at 04:03:49PM -0700, Jeremy Allison wrote:
> On Fri, Aug 26, 2016 at 02:46:19PM -0700, Jeremy Allison via samba wrote:
> > On Fri, Aug 26, 2016 at 06:44:05PM +0200, Ralph Böhme wrote:
> > >
> > > Cheerio!
> > > -slow
> >
> > Still reviewing this - but a few things that will need changing:
> >
> > When adding the validate_nt_acl_blob() function in
> > [PATCH 06/12] vfs_acl_common: move the ACL blob validation to a helper function
> >
> > this makes some of the existing function names in debug statements
> > incorrect.

thanks for spotting this!

> >
> > Eg. validate_nt_acl_blob() has debug statements:
>
> How about adding this on top - or squashing into it ?

Updated patchset attached. My patches now (hopefully) update all DEBUG
calls they touch. New patch on-top that does the rest of the file,
using the new DBG_LEVEL macros in all places.

Cheerio!
-slow

[Ralph Böhme via samba]

...and this one even has bug urls in all commit messages. Sorry for
forgetting this in the previous version.

Cheerio!
-slow

[Jeremy Allison via samba]

On Sat, Aug 27, 2016 at 12:46:12PM +0200, Ralph Böhme via samba wrote:
>
> ...and this one even has bug urls in all commit messages. Sorry for
> forgetting this in the previous version.

Juuuusttt *one* leetle change, sorry :-).

I was following the changes to the talloc heirarchy in the
code and realized that adding the following change made it
much clearer (at least to me).

diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 2163a75..870e6da 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -625,7 +625,7 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx,
vfs_handle_struct *handle,
files_struct *fsp,
const struct smb_filename *smb_fname,
- DATA_BLOB *blob,
+ const DATA_BLOB *blob,
struct security_descriptor **ppsd,
bool *psd_is_from_fs)
{

Making the blob pointer parameter to validate_nt_acl_blob() const helps
be realize it's an 'in' parameter the function should be messing with
(and extra const is always good, right ?).

Also, as you've re-written most of this can I add your name as a

diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 2163a75..fa65fd1 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -4,6 +4,7 @@
*
* Copyright (C) Volker Lendecke, 2008
* Copyright (C) Jeremy Allison, 2009
+ * Copyright (C) Ralph Böhme, 2016
*

to the top as well ? If you're good with these changes squashed
in, 'Reviewed-by: Jeremy Allison <j...@samba.org>'

Let me know and I'll push.

Cheers,

Jeremy.

> From 874b48593225e204e62c9b998a23b021c27dd023 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:04:24 +0200

> Subject: [PATCH 01/13] Revert "vfs_acl_xattr: objects without NT ACL xattr"

>
> This reverts commit 961c4b591bb102751079d9cc92d7aa1c37f1958c.
>
> Subsequent commits will add the same functionality as an optional
> feature.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> From 55d74651c664d210a55b6833dee575dc1b101271 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:08:12 +0200

> Subject: [PATCH 02/13] vfs_acl_common: rename psd to psd_blob in

> get_nt_acl_internal()
>
> This makes it explicit where the SD is originating from. No change in
> behaviour.
>
> This just paves the way for a later change that will simplify the whole
> logic and talloc hierarchy, therefor this also strictly renames the
> occurences after the out label.
>
> Logically, behind the out label, we're dealing with a variable that
> points to what we're going to return, so the name psd_blob is
> misleading, but I'm desperately trying to avoid logic changes in this
> commit and therefor I'm just strictly renaming.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> From b37829fdcc5dac4e289b3b6ccc5a05fad4e529a5 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:11:24 +0200

> Subject: [PATCH 03/13] vfs_acl_common: rename pdesc_next to psd_fs

>
> In most realistic cases the "next" VFS op will return the permissions
> from the filesystem. This rename makes it explicit where the SD is
> originating from. No change in behaviour.
>
> This just paves the way for a later change that will simplify the whole
> logic and talloc hierarchy.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> From 0d2d6afb990c382a508da1823631e1b76af57019 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 13:14:50 +0200

> Subject: [PATCH 04/13] vfs_acl_common: remove redundant NULL assignment

>
> The variables are already set to NULL by TALLOC_FREE.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index fb01bf4..0c40f37 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -551,7 +551,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - psd_blob = NULL;
> goto out;
> }
>
> @@ -562,7 +561,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> (unsigned int)hash_type,
> smb_fname->base_name));
> TALLOC_FREE(psd_blob);
> - psd_blob = NULL;
> goto out;
> }
>
> --
> 2.7.4
>
>

> From 832b1f982b3800c65b2031e4f2bb51c555997cdd Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 17:07:20 +0200

> Subject: [PATCH 05/13] vfs_acl_common: simplify ACL logic, cleanup and talloc

> hierarchy
>
> No change in behaviour (hopefully! :-). This paves the way for moving

> the ACL blob validation to a helper function in the next commit.

>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> From 695256ac9846ee8671388c58b6ded2dbd9adbce9 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Tue, 23 Aug 2016 22:32:57 +0200

> Subject: [PATCH 06/13] vfs_acl_common: move the ACL blob validation to a
> helper function
>
> No change in behaviour.

>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 216 +++++++++++++++++++++++----------------
> 1 file changed, 127 insertions(+), 89 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 66c58e7..2bc2499 100644

> -

> - DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name));
> + *ppsd = NULL;
> + *psd_is_from_fs = false;
>

> @@ -534,35 +533,29 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> * require confirmation of the hash. In particular,
> * the NTVFS file server uses version 1, but
> * 'samba-tool ntacl' can set these as well */
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> case 3:
> case 4:
> if (ignore_file_system_acl) {
> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;
> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
>
> break;

> default:
> - DEBUG(10, ("get_nt_acl_internal: ACL blob revision "
> - "mismatch (%u) for file %s\n",
> - (unsigned int)hash_type,
> - smb_fname->base_name));
> + DBG_DEBUG("ACL blob revision mismatch (%u) for file %s\n",
> + (unsigned int)hash_type, smb_fname->base_name);

> TALLOC_FREE(psd_blob);
> - goto out;
> + return NT_STATUS_OK;
> }
>
> /* determine which type of xattr we got */

> if (hash_type != XATTR_SD_HASH_TYPE_SHA256) {
> - DEBUG(10, ("get_nt_acl_internal: ACL blob hash type "
> - "(%u) unexpected for file %s\n",
> - (unsigned int)hash_type,
> - smb_fname->base_name));
> + DBG_DEBUG("ACL blob hash type (%u) unexpected for file %s\n",
> + (unsigned int)hash_type, smb_fname->base_name);

> TALLOC_FREE(psd_blob);
> - goto out;
> + return NT_STATUS_OK;
> }
>
> /* determine which type of xattr we got */

> @@ -600,12 +593,10 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> if (memcmp(&sys_acl_hash[0], &sys_acl_hash_tmp[0],
> XATTR_SD_HASH_SIZE) == 0) {
> /* Hash matches, return blob sd. */

> - DEBUG(10, ("get_nt_acl_internal: blob hash "
> - "matches for file %s\n",
> - smb_fname->base_name ));

> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;

> + DBG_DEBUG("blob hash matches for file %s\n",
> + smb_fname->base_name);

> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
> }
>

> @@ -629,51 +620,102 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> }
>
> if (!NT_STATUS_IS_OK(status)) {

> - DEBUG(10, ("get_nt_acl_internal: get_next_acl for file %s "
> - "returned %s\n",
> - smb_fname->base_name,
> - nt_errstr(status)));

> + DBG_DEBUG("get_next_acl for file %s returned %s\n",
> + smb_fname->base_name, nt_errstr(status));
> goto fail;

> }
>
> status = hash_sd_sha256(psd_fs, hash_tmp);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(psd_blob);
> - psd = psd_fs;
> - psd_fs = NULL;
> - psd_is_from_fs = true;
> - goto out;
> + *ppsd = psd_fs;
> + *psd_is_from_fs = true;
> + return NT_STATUS_OK;
> }
>
> if (memcmp(&hash[0], &hash_tmp[0], XATTR_SD_HASH_SIZE) == 0) {

> /* Hash matches, return blob sd. */

> - DEBUG(10, ("get_nt_acl_internal: blob hash "
> - "matches for file %s\n",
> - smb_fname->base_name ));

> - psd = psd_blob;
> - psd_blob = NULL;
> - goto out;

> + DBG_DEBUG("blob hash matches for file %s\n",
> + smb_fname->base_name);

> + *ppsd = psd_blob;
> + return NT_STATUS_OK;
> }
>
> /* Hash doesn't match, return underlying sd. */

> - DEBUG(10, ("get_nt_acl_internal: blob hash "
> - "does not match for file %s - returning "
> - "file system SD mapping.\n",
> - smb_fname->base_name ));
> + DBG_DEBUG("blob hash does not match for file %s - returning "
> + "file system SD mapping.\n",
> + smb_fname->base_name);
>
> if (DEBUGLEVEL >= 10) {
> - DEBUG(10,("get_nt_acl_internal: acl for blob hash for %s is:\n",
> - smb_fname->base_name ));
> + DBG_DEBUG("acl for blob hash for %s is:\n",
> + smb_fname->base_name);
> NDR_PRINT_DEBUG(security_descriptor, psd_fs);

> @@ -803,10 +845,6 @@ out:

>
> fail:
> TALLOC_FREE(psd);
> - TALLOC_FREE(psd_blob);
> - TALLOC_FREE(psd_fs);
> - TALLOC_FREE(sys_acl_blob_description);
> - TALLOC_FREE(sys_acl_blob.data);
> return status;
> }
>
> --
> 2.7.4
>
>

> From 69cd4f376d0a287719eadf96ca3324e76317ab66 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:01:17 +0200

> Subject: [PATCH 07/13] vfs_acl_tdb|xattr: use a config handle

>
> Better for performance and a subsequent commit will add one more option
> where this will pay off.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 50 ++++++++++++++++++++++++++++++++--------
> source3/modules/vfs_acl_tdb.c | 7 ++++++
> source3/modules/vfs_acl_xattr.c | 7 ++++++
> 3 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c

> index 2bc2499..fc72b98 100644

> @@ -685,11 +714,12 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> NTSTATUS status;
> struct security_descriptor *psd = NULL;
> const struct smb_filename *smb_fname = NULL;
> - bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
> - ACL_MODULE_NAME,
> - "ignore system acls",
> - false);
> bool psd_is_from_fs = false;
> + struct acl_common_config *config = NULL;
> +
> + SMB_VFS_HANDLE_GET_DATA(handle, config,
> + struct acl_common_config,
> + return NT_STATUS_UNSUCCESSFUL);
>
> if (fsp && smb_fname_in == NULL) {
> smb_fname = fsp->fsp_name;

> @@ -788,7 +818,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> From cdb2da35e9ddc394b9b4fef3080de4a1cae35000 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:30:15 +0200

> Subject: [PATCH 08/13] vfs_acl_common: move stat stuff to a helper function

>
> Will be reused in the next commit when moving the
> make_default_filesystem_acl() stuff to a different place.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 79 ++++++++++++++++++++++++----------------
> 1 file changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c

> index fc72b98..cf50689 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -697,6 +697,48 @@ fail:

> @@ -784,38 +826,13 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> From 7c9c463cdf2204ab7d4cdb7c8bfb8164765615fa Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 10:43:47 +0200

> Subject: [PATCH 09/13] vfs_acl_common: check for ignore_system_acls before

> fetching filesystem ACL
>
> If ignore_system_acls is set and we're synthesizing a default ACL, we
> were fetching the filesystem ACL just to free it again. This change
> avoids this.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 99 ++++++++++++++++++++++------------------
> 1 file changed, 55 insertions(+), 44 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c

> index cf50689..93b4e39 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -792,35 +792,57 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> @@ -835,34 +857,23 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

> From ec7c255a37b72df31f859343eaeb07065aa4653c Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Wed, 24 Aug 2016 20:31:00 +0200

> Subject: [PATCH 10/13] vfs_acl_xattr|tdb: add option to control default ACL

> style
>
> Existing behaviour is "posix" style. Next commit will (re)add the
> "windows" style. This commit doesn't change behaviour in any way.
>

> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> docs-xml/manpages/vfs_acl_tdb.8.xml | 25 ++++++++++++++++++
> docs-xml/manpages/vfs_acl_xattr.8.xml | 25 ++++++++++++++++++
> source3/modules/vfs_acl_common.c | 48 ++++++++++++++++++++++++++++++-----
> 3 files changed, 92 insertions(+), 6 deletions(-)

> index 93b4e39..45ce532 100644

> @@ -400,8 +413,7 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,

> struct security_acl *new_dacl = NULL;

> int idx = 0;
>
> - DEBUG(10,("make_default_filesystem_acl: file %s mode = 0%o\n",
> - name, (int)mode ));
> + DBG_DEBUG("file %s mode = 0%o\n",name, (int)mode);
>
> uid_to_sid(&owner_sid, psbuf->st_ex_uid);
> gid_to_sid(&group_sid, psbuf->st_ex_gid);
> @@ -495,6 +507,29 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,

> @@ -805,6 +840,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,

>
> status = make_default_filesystem_acl(
> mem_ctx,
> + config,
> smb_fname->base_name,
> psbuf,
> &psd);
> --
> 2.7.4
>
>

> From 7afcd4f2164b83a3aaf3b651b69e4a5ac03b8d24 Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Thu, 25 Aug 2016 07:45:34 +0200

> Subject: [PATCH 11/13] vfs_acl_common: Windows style default ACL

>
> Reintroduce Windows style default ACL, but this time as an optional
> feature, not changing default behaviour.
>
> Original bugreport that got reverted because it changed the default
> behaviour: https://bugzilla.samba.org/show_bug.cgi?id=12028
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---
> source3/modules/vfs_acl_common.c | 76 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c

> index 45ce532..9a5b9bd 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -507,6 +507,78 @@ static NTSTATUS make_default_acl_posix(TALLOC_CTX *ctx,

> @@ -521,6 +593,10 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,

> status = make_default_acl_posix(ctx, name, psbuf, ppdesc);
> break;
>
> + case DEFAULT_ACL_WINDOWS:
> + status = make_default_acl_windows(ctx, name, psbuf, ppdesc);
> + break;
> +
> default:
> DBG_ERR("unknown acl style %d", config->default_acl_style);
> status = NT_STATUS_INTERNAL_ERROR;
> --
> 2.7.4
>
>

> From cb873d7d6707d55c5d4a2cc5438d09c1d979c53e Mon Sep 17 00:00:00 2001

> From: Ralph Boehme <sl...@samba.org>
> Date: Thu, 25 Aug 2016 16:30:24 +0200

> Subject: [PATCH 12/13] s4/torture: tests for vfs_acl_xattr default ACL styles

>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> From 4b5d7a5577094544aa52b65a22cc0c61fde97478 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <sl...@samba.org>
> Date: Sat, 27 Aug 2016 10:11:14 +0200
> Subject: [PATCH 13/13] vfs_acl_common: use DBG_LEVEL and remove function
> prefixes in DEBUG statements

>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12177
>
> Signed-off-by: Ralph Boehme <sl...@samba.org>
> ---

> source3/modules/vfs_acl_common.c | 75 ++++++++++++++++++----------------------
> 1 file changed, 33 insertions(+), 42 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 9a5b9bd..2163a75 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -144,8 +144,8 @@ static NTSTATUS parse_acl_blob(const DATA_BLOB *pblob,
> (ndr_pull_flags_fn_t)ndr_pull_xattr_NTACL);
>
> if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
> - DEBUG(5, ("parse_acl_blob: ndr_pull_xattr_NTACL failed: %s\n",
> - ndr_errstr(ndr_err)));
> + DBG_INFO("ndr_pull_xattr_NTACL failed: %s\n",
> + ndr_errstr(ndr_err));
> TALLOC_FREE(frame);
> return ndr_map_error2ntstatus(ndr_err);
> }
> @@ -241,8 +241,8 @@ static NTSTATUS create_acl_blob(const struct security_descriptor *psd,
> (ndr_push_flags_fn_t)ndr_push_xattr_NTACL);
>
> if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
> - DEBUG(5, ("create_acl_blob: ndr_push_xattr_NTACL failed: %s\n",
> - ndr_errstr(ndr_err)));
> + DBG_INFO("ndr_push_xattr_NTACL failed: %s\n",
> + ndr_errstr(ndr_err));
> return ndr_map_error2ntstatus(ndr_err);
> }
>
> @@ -287,8 +287,8 @@ static NTSTATUS create_sys_acl_blob(const struct security_descriptor *psd,
> (ndr_push_flags_fn_t)ndr_push_xattr_NTACL);
>
> if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
> - DEBUG(5, ("create_acl_blob: ndr_push_xattr_NTACL failed: %s\n",
> - ndr_errstr(ndr_err)));
> + DBG_INFO("ndr_push_xattr_NTACL failed: %s\n",
> + ndr_errstr(ndr_err));
> return ndr_map_error2ntstatus(ndr_err);
> }
>
> @@ -345,10 +345,7 @@ static NTSTATUS add_directory_inheritable_components(vfs_handle_struct *handle,
>
> mode = dir_mode | file_mode;
>
> - DEBUG(10, ("add_directory_inheritable_components: directory %s, "
> - "mode = 0%o\n",
> - name,
> - (unsigned int)mode ));
> + DBG_DEBUG("directory %s, mode = 0%o\n", name, (unsigned int)mode);
>
> if (num_aces) {
> memcpy(new_ace_list, psd->dacl->aces,
> @@ -880,7 +877,7 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> smb_fname = smb_fname_in;

> }
>
> - DEBUG(10, ("get_nt_acl_internal: name=%s\n", smb_fname->base_name));

> + DBG_DEBUG("name=%s\n", smb_fname->base_name);

>
> status = get_acl_blob(mem_ctx, handle, fsp, smb_fname, &blob);

> if (NT_STATUS_IS_OK(status)) {
> @@ -1004,8 +1001,8 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
> }
>
> if (DEBUGLEVEL >= 10) {
> - DEBUG(10,("get_nt_acl_internal: returning acl for %s is:\n",
> - smb_fname->base_name ));
> + DBG_DEBUG("returning acl for %s is:\n",
> + smb_fname->base_name);
> NDR_PRINT_DEBUG(security_descriptor, psd);
> }
>
> @@ -1072,9 +1069,8 @@ static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp,
> return NT_STATUS_ACCESS_DENIED;
> }
>
> - DEBUG(10, ("fset_nt_acl_common: overriding chown on file %s "
> - "for sid %s\n",
> - fsp_str_dbg(fsp), sid_string_tos(psd->owner_sid)));
> + DBG_DEBUG("overriding chown on file %s for sid %s\n",
> + fsp_str_dbg(fsp), sid_string_tos(psd->owner_sid));
>
> /* Ok, we failed to chown and we have
> SEC_STD_WRITE_OWNER access - override. */
> @@ -1097,27 +1093,25 @@ static NTSTATUS store_v3_blob(vfs_handle_struct *handle, files_struct *fsp,
> DATA_BLOB blob;
>
> if (DEBUGLEVEL >= 10) {
> - DEBUG(10, ("fset_nt_acl_xattr: storing xattr sd for file %s\n",
> - fsp_str_dbg(fsp)));
> + DBG_DEBUG("storing xattr sd for file %s\n",
> + fsp_str_dbg(fsp));
> NDR_PRINT_DEBUG(
> security_descriptor,
> discard_const_p(struct security_descriptor, psd));
>
> if (pdesc_next != NULL) {
> - DEBUG(10, ("fset_nt_acl_xattr: storing has in xattr sd "
> - "based on \n"));
> + DBG_DEBUG("storing xattr sd based on \n");
> NDR_PRINT_DEBUG(
> security_descriptor,
> discard_const_p(struct security_descriptor,
> pdesc_next));
> } else {
> - DEBUG(10,
> - ("fset_nt_acl_xattr: ignoring underlying sd\n"));
> + DBG_DEBUG("ignoring underlying sd\n");
> }
> }
> status = create_acl_blob(psd, &blob, XATTR_SD_HASH_TYPE_SHA256, hash);
> if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("fset_nt_acl_xattr: create_acl_blob failed\n"));
> + DBG_DEBUG("create_acl_blob failed\n");
> return status;
> }
>
> @@ -1146,8 +1140,7 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,

> SNUM(handle->conn), ACL_MODULE_NAME, "ignore system acls", false);
>

> if (DEBUGLEVEL >= 10) {
> - DEBUG(10,("fset_nt_acl_xattr: incoming sd for file %s\n",
> - fsp_str_dbg(fsp)));
> + DBG_DEBUG("incoming sd for file %s\n", fsp_str_dbg(fsp));
> NDR_PRINT_DEBUG(security_descriptor,
> discard_const_p(struct security_descriptor, orig_psd));
> }
> @@ -1265,12 +1258,12 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,
> }
>
> if (DEBUGLEVEL >= 10) {
> - DEBUG(10,("fset_nt_acl_xattr: storing xattr sd for file %s based on system ACL\n",
> - fsp_str_dbg(fsp)));
> + DBG_DEBUG("storing xattr sd for file %s based on system ACL\n",
> + fsp_str_dbg(fsp));
> NDR_PRINT_DEBUG(security_descriptor,
> discard_const_p(struct security_descriptor, psd));
>
> - DEBUG(10,("fset_nt_acl_xattr: storing hash in xattr sd based on system ACL and:\n"));
> + DBG_DEBUG("storing hash in xattr sd based on system ACL and:\n");
> NDR_PRINT_DEBUG(security_descriptor,
> discard_const_p(struct security_descriptor, pdesc_next));
> }
> @@ -1282,7 +1275,7 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,
> status = create_sys_acl_blob(psd, &blob, XATTR_SD_HASH_TYPE_SHA256, hash,
> sys_acl_description, sys_acl_hash);
> if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("fset_nt_acl_xattr: create_sys_acl_blob failed\n"));
> + DBG_DEBUG("create_sys_acl_blob failed\n");
> TALLOC_FREE(frame);
> return status;
> }
> @@ -1319,9 +1312,8 @@ static int acl_common_remove_object(vfs_handle_struct *handle,
> goto out;
> }
>
> - DEBUG(10,("acl_common_remove_object: removing %s %s/%s\n",
> - is_directory ? "directory" : "file",
> - parent_dir, final_component ));
> + DBG_DEBUG("removing %s %s/%s\n", is_directory ? "directory" : "file",
> + parent_dir, final_component);
>
> /* cd into the parent dir to pin it. */
> ret = vfs_ChDir(conn, parent_dir);
> @@ -1354,10 +1346,9 @@ static int acl_common_remove_object(vfs_handle_struct *handle,
> }
>
> if (!fsp) {
> - DEBUG(10,("acl_common_remove_object: %s %s/%s "
> - "not an open file\n",
> - is_directory ? "directory" : "file",
> - parent_dir, final_component ));
> + DBG_DEBUG("%s %s/%s not an open file\n",
> + is_directory ? "directory" : "file",
> + parent_dir, final_component);
> saved_errno = EACCES;
> goto out;
> }
> @@ -1405,9 +1396,9 @@ static int rmdir_acl_common(struct vfs_handle_struct *handle,
> true);
> }
>
> - DEBUG(10,("rmdir_acl_common: unlink of %s failed %s\n",
> - smb_fname->base_name,
> - strerror(errno) ));
> + DBG_DEBUG("unlink of %s failed %s\n",
> + smb_fname->base_name,
> + strerror(errno));
> return -1;
> }
>
> @@ -1434,9 +1425,9 @@ static int unlink_acl_common(struct vfs_handle_struct *handle,
> false);
> }
>
> - DEBUG(10,("unlink_acl_common: unlink of %s failed %s\n",
> - smb_fname->base_name,
> - strerror(errno) ));
> + DBG_DEBUG("unlink of %s failed %s\n",
> + smb_fname->base_name,
> + strerror(errno));
> return -1;
> }
>
> --
> 2.7.4

[Ralph Böhme via samba]

Hi Jeremy,

On Mon, Aug 29, 2016 at 01:38:21PM -0700, Jeremy Allison wrote:
> On Sat, Aug 27, 2016 at 12:46:12PM +0200, Ralph Böhme via samba wrote:
> >
> > ...and this one even has bug urls in all commit messages. Sorry for
> > forgetting this in the previous version.
>
> Juuuusttt *one* leetle change, sorry :-).

:)

> I was following the changes to the talloc heirarchy in the
> code and realized that adding the following change made it
> much clearer (at least to me).
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 2163a75..870e6da 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -625,7 +625,7 @@ static NTSTATUS validate_nt_acl_blob(TALLOC_CTX *mem_ctx,
> vfs_handle_struct *handle,
> files_struct *fsp,
> const struct smb_filename *smb_fname,
> - DATA_BLOB *blob,
> + const DATA_BLOB *blob,
> struct security_descriptor **ppsd,
> bool *psd_is_from_fs)
> {
>
> Making the blob pointer parameter to validate_nt_acl_blob() const helps
> be realize it's an 'in' parameter the function should be messing with
> (and extra const is always good, right ?).

makes perfect sense.

> Also, as you've re-written most of this can I add your name as a
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index 2163a75..fa65fd1 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -4,6 +4,7 @@
> *
> * Copyright (C) Volker Lendecke, 2008
> * Copyright (C) Jeremy Allison, 2009
> + * Copyright (C) Ralph Böhme, 2016
> *
>
> to the top as well ? If you're good with these changes squashed
> in, 'Reviewed-by: Jeremy Allison <j...@samba.org>'

ack.

> Let me know and I'll push.

yes please. :)

Thanks!

Cheerio!
-slow