smb2.acls.ownerrights failure

[Jeremy Allison]

On Thu, Dec 01, 2016 at 10:34:07AM -0800, Jeremy Allison wrote:
> On Thu, Dec 01, 2016 at 02:43:43PM +0530, Shilpa K wrote:
> > Hi Jeremy,
> > 
> > I have pasted below the git diff for samba-master branch.
> 
> Thanks a *lot*. I'm going to do some work on this to
> ensure I understand exactly what the semantics of setting
> an ACE for sid "S-1-3-4" with restricted access, followed
> by an ACE for the actual owner sid with SEC_RIGHTS_FILE_ALL.
> 
> There's more clarification I need on that (I'll test
> against Windows to be sure).

OK, I've run this against Win2K12 and I *think* I see what
you're trying to do, but IMHO this test doesn't
do what you think it does :-).

Your test array is:

         const struct {
                uint32_t owner_flags;
                uint32_t owner_rights;
        } test_perm[] = {
                {
                        0,
                        0
                },
                {
                        SEC_ACE_TYPE_ACCESS_ALLOWED,
                        SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL,
                },
                {
                        SEC_ACE_TYPE_ACCESS_ALLOWED,
                        SEC_STD_WRITE_DAC,
                },
                {
                        SEC_ACE_TYPE_ACCESS_DENIED,
                        SEC_STD_WRITE_DAC,
                },
        };

but when you replace the new ACL on the file, you always
append an owner_sid/SEC_ACE_TYPE_ACCESS_ALLOWED/SEC_RIGHTS_FILE_ALL
ACE.

               sd = security_descriptor_dacl_create(tctx,
                                                0, NULL, NULL,
                                                owner_rights_sid,
                                                test_perm[i].owner_flags,
                                                test_perm[i].owner_rights,
                                                0,
                                                owner_sid,
						SEC_ACE_TYPE_ACCESS_ALLOWED,
                                                SEC_RIGHTS_FILE_ALL,
                                                0,
                                                NULL);

This means all of your get/set calls over all of
these entries always work - they always return
NT_STATUS_OK. For the whatever you set as the
first ACCESS_ALLOWED ACE is essentially ignored
as the logic always gets to the second ACE which
allows everything.

I'm assuming that's not what you wanted because
of this if clause:

                if (test_perm[i].owner_rights & (SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL)) {
                        CHECK_STATUS(status, NT_STATUS_OK);
                } else if (test_perm[i].owner_rights & SEC_STD_WRITE_DAC) {
                        CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
                }

But this if clause is wrong. The:

                } else if (test_perm[i].owner_rights & SEC_STD_WRITE_DAC) {
                        CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
                }

clause will never be selected. The first clause is selected
for the last 3 entries in the array (as it gets selected
if *either* SEC_STD_WRITE_DAC *or* SEC_STD_READ_CONTROL
are set in the owner_rights field). The only case where
the first clause isn't selected is in the first entry,
where owner_flags == 0, owner_rights == 0.

But in that case the second clause *still* isn't selected
as (test_perm[i].owner_rights & SEC_STD_WRITE_DAC) is false.

If you intended the last case in your array to get NT_STATUS_ACCESS_DENIED,
sorry - it doesn't (as I've confirmed in wireshark). All these
cases get NT_STATUS_OK.

I'm not sure if this code is just left over from your
experiments, or you intended it to show something.

Can you let me know ?

The only part of the test that attempts to show the Samba bug
is the last part - the :

                if((test_perm[i].owner_flags & SEC_ACE_TYPE_ACCESS_DENIED)) {
                        smb2_util_close(tree, handle);
                        io.in.desired_access = 0x00020080;
                        io.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
                        status = smb2_create(tree, tctx, &io);

code.

This does shows that when an ACL of:

[0] S-1-3-4/DENY_ACE/WRITE_DAC
[1] <owner>/ALLOW_ACE/SEC_RIGHTS_FILE_ALL

is set, that trying to write a new ACL
to the file returns ACCESS_DENIED.

However, you didn't open the second handle
with WRITE_DAC request, so it looks
to me that the ACCESS_DENIED is due to the
handle not having WRITE_DAC permission, and
maybe not the ACL you had set.

Can you explain to me EXACTLY what
you are trying to do with this test ?

Can you walk me through the logic step
by step so I understand what the test
is supposed to do ?

I still think we have a bug here, but
this isn't the regression test to
show it.

Cheers,

	Jere,y.