smb2.acls.ownerrights failure
Shilpa K
shilpa.krishnareddy at gmail.com
Thu Dec 8 07:48:48 UTC 2016
Thanks Jeremy. BTW, I wonder if the following in se_access_check() also
needs similar change?
/* Explicitly denied bits always override */
bits_remaining |= explicitly_denied_bits;
Regards,
Shilpa
On Thu, Dec 8, 2016 at 5:07 AM, Jeremy Allison <jra at samba.org> wrote:
> On Wed, Dec 07, 2016 at 05:27:53PM +0530, Shilpa K wrote:
> > 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.
>
> OK, thanks for the info. I think I'll re-work this into a specific
> regression test that shows we're mishandling the owner_rights bits
> to go with the patch and leave it at that.
>
> Cheers,
>
> Jeremy.
>
> > 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.
> > >
>
More information about the samba-technical
mailing list