Bug 8795 - Samba does not handle the Owner Rights permissions at all
Samba does not handle the Owner Rights permissions at all
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.3
All All
: P5 normal
: ---
Assigned To: Volker Lendecke
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-06 06:09 UTC by Richard Sharpe
Modified: 2012-05-10 08:48 UTC (History)
0 users

See Also:


Attachments
A potential patch to fix this possible problem (3.55 KB, patch)
2012-03-06 06:12 UTC, Richard Sharpe
no flags Details
More correct Owner Rights patch (4.35 KB, patch)
2012-03-10 17:13 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-03-06 06:09:27 UTC
Here http://technet.microsoft.com/en-us/library/dd125370%28v=WS.10%29.aspx
it suggests that if an ACL on an object contains the Owner Rights
principal (S-1-3-4) and the permissions do not contain WRITE_DAC and
READ_CONTROL then the current handling of se_access_check
(libcli/security/access_check.c) is incorrect.

The solution seems simple. Defer the check for SEC_STD_WRITE_DAC and
SEC_STD_READ_CONTROL until after we have scanned the ACL and save the
permissions associated with S-1-3-4 in a variable that starts out as
~0 and is used with SEC_STD_WRITE_DAC and SEC_STD_READ_CONTROL to
determine the default permissions that should apply and therefore
those bits that should be removed ...

Thoughts? I guess I need to fire up a Windows Server 2008 VM to see if
this applies to file objects, but I suspect it does.
Comment 1 Richard Sharpe 2012-03-06 06:12:21 UTC
Created attachment 7360 [details]
A potential patch to fix this possible problem

This is a patch that possibly implements the required functionality.
Comment 2 Richard Sharpe 2012-03-07 16:35:44 UTC
If there are no objections here I would like to push these changes in a few days.

The only issues I see are whether or not we should introduce a new parameter to control this behavior, and the full behavior of Windows with these ACEs.

I don't think we need a new parameter because earlier versions of Windows probably ignore such ACEs and might not even be able to apply them.

I will explore the full behavior of Windows 2008 here over the next few days and detail my findings here.
Comment 3 Jeremy Allison 2012-03-07 17:46:12 UTC
I'd like to review first please - I promise to get to it this week.

Jeremy.
Comment 4 Richard Sharpe 2012-03-07 22:21:28 UTC
(In reply to comment #3)
> I'd like to review first please - I promise to get to it this week.
> 
> Jeremy.

OK. I also want to check out exactly what happens on Win2K08. There are two things I want to check. The first is the actual functioning and the second is what happens when take ownership occurs ... that is, when a privileged user takes ownership what happens ...
Comment 5 Richard Sharpe 2012-03-07 23:26:25 UTC
My first test panned out.

I created a share on a Win2K08 DC. Added the Owner Rights ACE giving the owner only Read Permissions permission as an inheritable ACE.

Then, on a Win7 client, mapped the share and created a new file.

The user was testu1. Testu1 was the owner and the Owner Rights ACE had been inherited. (Clearly, most likely, my patch is incorrect because the code that handles inheritance may have to be revisited.)

I could bring up the Security tab in the properties dialog box, but I could not remove any ACEs. I could bring up the Edit dialog boxes but could not actually change any permissions. I tried to add new permissions, but when I went to save I was told:

   Unable to save permission changes to New Text Document (2).

   Access is denied.
Comment 6 Richard Sharpe 2012-03-07 23:28:44 UTC
Just to complete the picture, when I tried to edit existing permissions, it allows me to set the bits but gave me the same error message as above when I went to save the permissions.

Next up. A check to see what happens when someone takes ownership.
Comment 7 Jeremy Allison 2012-03-08 01:00:51 UTC
This code looks right (IMHO).
Comment 8 Jeremy Allison 2012-03-08 01:03:46 UTC
Hmmm. What happens if there is more than one OWNER_RIGHTS sid ACE entry ? Right now the patch replaces, maybe it should OR them together. What if it's an ACCESS_DENIED entry - it should be subtracted from the rights granted.

This isn't quite so simple...

Jeremy.
Comment 9 Jeremy Allison 2012-03-08 01:08:03 UTC
I think we need two bitmasks - one owner_explicitly_allowed set of bits, initialized to (SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL), and another owner_explicitly_denied set of bits, initialized to zero and OR'ed in if the OWNER_RIGHTS ace entry is ACCESS_DENIED. We then combine the two together in the same way as we do for bits_remaining.
Comment 10 Jeremy Allison 2012-03-08 01:18:46 UTC
Can you write some smbcacls test that adds more than one OWNER_RIGHT ACE entry to a security descrptor on a file on a Windows hare ? That should mess up the Microsoft code if they didn't think the algorithm though :-).

Maybe we should ask this on dochelp...
Comment 11 Richard Sharpe 2012-03-08 15:24:26 UTC
Yes, I agree that more research is needed ...

I will try adding multiple ACEs of the type you mention later today. I imagine that the Windows Explorer will not let that happen but smbcacls will probably allow me to do so.
Comment 12 Richard Sharpe 2012-03-08 15:25:29 UTC
(In reply to comment #10)
> Can you write some smbcacls test that adds more than one OWNER_RIGHT ACE entry
> to a security descrptor on a file on a Windows hare ? That should mess up the
> Microsoft code if they didn't think the algorithm though :-).
> 
> Maybe we should ask this on dochelp...

What is dochelp?

Let me try the tests and then we can ask if we need clarification.
Comment 13 Richard Sharpe 2012-03-09 01:29:52 UTC
(In reply to comment #10)
> Can you write some smbcacls test that adds more than one OWNER_RIGHT ACE entry
> to a security descrptor on a file on a Windows hare ? That should mess up the
> Microsoft code if they didn't think the algorithm though :-).

OK, so the maximally confusing case would seem to be if there is an earlier Owner Rights ACE in the list that removes WRITE_DAC and then a later one that adds it back (or perhaps vice versa).

Let me try that to see what happens. smbcacls is quite happy to add these by name.
Comment 14 Jeremy Allison 2012-03-09 01:35:46 UTC
Can you add a DENY_ACE entry as well, so we can see what effect that might have ?
Comment 15 Richard Sharpe 2012-03-09 01:59:17 UTC
(In reply to comment #14)
> Can you add a DENY_ACE entry as well, so we can see what effect that might have
> ?

Sure.

Preliminary results are confusing as I had a mixture of inherited Owner Rights and non-inherited Owner Rights, which seemed to confuse the File Mangler (Windows Explorer) because when I asked to edit the Owner Rights permission it allowed me to do so, but then deleted everything except for the Owner Rights entry.

More checking required.
Comment 16 Richard Sharpe 2012-03-09 16:09:33 UTC
I think that the following demonstrates that Windows 2K08 only processes the last Owner Rights ACE that it sees:

[root@rjs-dev ~]# smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt -Utestu1%Passw0rd1
REVISION:1
CONTROL:0x8004
OWNER:RJSTEST\testu1
GROUP:RJSTEST\Domain Users
ACL:OWNER RIGHTS:ALLOWED/0x0/P
ACL:OWNER RIGHTS:ALLOWED/0x0/0x00120000
ACL:RJSTEST\Administrator:ALLOWED/I/FULL
ACL:BUILTIN\Administrators:ALLOWED/I/FULL
ACL:NT AUTHORITY\SYSTEM:ALLOWED/I/FULL
ACL:RJSTEST\Domain Admins:ALLOWED/I/FULL
ACL:RJSTEST\Domain Users:ALLOWED/I/0x00100027
[root@rjs-dev ~]# smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt -Utestu1%Passw0rd1 -D ACL:'OWNER RIGHTS':ALLOWED/0/0x40000
Failed to open \New Text Document (2).txt: NT_STATUS_ACCESS_DENIED

Note that there are two Owner Rights ACEs in that list. The first grants WRITE_DAC the second grants READ_PERMISSIONS.

The user testu1 was able to read his SD but unable to change it. If Windows merged the contents of each of these ACEs, then things would be different.

I will have to do another test to prove this, though, which is to reverse the order of those two ACEs. I would expect that testu1 would not be able to read his SD in that case.
Comment 17 Richard Sharpe 2012-03-09 19:57:47 UTC
I'm still not happy that I have a smoking gun, so I will go back and try that some more times.

I will start with a file that has no Owner Rights ACEs. Then I will add one as the Owner that takes away WRITE_DAC. That should prevent the owner from adding any more. Then I will try adding one allowing WRITE_DAC along with a separate one.

Once I think I understand it, I will send email to dochelp.at.microsoft.com to see if they can clarify the intended semantics.

I think we need to implement exactly the same semantics as Windows does here ...
Comment 18 Richard Sharpe 2012-03-09 20:48:29 UTC
Damn. The version of smbcacls I am using tries to open the file that I want to add ACEs to with WRITE_OWNER as well as WRITE_DAC.

That is too much!
Comment 19 Richard Sharpe 2012-03-09 20:54:31 UTC
(In reply to comment #18)
> Damn. The version of smbcacls I am using tries to open the file that I want to
> add ACEs to with WRITE_OWNER as well as WRITE_DAC.
> 
> That is too much!

OK, I notice this comment from smbcacls.c:

           I need to check that setting a SD with no owner set works against WNT
           and W2K. JRA.

as justification for asking for WRITE_OWNER as well as WRITE_DAC ...

Perhaps that needs to change.
Comment 20 Richard Sharpe 2012-03-09 21:33:26 UTC
(In reply to comment #16)
> I think that the following demonstrates that Windows 2K08 only processes the
> last Owner Rights ACE that it sees:
> 
> [root@rjs-dev ~]# smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt
> -Utestu1%Passw0rd1
> REVISION:1
> CONTROL:0x8004
> OWNER:RJSTEST\testu1
> GROUP:RJSTEST\Domain Users
> ACL:OWNER RIGHTS:ALLOWED/0x0/P
> ACL:OWNER RIGHTS:ALLOWED/0x0/0x00120000
> ACL:RJSTEST\Administrator:ALLOWED/I/FULL
> ACL:BUILTIN\Administrators:ALLOWED/I/FULL
> ACL:NT AUTHORITY\SYSTEM:ALLOWED/I/FULL
> ACL:RJSTEST\Domain Admins:ALLOWED/I/FULL
> ACL:RJSTEST\Domain Users:ALLOWED/I/0x00100027
> [root@rjs-dev ~]# smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt
> -Utestu1%Passw0rd1 -D ACL:'OWNER RIGHTS':ALLOWED/0/0x40000
> Failed to open \New Text Document (2).txt: NT_STATUS_ACCESS_DENIED
> 
> Note that there are two Owner Rights ACEs in that list. The first grants
> WRITE_DAC the second grants READ_PERMISSIONS.
> 
> The user testu1 was able to read his SD but unable to change it. If Windows
> merged the contents of each of these ACEs, then things would be different.
> 
> I will have to do another test to prove this, though, which is to reverse the
> order of those two ACEs. I would expect that testu1 would not be able to read
> his SD in that case.

This comment was WRONG!

The problem was that smbcacls was asking for WRITE_OWNER permissions, which could not be granted.

When I modified smbcacls to not ask for WRITE_OWNER permissions, it became clear that Win2K08 does merge the separate OWNER RIGHTS ALLOW ACEs.

Next up, a test of DENY entries.
Comment 21 Richard Sharpe 2012-03-09 21:50:25 UTC
My final test suggests that Windows correctly handles multiple OWNER RIGHTS ACEs and correctly handles DENY and ALLOW entries.

Thus, the semantics seem to be:

Accumulate all the ALLOW OWNER RIGHTS but remove any DENY bits. Then apply the resulting permissions.

Thus your objection to my initial patch is correct. I will have to re-do it along the lines of your suggestion. We have to remember, however, that the default behavior applies if there are no OWNER RIGHTS entries. I guess that means I also need to check whether a single DENIED bit removes that bit from the default rights or not.
Comment 22 Richard Sharpe 2012-03-10 05:30:40 UTC
The result of my latest test is that a single DENIED entry, regardless of the bits, removes the default OWNER permissions of READ_PERMISSIONS (called READ_CONTROL, I think) and WRITE_DAC.

Thus, the semantics seem to be:

No OWNER RIGHTS ACEs means owner gets at least READ_CONTROL and WRITE_DAC or'ed with whatever permissions are explicitly granted.

If there are any OWNER_RIGHTS, then the ALLOWED entries are or'ed together and then any explicitly ALLOWED bits are or'ed in and then DENIED entries remove bits, and the remaining bits are what the user gets.

I will create a more correct patch that expresses this.
Comment 23 Richard Sharpe 2012-03-10 05:38:34 UTC
One final thing. 

A single 'OWNER RIGHTS':ALLOWED/0/0 or 'OWNER RIGHTS':DENIED/0/0 gives the OWNER no default rights at all.
Comment 24 Richard Sharpe 2012-03-10 05:43:55 UTC
The following also shows that any bit that is DENIED cannot be put back by an ALLOWED entry.

[rsharpe@rjs-dev source3]$ ./bin/smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt -Utestu1%Passw0rd1
params.c:OpenConfFile() - Unable to open configuration file "/usr/local/samba/etc/smb.conf":
	No such file or directory
REVISION:1
CONTROL:SR|DP
OWNER:RJSTEST\testu1
GROUP:RJSTEST\Domain Users
ACL:OWNER RIGHTS:DENIED/0x0/P
ACL:OWNER RIGHTS:ALLOWED/0x0/P
ACL:OWNER RIGHTS:ALLOWED/0x0/0x00120000
ACL:RJSTEST\Administrator:ALLOWED/I/FULL
ACL:BUILTIN\Administrators:ALLOWED/I/FULL
ACL:NT AUTHORITY\SYSTEM:ALLOWED/I/FULL
ACL:RJSTEST\Domain Admins:ALLOWED/I/FULL
ACL:RJSTEST\Domain Users:ALLOWED/I/0x00100027
[rsharpe@rjs-dev source3]$ ./bin/smbcacls //192.168.2.41/share1 New\ Text\ Document\ \(2\).txt -Utestu1%Passw0rd1 -a ACL:'OWNER RIGHTS':DENIED/0/0x40000
params.c:OpenConfFile() - Unable to open configuration file "/usr/local/samba/etc/smb.conf":
	No such file or directory
Failed to open \New Text Document (2).txt: NT_STATUS_ACCESS_DENIED
---------------------------

Note also that these were all done with my slightly modified version of smbcacls. I will create a bug for that.
Comment 25 Richard Sharpe 2012-03-10 16:32:08 UTC
One more thing to test.

What is the effect of the OWNER RIGHTS ACE if there is an explicit deny entry in the ACL for any of the bits in the OWNER RIGHTS ACE.
Comment 26 Richard Sharpe 2012-03-10 17:02:30 UTC
(In reply to comment #25)
> One more thing to test.
> 
> What is the effect of the OWNER RIGHTS ACE if there is an explicit deny entry
> in the ACL for any of the bits in the OWNER RIGHTS ACE.

And the answer is that explicitly denied bits always override allowed bits.
Comment 27 Richard Sharpe 2012-03-10 17:13:14 UTC
Created attachment 7369 [details]
More correct Owner Rights patch

Attached is a second patch for review. I think this is more correct that the previous one and is based on testing Windows Server 2008 for what it does.

If it is approved I can push it from my repos. If there is still criticism I will make changes.
Comment 28 Richard Sharpe 2012-05-10 08:48:32 UTC
Fixed in the checkin:

commit 1e8141f40ae7b67a45906f26483caff0a7cca7ed
Author: Richard Sharpe <realrichardsharpe@gmail.com>
Date:   Fri Mar 9 14:54:38 2012 -0800

    Fix bug #8797 - Samba does not correctly handle DENY ACEs when privileges ap
    Signed-off-by: Jeremy Allison <jra@samba.org>
    
    Autobuild-User: Jeremy Allison <jra@samba.org>
    Autobuild-Date: Sat Mar 10 01:33:45 CET 2012 on sn-devel-104