smb2.acls.ownerrights failure

[Shilpa K]

Hi Jeremy,

I agree with you that  the test is not complete. First, we wanted to verify
that what  was returned matches with what was set. Secondly, I think we
shouldn't be setting owner ACE as we purely wanted to test owner rights
when there is no explicit ACE for owner to grant any access. In any case, I
think the test does show that there is problem in se_access_check() even
though it is not complete. So, yes, it is not ready to be submitted to the
branch.

Thanks,
Shilpa

On Fri, Dec 2, 2016 at 6:30 AM, Jeremy Allison <jra at samba.org> wrote:

> 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.
>