Bug 7933 - samba fails to honor SEC_STD_WRITE_OWNER bit with the acl_xattr module
samba fails to honor SEC_STD_WRITE_OWNER bit with the acl_xattr module
Status: ASSIGNED
Product: Samba 3.5
Classification: Unclassified
Component: VFS Modules
3.5.6
Other Linux
: P3 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-26 02:36 UTC by Matthieu Patou
Modified: 2012-10-05 08:12 UTC (History)
0 users

See Also:


Attachments
capture with a simple user (demo) not in the admin group taking ownership (132.86 KB, application/cap)
2011-11-25 00:01 UTC, Matthieu Patou
no flags Details
acl on the file without the take ownership right (108.95 KB, image/jpeg)
2011-11-25 00:02 UTC, Matthieu Patou
no flags Details
acl on the file with the take ownership right (107.37 KB, image/jpeg)
2011-11-25 00:02 UTC, Matthieu Patou
no flags Details
screenshot showning that the change of owner if forbidden on file without_to_aclrights (103.65 KB, image/jpeg)
2011-11-25 00:03 UTC, Matthieu Patou
no flags Details
fix for 3.6.next (2.30 KB, patch)
2012-02-03 22:51 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x that contains both fixes from master. (7.96 KB, patch)
2012-03-13 20:59 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x that contains both fixes from master. (7.96 KB, patch)
2012-05-22 22:00 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthieu Patou 2011-01-26 02:36:52 UTC
If a user has the take ownership bit set on a file he should be able to take ownership of a file (this option is offered by XP only if this bit is set).

Currently when trying to do so we receive an access denied dialog box.

To reproduce the problem: 

* Setup a share with the acl_xattr module
* create a file with user A gives read/write rights to the user B
* with user B check on Windows that you are not offered the right to change the owner
* with user A give the "take ownership right" to user B (security tab then advanced then add then check "take ownership").
* with user B try to take the ownership, windows will allow you to proceed but you'll receive an access denied from the server
Comment 1 Jeremy Allison 2011-01-26 19:16:26 UTC
Matthieu, I'm guessing that when you tried this on a Windows box the user who was taking the ownership had SE_TAKE_OWNERSHIP privilege, and the user you're trying this with on the Samba box does not.

Can you add the SE_TAKE_OWNERSHIP privilege to the user who is taking the ownership on the Samba box and retry please, as this is required in order to do the underlying chown action.

Jeremy.
Comment 2 Matthieu Patou 2011-01-27 02:51:28 UTC
>Matthieu, I'm guessing that when you tried this on a Windows box the user who
>was taking the ownership had SE_TAKE_OWNERSHIP privilege, and the user you're
>trying this with on the Samba box does not.

No the user who created the file was an admin but the user who was granted the "take ownership right" was a simple user.

If you are a user with the SE_TAKE_OWNERSHIP privilege, you don't need the "take ownership" right on a file to be allowed to take the ownership.

>Can you add the SE_TAKE_OWNERSHIP privilege to the user who is taking
>the
>ownership on the Samba box and retry please, as this is required in order to do
>the underlying chown action.
This I tried it's working correctly.

This is just the handling of the ACL's bit "take ownership" that is not correct.


Jeremy.
Comment 3 Jeremy Allison 2011-01-27 12:58:21 UTC
Ok I tried to test this on my Win7 client box locally. I created a new "test" user with no privileges and gave that user "take ownership" on a file owned by a different user, but when I tried to take ownership I got a security privilege request asking for an administrator password.

I think we need to torture test this to be sure.

Jeremy.
Comment 4 Matthieu Patou 2011-01-27 13:43:22 UTC
I must confess that I didn't try with a windows 7 but I tried with XP one more time and windows server 2003 R2.

I have the feeling that UAC is entering into the game to "protect" your computer. You should redo the test on a share remotely or deactivate UAC. 
Comment 5 Jeremy Allison 2011-01-31 17:38:22 UTC
I still can't reproduce this on an XP box :-(. Can you upload a debug level 10 log of this failing against the v3-5-test tree as well as a wireshark trace (as there has been a lot of changes to the ACL code between 3.5.6 and the current v3-5-test tree).

Jeremy.
Comment 6 Matthieu Patou 2011-02-01 01:53:37 UTC
Jeremy, do you mean that you are unable to reproduce the fact that a user that has not SE_TAKE_OWNERSHIP right can still take the ownership of a file if he has be granted the "take ownership" bit on a file in XP or you can't reproduce the problem when using XP as a client to s3.5-test server ?
Comment 7 Matthieu Patou 2011-11-18 14:20:50 UTC
Jeremy any update on this bug ?
Comment 8 Jeremy Allison 2011-11-18 22:32:01 UTC
No. I still assert that SE_TAKE_OWNERSHIP privilege is *required* to change file ownership to yourself. You haven't shown me any traces that prove me wrong here.
Comment 9 Matthieu Patou 2011-11-25 00:01:05 UTC
Jeremy, you have to review your assertion :-)

See the attached trace, at packet 300 you can see that the owner is the local administrator group and at packet 321 I'm able to change the owner, if you look at the DACL part of the ACL (in packet 184 you'll see that this user has the right to set the owner on this file).

I added a couple of screenshot that shows that if I don't have the right (file without_to_aclright.jpg) then I can't take the ownership ...
Comment 10 Matthieu Patou 2011-11-25 00:01:57 UTC
Created attachment 7140 [details]
capture with a simple user (demo) not in the admin group taking ownership
Comment 11 Matthieu Patou 2011-11-25 00:02:33 UTC
Created attachment 7141 [details]
acl on the file without the take ownership right
Comment 12 Matthieu Patou 2011-11-25 00:02:56 UTC
Created attachment 7142 [details]
acl on the file with the take ownership right
Comment 13 Matthieu Patou 2011-11-25 00:03:39 UTC
Created attachment 7143 [details]
screenshot showning that the change of owner if forbidden on file without_to_aclrights
Comment 14 Jeremy Allison 2011-11-25 05:10:29 UTC
Ok we should write a torture test to check this against Windows from a non-privileged user. Will have to wait until after thanksgiving though.

Jeremy.
Comment 15 Jeremy Allison 2012-02-03 00:17:24 UTC
Crap, I'm wrong - tested on Win7 :-(. I'll add a patch that enables this for vfs_acl_common.c but I really don't want to do this for "standard" POSIX mapping as rwx maps to FULL CONTROL which means anyone with rwx on a file could chown it.

Matthieu, thanks for being so persistent in making me check this.

Jeremy.
Comment 16 Jeremy Allison 2012-02-03 03:40:05 UTC
Ok, I have a fix for this for the vfs_acl_xattr and vfs_acl_tdb modules (in master). I'll check in tomorrow once I've confirmed the patch.

Jeremy.
Comment 17 Jeremy Allison 2012-02-03 22:51:13 UTC
Created attachment 7292 [details]
fix for 3.6.next

Works for me here (although it exposed another mapping bug). Matthieu, can you test this out and confirm please ?

Thanks,

Jeremy.
Comment 18 Jeremy Allison 2012-02-15 18:49:28 UTC
Comment on attachment 7292 [details]
fix for 3.6.next

Matthieu, can you review for 3.6.next please ?
Comment 19 Jeremy Allison 2012-03-13 19:11:00 UTC
Patch is incorrect - will crash smbd. Thanks to 
Andrew Bartlett <abartlet@samba.org> and Ricky Nance <ricky.nance@weaubleau.k12.mo.us> for finding this problem.

Fixed patch to follow !

Jeremy
Comment 20 Jeremy Allison 2012-03-13 19:11:26 UTC
Comment on attachment 7292 [details]
fix for 3.6.next

Patch crashes smbd. Modified fix to follow.
Comment 21 Jeremy Allison 2012-03-13 20:59:25 UTC
Created attachment 7384 [details]
git-am fix for 3.6.x that contains both fixes from master.
Comment 22 Jeremy Allison 2012-05-22 22:00:29 UTC
Created attachment 7581 [details]
git-am fix for 3.6.x that contains both fixes from master.

New patch with fix noticed by 逸群周 <jxxyyzz@gmail.com>.
Comment 23 Matthieu Patou 2012-10-05 08:12:14 UTC
should we close this bug ?