Bug 8768 - Samba does not honor SeTakeOwnershipPrivilege when file opened with SEC_STD_WRITE_OWNER
Samba does not honor SeTakeOwnershipPrivilege when file opened with SEC_STD_W...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-20 05:51 UTC by Richard Sharpe
Modified: 2012-02-26 20:01 UTC (History)
0 users

See Also:
rsharpe: review+
rsharpe: review+
rsharpe: review+


Attachments
A patch that seems to work for Samba 3.5.12 (497 bytes, application/octet-stream)
2012-02-20 15:59 UTC, Richard Sharpe
no flags Details
git-am fix for 3.6.next (1.21 KB, patch)
2012-02-22 22:38 UTC, Jeremy Allison
jra: review+
Details
A capture showing what Windows tries ... packet 48 should succeed (25.00 KB, application/octet-stream)
2012-02-23 03:30 UTC, Richard Sharpe
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2012-02-20 05:51:45 UTC
If a client tries to open a file with SEC_STD_WRITE_OWNER in the access_mask but the ACL does not allow such access while the user's token contains SeTakeOwnershipPrivilege, Samba still denies the access.

This can be fixed with the following patch:

diff --git a/libcli/security/access_check.c b/libcli/security/access_check.c
index 1b02a86..a9b618f 100644
--- a/libcli/security/access_check.c
+++ b/libcli/security/access_check.c
@@ -205,6 +205,11 @@ NTSTATUS se_access_check(const struct security_descriptor *
                bits_remaining &= ~(SEC_RIGHTS_PRIV_BACKUP);
        }
 
+       if ((bits_remaining & SEC_STD_WRITE_OWNER) &&
+            security_token_has_privilege(token, SEC_PRIV_TAKE_OWNERSHIP)) {
+               bits_remaining &= ~(SEC_STD_WRITE_OWNER);
+       }
+
        /* a NULL dacl allows access */
        if ((sd->type & SEC_DESC_DACL_PRESENT) && sd->dacl == NULL) {
                *access_granted = access_desired;

----------------

I have tested this with a similar patch on Samba 3.5.12 and Windows says that I do not have privilege to view the ACL/Permissions but that I can take ownership and when I take ownership, things work as expected.
Comment 1 Richard Sharpe 2012-02-20 15:59:13 UTC
Created attachment 7333 [details]
A patch that seems to work for Samba 3.5.12

The attached patch works for Samba 3.5.12. With it, Windows does the right things and without it things do not work.
Comment 2 Richard Sharpe 2012-02-20 16:00:23 UTC
I have a capture showing what the client tries to do when you get the properties of a file that you have no permissions to access, but I will need to get permission to post it here.
Comment 3 Richard Sharpe 2012-02-20 16:00:56 UTC
If no one objects to the change above I will push it to master in a day or so.
Comment 4 Jeremy Allison 2012-02-22 22:38:24 UTC
Created attachment 7338 [details]
git-am fix for 3.6.next

Richard, this is your master fix cherry-picked to 3.6.next.
Please +1 and I'll re-assign to Karolin for inclusion in 3.6.next.
Comment 5 Richard Sharpe 2012-02-23 03:30:13 UTC
Created attachment 7341 [details]
A capture showing what Windows tries ... packet 48 should succeed

In the attached capture packet 48 is the one that should succeed because the session credentials are TESTAD\Administrator ...

Windows tries quite a few different ways to open the file ...
Comment 6 Jeremy Allison 2012-02-23 20:48:44 UTC
Richard, can you change the '?' to '+' on this link:

https://bugzilla.samba.org/attachment.cgi?id=7338&action=edit

to show you approve, and I'll re-assign to Karolin for inclusion in 3.6.next.

Jeremy.
Comment 7 Richard Sharpe 2012-02-23 20:54:24 UTC
Comment on attachment 7338 [details]
git-am fix for 3.6.next

Hmmm, I do not understand how to change it to a plus, but I want to.
Comment 8 Jeremy Allison 2012-02-23 21:09:24 UTC
Comment on attachment 7338 [details]
git-am fix for 3.6.next

Click on the dropdown box next on the right of the "review" text, and change it from '?' to '+'. You'll need to be logged into bugzilla to do this.
Comment 9 Richard Sharpe 2012-02-23 21:20:13 UTC
(In reply to comment #8)
> Comment on attachment 7338 [details]
> git-am fix for 3.6.next
> 
> Click on the dropdown box next on the right of the "review" text, and change it
> from '?' to '+'. You'll need to be logged into bugzilla to do this.

Yeah, I have done that repeatedly ... doesn't seem to help ...
Comment 10 Richard Sharpe 2012-02-23 21:21:53 UTC
OK, I see what the problem is.

The reviewer has to be realrichardsharpe@gmail.com :-)
Comment 11 Richard Sharpe 2012-02-24 00:45:43 UTC
(In reply to comment #8)
> Comment on attachment 7338 [details]
> git-am fix for 3.6.next
> 
> Click on the dropdown box next on the right of the "review" text, and change it
> from '?' to '+'. You'll need to be logged into bugzilla to do this.

Just so you are aware, it seems I cannot mark it reviewed until you change the reviewer name to match my logon handle ...
Comment 12 Jeremy Allison 2012-02-24 22:28:48 UTC
Comment on attachment 7338 [details]
git-am fix for 3.6.next

Ok, I don't know what your review handle is - I set it to the email address you opened the bug with.

But as you're trying to review it as '+' anyway I'm going to give to Karolin for inclusion.
Comment 13 Jeremy Allison 2012-02-24 22:29:15 UTC
Changing owner to Karolin for inclusion in 3.6.4.
Jeremy.
Comment 14 Karolin Seeger 2012-02-26 20:01:50 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!