Bug 8083 - "inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module
"inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_ac...
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: VFS Modules
3.5.8
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on: 8072
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-12 22:21 UTC by Jeremy Allison
Modified: 2011-06-10 19:14 UTC (History)
2 users (show)

See Also:


Attachments
git-am fix for 3.5.next (2.64 KB, patch)
2011-04-13 00:38 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 3.5.next (2.62 KB, patch)
2011-04-19 20:27 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 3.6.0 (2.26 KB, patch)
2011-06-07 19:26 UTC, Jeremy Allison
bjacke: review+
Details
git-am fix for 3.5.9. (2.34 KB, patch)
2011-06-07 19:38 UTC, Jeremy Allison
bjacke: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2011-04-12 22:21:55 UTC
Even though setting "inherit permissions = yes" correctly sets the owner of a newly created file, the code in modules/vfs_acl_common will overwrite it with the owner sid from the currently connected user token.

Jeremy.
Comment 1 Jeremy Allison 2011-04-12 22:41:16 UTC
It turns out that the vfs_acl_common code doesn't actually have to handle the CHOWN or FCHOWN calls, as these are currently made only from codepaths triggered by "inherit owner = yes", which we will handle inside the inherit_new_acl() code inside modules/vfs_acl_common.c.

I will post a small patch for 3.5.next here, and post a larger patch set for metze to review for master/v3-6-test on samba-technical.

Jeremy.
Comment 2 Jeremy Allison 2011-04-13 00:31:11 UTC
Arg. I meant "inherit owner", not inherit permissions of course. Fixed the bug subject line..

Jeremy.
Comment 3 Jeremy Allison 2011-04-13 00:38:43 UTC
Created attachment 6402 [details]
git-am fix for 3.5.next

Metze - please review. If you're ok with this I'll create a similar fix for master/v3-6-test. For master v-3-6-test I'm working on adding more secure versions of the chown_fsp call, to reduce the risk of symlink races on pathnames when root, but this is a separate change to this fix.

"If "inherit owner = yes", pass in the directory owner and group
owner as the target for CREATOR_OWNER and CREATOR_GROUP substitutions,
and also as the owner and primary group of the new security descriptor
being applied to the object."

Jeremy.
Comment 4 Björn Baumbach 2011-04-13 11:29:57 UTC
Jeremy, thank you for the patch!
I applied it to my test env and did the tests again.

I've a test directory, owned by root:root and the ace which allows my user bbaumba to write to the dir.
I can create files and sub directories in the directory and the ownership is inherited to root:root without any errors on the Windows client side, great!

But if I create a file (not a directory), there are NT_STATUS_ACCESS_DENIED messages on first open_acl_common() call in Sambas log file. After playing with the NT acls of the parent on windows side (added inheritance to sub dirs/files and removed the rule to delete and change the ownership) I get files with -r--rwx---+  root root permissions and ACCESS_DENIED messages if I read the NT ACLS with Windows. But I can see bbaumba's permissions and get no errors on windows side.
Comment 5 Björn Baumbach 2011-04-13 14:48:24 UTC
I did some additional tests.

I created a directory and set the ownership to
root:bbaumba

With Windows I added the rules for the group, that allows users to create, write, read, ... and NOT to delete files or change the permissions/ownership (and inherit them).

getfacl tells me now
default:group:bbaumba:rwx
which is correct.

I can now create files in the directory, can NOT remove them (fine), but unfortunately not write to them. If I edit a text file and try to safe my changes Samba says:

[2011/04/13 16:39:34.419047,  5] smbd/open.c:1769(open_file_ntcreate)
  open_file_ntcreate: write access requested for file testdir3/Neu Textdokument (5).txt on read only file
[2011/04/13 16:39:34.419069,  5] smbd/files.c:497(file_free)
  freed files structure 18316 (1 used)
[2011/04/13 16:39:34.419085, 10] smbd/open.c:3209(create_file_unixpath)
  create_file_unixpath: NT_STATUS_ACCESS_DENIED
[2011/04/13 16:39:34.419100, 10] smbd/open.c:3462(create_file_default)
  create_file: NT_STATUS_ACCESS_DENIED

But getfacl says it should be allowed:
# owner: root
# group: bbaumba
user::r-x
group::rwx
other::r-x

Same with smbcacl and Windows:
OWNER:BBASDF\root
GROUP:Unix Group\bbaumba
ACL:Unix Group\bbaumba:ALLOWED/0x0/RWX
Comment 6 Jeremy Allison 2011-04-13 22:13:17 UTC
This error:

"open_file_ntcreate: write access requested for file testdir3/Neu Textdokument (5).txt on read only file"

is a check on DOS attributes, not on the permissions of a file. Using a GUI text editor may be trying to do other things than simply open the file for write. You might want to make sure you also have WRITE_ATTRIBUTES set on the inherited ACL, as well as WRITE_DATA.

Don't use a GUI test, try just doing:

copy file.txt \\server\dropbox_share\dir

and then:

echo >>\\server\dropbox_share\dir\file.txt

to test this. That should match more what a dropbox can realistically do.
Remember, trying to use complex apps. on this will fail, as things like MS-Office do:

create new file with tmp name
write data
rename tmp name -> orig name

when editing a file. The "rename" here will fail, as it requires DELETE access to the file (which the dropbox denies).

Dropboxes are limited. I still think this patch is good :-).

Jeremy.
Comment 7 Björn Baumbach 2011-04-14 08:10:08 UTC
Okay, you're right, this patch is good. Dropped files with your patches and the owner was inherited successfully. :-)

Thank you very much :-)
Comment 8 Jeremy Allison 2011-04-14 16:17:42 UTC
Metze please review asap as I'd like to get this fix into master and v3-6-test (and I'm honoring your request not to do that without review :-).

Jeremy.
Comment 9 Stefan Metzmacher 2011-04-14 20:02:41 UTC
I guess that's fine for 3.6 and master.
Comment 10 Jeremy Allison 2011-04-14 20:21:38 UTC
Can you also bless this for 3.5.next and I'll reassign to Karolin for inclusion.

Thanks,

Jeremy.
Comment 11 Stefan Metzmacher 2011-04-19 17:55:57 UTC
Comment on attachment 6402 [details]
git-am fix for 3.5.next

Looks good
Comment 12 Stefan Metzmacher 2011-04-19 17:56:33 UTC
Karolin, please pick for the next release
Comment 13 Karolin Seeger 2011-04-19 19:13:02 UTC
Patch does not apply (HEAD 4a46715):

user@host:/data/git/samba/v3-5-test> git am 0001-Fix-bug-8083-inherit-owner-yes-doesn-t-interact-corr.patch
Applying: Fix bug #8083 - "inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module.
error: patch failed: source3/modules/vfs_acl_common.c:472
error: source3/modules/vfs_acl_common.c: patch does not apply
Patch failed at 0001 Fix bug #8083 - "inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module.
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Comment 14 Jeremy Allison 2011-04-19 20:27:24 UTC
Created attachment 6417 [details]
git-am fix for 3.5.next

Karolin - please try this version (should apply to v3-5-test cleanly). It should be identical to the version that won't apply (don't know why that failed).
Jeremy.
Comment 15 Stefan Metzmacher 2011-04-20 07:58:21 UTC
Comment on attachment 6417 [details]
git-am fix for 3.5.next

Looks ok, too
Comment 16 Björn Jacke 2011-05-30 10:33:34 UTC
Karolin, your turn again :-)
Comment 17 Karolin Seeger 2011-05-30 18:01:06 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 18 Björn Jacke 2011-06-07 09:55:14 UTC
with

 inherit permissions = yes

and

 inherit acls = yes

inherit owner = yes does also not work. Jeremy, can you have another look at that combination?
Comment 19 Jeremy Allison 2011-06-07 17:51:29 UTC
Ok, reproduced this. I'll look into it asap.

Jeremy.
Comment 20 Jeremy Allison 2011-06-07 19:26:47 UTC
Created attachment 6540 [details]
git-am fix for 3.6.0
Comment 21 Jeremy Allison 2011-06-07 19:38:27 UTC
Created attachment 6541 [details]
git-am fix for 3.5.9.

Fix for 3.5.9 as well.

If the fix goes into 3.5.9 it also needs to go into 3.6.0-final so we don't regress between 3.5.9 -> 3.6.0.

Jeremy.
Comment 22 Björn Jacke 2011-06-07 20:41:52 UTC
just tried the 3.5 patch for now and that patch doesn't fix it.

By the way - the problem occurs only with created directories. For created files "inherit owner" works for me.
Comment 23 Jeremy Allison 2011-06-07 20:59:00 UTC
Very strange. It fixes it perfectly for me (plus it also occurred on files).

What settings do you have in the share stanza for your smb.conf ? I have:

        path = /tmp/foo
        vfs objects = acl_xattr
        inherit permissions = yes
        inherit acls = yes
        inherit owner = yes
        read only = no
        guest ok = true

The logic added should certainly fix this issue. Can you describe more how you're reproducing this ?

Jeremy.
Comment 24 Björn Jacke 2011-06-07 21:11:00 UTC
the main difference that probably matters is that I testes without the acl_xattr module but just POSIX ACLs
Comment 25 Jeremy Allison 2011-06-07 21:13:46 UTC
Oh, then that is a new bug. Look at the title of this bug :

""inherit owner = yes" doesn't interact correctly with vfs_acl_xattr or vfs_acl_tdb module"

Kind of implies the vfs_acl is part of the bug report :-).

Ok, please open a new bug - the logic changes here are certainly needed for use with acl_xattr. The new bug can track the posix acl-only problem.

Please confirm the fix on this bug works with acl_xattr, and we can get this change into 3.5.9 and 3.6.0-final.

Jeremy.
Comment 26 Björn Jacke 2011-06-08 12:29:06 UTC
Comment on attachment 6540 [details]
git-am fix for 3.6.0

works for me with the xattr acl module, thanks!
Comment 27 Jeremy Allison 2011-06-08 16:09:40 UTC
Re-assigning to Karolin.

Karolin, patch :

https://attachments.samba.org/attachment.cgi?id=6541

needs to go into 3.5.9, and patch:

https://attachments.samba.org/attachment.cgi?id=6540

needs to go into 3.6.0-final.

Thanks !

Jeremy.
Comment 28 Karolin Seeger 2011-06-09 18:07:58 UTC
(In reply to comment #27)
> Re-assigning to Karolin.
> 
> Karolin, patch :
> 
> https://attachments.samba.org/attachment.cgi?id=6541

Unfortunately, it's too late for 3.5.9.
Do we need to delay the release or is shipping the fix with 3.5.10 ok?

> needs to go into 3.5.9, and patch:
> 
> https://attachments.samba.org/attachment.cgi?id=6540
> 
> needs to go into 3.6.0-final.

Pushed, thanks!

> Thanks !
> 
> Jeremy.
Comment 29 Karolin Seeger 2011-06-10 19:14:48 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > Re-assigning to Karolin.
> > 
> > Karolin, patch :
> > 
> > https://attachments.samba.org/attachment.cgi?id=6541
> 
> Unfortunately, it's too late for 3.5.9.
> Do we need to delay the release or is shipping the fix with 3.5.10 ok?

Pushed to v3-5-test.
If there are no objections, it will be included in 3.5.9.
Closing out bug report.

Thanks!