Bug 8072 - PANIC: create_file_acl_common frees handle two times
PANIC: create_file_acl_common frees handle two times
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: VFS Modules
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8083
  Show dependency treegraph
 
Reported: 2011-04-08 16:00 UTC by Björn Baumbach
Modified: 2011-04-19 19:22 UTC (History)
1 user (show)

See Also:


Attachments
backtrace create_file_acl_common() => create_file_acl_common() (13.88 KB, text/plain)
2011-04-08 16:00 UTC, Björn Baumbach
no flags Details
backtrace create_file_acl_common() => open_acl_common() (12.80 KB, text/plain)
2011-04-08 16:04 UTC, Björn Baumbach
no flags Details
git-am fix for 3.5.next (6.82 KB, patch)
2011-04-08 22:27 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Baumbach 2011-04-08 16:00:28 UTC
Created attachment 6393 [details]
backtrace create_file_acl_common() => create_file_acl_common()

Using parameter:
vfs objects = acl_xattr
inherit owner = Yes
Comment 1 Björn Baumbach 2011-04-08 16:04:20 UTC
Created attachment 6394 [details]
backtrace create_file_acl_common() => open_acl_common()
Comment 2 Stefan Metzmacher 2011-04-08 16:05:45 UTC
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!
Comment 3 Jeremy Allison 2011-04-08 16:58:32 UTC
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.
Comment 4 Jeremy Allison 2011-04-08 17:05:01 UTC
One more thing Björn, exactly how did you reproduce this crash ?
Jeremy.
Comment 5 Stefan Metzmacher 2011-04-08 19:02:25 UTC
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().
Comment 6 Jeremy Allison 2011-04-08 19:09:35 UTC
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.
Comment 7 Jeremy Allison 2011-04-08 22:27:14 UTC
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.
Comment 8 Björn Baumbach 2011-04-09 19:17:11 UTC
Great! I'll try the fix Monday morning in my test env.
Comment 9 Stefan Metzmacher 2011-04-11 12:16:15 UTC
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.
Comment 10 Björn Baumbach 2011-04-12 13:52:36 UTC
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.
Comment 11 Jeremy Allison 2011-04-12 19:43:20 UTC
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.
Comment 12 Jeremy Allison 2011-04-12 22:22:35 UTC
Ok, new bug added is :

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

I will attach patches there for review.

Jeremy.
Comment 13 Jeremy Allison 2011-04-12 23:23:47 UTC
(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 14 Stefan Metzmacher 2011-04-19 17:54:17 UTC
Comment on attachment 6396 [details]
git-am fix for 3.5.next

Looks good
Comment 15 Stefan Metzmacher 2011-04-19 17:55:03 UTC
Karolin, please pick for the next release
Comment 16 Karolin Seeger 2011-04-19 19:22:28 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!