Bug 7933 - samba fails to honor SEC_STD_WRITE_OWNER bit with the acl_xattr module
Summary: samba fails to honor SEC_STD_WRITE_OWNER bit with the acl_xattr module
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 3.5.6
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 13085
  Show dependency treegraph
 
Reported: 2011-01-26 02:36 UTC by Matthieu Patou
Modified: 2017-11-01 08:34 UTC (History)
2 users (show)

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
WIP patch for master (3.14 KB, patch)
2017-10-04 20:38 UTC, Ralph Böhme
no flags Details
Patch for 4.6 backported from master (24.84 KB, patch)
2017-10-10 11:44 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.7 cherry-picked from master (28.02 KB, patch)
2017-10-10 11:45 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.6 backported from master (27.99 KB, patch)
2017-10-15 09:26 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.7 backported from master (31.17 KB, patch)
2017-10-15 09:26 UTC, Ralph Böhme
jra: review+
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 ?
Comment 24 Ralph Böhme 2017-10-04 10:47:58 UTC
Looks like with the change introduced with this patchset, we implement give ownership semantics with vfs_acl_xattr|tdb for any user. Testing suggests that this is not the way Windows behaves here.

It seems Windows implements SEC_STD_WRITE_OWNER *filesystem permission* for normal users as only allowing "take ownership", only users with SEC_PRIV_RESTORE *privilege* are allowed to "give ownership".

Reproducer:
- Samba 4 AD DC
- Windows 10 member server (IP 10.10.11.102)
- on the Windows 10 member server a share "share" with a file "WHATSNEW.txt" with FULL permissions for user "HILLHOUSE\administrator"
- BUILTIN\administrators on the Windows 10 system initially has the default priveleges for the builtin admins group including SeTakeOwnershipPrivilege and SeRestorePrivilege

Test showing that giving ownership away works with SEC_STD_WRITE_OWNER on the file *and* SEC_PRIV_RESTORE:

$ bin/rpcclient -U HILLHOUSE\\administrator%Passw0rd 10.10.11.102 -c "lsaenumacctrights S-1-5-32-544" | egrep "SeTakeOwnershipPrivilege|SeRestorePrivilege"
        SeTakeOwnershipPrivilege
        SeRestorePrivilege
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" | grep OWNER
OWNER:WIN10\test2
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" -C WIN10\\test1
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" | grep OWNER
OWNER:WIN10\test1

Removing SEC_PRIV_RESTORE, giving away ownership now fails:

$ bin/rpcclient -U HILLHOUSE\\administrator%Passw0rd 10.10.11.102 -c "lsadelpriv S-1-5-32-544 SeRestorePrivilege"
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" -C WIN10\\test2
ERROR: security descriptor set failed: NT_STATUS_INVALID_OWNER

Readding, works again:

$ bin/rpcclient -U HILLHOUSE\\administrator%Passw0rd 10.10.11.102 -c "lsaaddpriv S-1-5-32-544 SeRestorePrivilege"
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" -C WIN10\\test2
$ bin/smbcacls -W HILLHOUSE -U Administrator%Passw0rd -m smb3 //10.10.11.102/share "WHATSNEW.txt" | grep OWNER
OWNER:WIN10\test2

Note the error coce NT_STATUS_INVALID_OWNER. This is described in MS-FSA 2.1.5.16:

  If InputBuffer.OwnerSid is not a valid owner SID for a file in the objectstore, as determined in an implementation-specific manner, the object store MUST return STATUS_INVALID_OWNER.

Man, how I love "implementation-specific manners"!
Comment 25 Ralph Böhme 2017-10-04 20:38:14 UTC
Created attachment 13652 [details]
WIP patch for master

Something like this...
Comment 26 Jeremy Allison 2017-10-04 21:13:07 UTC
(In reply to Ralph Böhme from comment #25)

Don't you still need to allow the original behaviour if the current token has SeRestore privilege ?
Comment 27 Ralph Böhme 2017-10-05 07:52:37 UTC
(In reply to Jeremy Allison from comment #26)
That is already dealt with in posix_acls.c:try_chown().
Comment 28 Ralph Böhme 2017-10-10 11:44:53 UTC
Created attachment 13674 [details]
Patch for 4.6 backported from master
Comment 29 Ralph Böhme 2017-10-10 11:45:21 UTC
Created attachment 13675 [details]
Patch for 4.7 cherry-picked from master
Comment 30 Jeremy Allison 2017-10-10 21:51:52 UTC
Re-assigning to Karo/metze for release in 4.6.next, 4.7.next.
Comment 31 Ralph Böhme 2017-10-13 12:43:46 UTC
Comment on attachment 13674 [details]
Patch for 4.6 backported from master

See bug 13085.
Comment 32 Ralph Böhme 2017-10-13 12:43:55 UTC
Comment on attachment 13675 [details]
Patch for 4.7 cherry-picked from master

See bug 13085.
Comment 33 Ralph Böhme 2017-10-13 12:47:19 UTC
There's a problem in the test script test_give_owner.sh:

/bin/sh on sn-devel-144 is dash which has a XSI compliant echo builtin. That means echo will always interpret escape sequences in strings, eg

$ /bin/sh -c 'echo -n "\a" | hexdump -C'
00000000  07                                                |.|

There's no way to prevent the echo command from doing this.

In the script test_give_owner.sh echo was used to print an ACE which will contain a DOMAIN\USER substring. If the first character of USER is a valid escape sequence, the resulting string was interpreted as an escape sequence:

$ /bin/sh -c 'echo "FILESERVER\asn"'
FILESERVERsn

The only portable way to print a string without interpreting embedded escape sequences seems to be using printf:

$ /bin/sh -c 'printf "%s\n" "FILESERVER\asn"'
FILESERVER\asn
Comment 34 Ralph Böhme 2017-10-15 09:26:02 UTC
Created attachment 13689 [details]
Patch for 4.6 backported from master

Patch for 4.6 including the fix for the selftest regression.
Comment 35 Ralph Böhme 2017-10-15 09:26:22 UTC
Created attachment 13690 [details]
Patch for 4.7 backported from master
Comment 36 Ralph Böhme 2017-10-15 09:27:01 UTC
Comment on attachment 13690 [details]
Patch for 4.7 backported from master

Patch for 4.7 including the fix for the selftest regression.
Comment 37 Jeremy Allison 2017-10-16 20:41:02 UTC
Karolin please push to 4.6.next, 4.7.next.
Comment 38 Karolin Seeger 2017-10-24 07:56:55 UTC
(In reply to Jeremy Allison from comment #37)
Pushed to autobuild-v4-{7,6}-test.
Comment 39 Karolin Seeger 2017-11-01 08:34:23 UTC
(In reply to Karolin Seeger from comment #38)
Pushed to both branches.
Closing out bug report.

Thanks!