Created attachment 6394 [details]
backtrace create_file_acl_common() => open_acl_common()

create_file_acl_common() is called recursivly.

And passing the parent security descriptor from open_acl_common()
to create_file_acl_common() via SMB_VFS_HANDLE_SET/GET_DATA()
can't work.

handle->data is the global state for the module!
It's not for temporary stuff!

I think it may be the "inherit owner = yes" that causes the recursive call. I'll look into this asap. It certainly doesn't happen without this.

The use of the module handle is incredibly ugly, but does demonstrably work (unless we get called recursively :-). The problem is in the mkdir path, we only have a path based call, not a handle based call. So there is nowhere to put the security descriptor we need to check and then re-use for the inherit_new_acl() call required in Windows security descriptor processing when we're creating a new object.

As it's only an optimization and won't hurt to call twice in the directory case, I can move this to the fsp handle in the file case, and just do the extra work in the mkdir case.

Jeremy.

One more thing Björn, exactly how did you reproduce this crash ?
Jeremy.

Just create a new file text file in the windows explorer.

The panic doesn't happen when creating a new directory.

The problem is file_set_dosmode() -> set_ea_dos_attribute()
and at the first look it seem unrelated to inherit owner = yes.

change_file_owner_to_parent() is the only change based on
lp_inherit_owner() and that only calls SMB_VFS_FCHOWN().

Ah, ok. It happens when :

        if (SMB_VFS_SETXATTR(conn, smb_fname->base_name,
                             SAMBA_XATTR_DOS_ATTRIB, blob.data, blob.length,

returns -1 with errno = EPERM or EACCESS. That's when it goes into this codepath. That's why I didn't catch this earlier. Thanks !

Jeremy.

Created attachment 6396 [details]
git-am fix for 3.5.next

Can you try this fix in your test env. to see if it fixes it (it should).
Jeremy.

Great! I'll try the fix Monday morning in my test env.

The patch itself looks good, but "inherit owner = yes" is useless
as the code in inherit_new_acl() resets the owner to the connected user,
if there're inheritable ACEs in the parent sd. That's why I'll set the review '+'
later.

I'm not sure on how "inherit owner = yes" should work at all.
Does the "CREATOR OWNER"/"CREATOR GROUP" ACE's should apply to
the connected user or the new inherited owner or maybe both?

Jeremy, please discuss possible patches with me before pushing
them to master and v3-6-test. We need to define the supposed
behavior before pushing any patches.

I tried the fix in my test env.

If I create a file as user bbaumba in a directory with root:root permissions, and
the owner is successfully inherited.
The ACE (which allows the user to write is user:bbaumba:rwx
and the nt acl is ACL:BBASDF\bbaumba:ALLOWED/0x0/FULL

On the first view on Windows it looked fine, but the new file got root:bbaumba permissions. bbaumba is the primary group of user bbaumba.
I think the group should also be inherited, doesnt't it?

If I change now the create mask to 0774, that should allow bbaumba (because he is in the group bbaumba) to write to the file, windows tells me, that I'm not allowed to create a file. Samba creates the file with root:bbaumba anyway.

In all cases there are NT_STATUS_ACCESS_DENIED Errors in Sambas log file. I think it's easier if you try the stuff on your test env. and analyze the behavior.
Please feel free to ask, if you need more details or data.

---

BTW: There is no dependency to modules/vfs_acl_common.c in the Makefile.in, because modules/vfs_acl_xattr.c and modules/vfs_acl_acl.c includes the modules/vfs_acl_common.c.
Just wondered why there was nothing to rebuild after applying your patch.

Ok I see the issue. The acl_xattr and acl_tdb code (via -> acl_common) don't interact well with the "inherit owner" smb.conf parameter. None of them intercept the SMB_VFS_CHOWN or SMB_VFS_FCHOWN calls. I think we should apply the fix for this one as-is, and create a new bug to handle the separate patch for the "inherit owner" fix.

Don't worry - no new code in master or 3.6.0 without metze review :-).

Jeremy.

Ok, new bug added is :

https://bugzilla.samba.org/show_bug.cgi?id=8083

I will attach patches there for review.

Jeremy.

(In reply to comment #10)
> I tried the fix in my test env.
> 
> If I create a file as user bbaumba in a directory with root:root permissions,
> and
> the owner is successfully inherited.
> The ACE (which allows the user to write is user:bbaumba:rwx
> and the nt acl is ACL:BBASDF\bbaumba:ALLOWED/0x0/FULL
> 
> On the first view on Windows it looked fine, but the new file got root:bbaumba
> permissions. bbaumba is the primary group of user bbaumba.
> I think the group should also be inherited, doesnt't it?

"inherit owner = yes" when used without vfs_acl_xattr or vfs_acl_tdb only inherits the owner, not the group. However, when using these modules I do think it makes sense to also inherit the group from the containing directory.

The main use case of "inherit owner = yes" when combined with vfs_acl_xattr or vfs_acl_tdb modules is to create a dropbox, so an additional ACL granting access to a secondary group can be used to fine tune the dropbox permissions I think.

I'll make this change to the patch I'm creating for:

https://bugzilla.samba.org/show_bug.cgi?id=8083

Jeremy.

Comment on attachment 6396 [details]
git-am fix for 3.5.next

Looks good

Karolin, please pick for the next release

Pushed to v3-5-test.
Closing out bug report.

Thanks!