smb2.acls.ownerrights failure

[Jeremy Allison]

On Fri, Nov 18, 2016 at 06:21:58PM +0530, Shilpa K wrote:
> Hello,
> 
> We have a smbtorture test smb2.acls.onwerrights for testing for permissions
> for owner SID. Test case is given below. In one scenario, owner is denied
> WRITE_DAC and the administrator has full access. I am executing the test
> using administrator credentials. In one instance, create request is sent
> with access_mask SEC_RIGHTS_FILE_READ while the owner is denied WRITE_DAC.
> In Samba 4.5, this test case fails. Upon some investigation, I found the
> issue to be in:
> 
> 
> /* The owner always gets owner rights as defined above. */
> if (security_token_has_sid(token, sd->owner_sid)) {
> if (owner_rights_default) {
> /*
> * Just remove them, no need to check if they are
> * there.
> */
> bits_remaining &= ~(SEC_STD_WRITE_DAC |
> SEC_STD_READ_CONTROL);
> } else {
> bits_remaining &= ~owner_rights_allowed;
> bits_remaining |= owner_rights_denied;      ---> issue here
> }
> }
> 
> Because I am executing the test administrator, bits_remaining becomes 0
> until the above code path is reached. At the following line, bits_remaining
> becomes 0x40000:
> 
> bits_remaining |= owner_rights_denied;
> 
> And following message is logged in the Samba log file:
> 
> [2016/11/18 03:22:44.464327, 10, pid=9742, effective(586678772, 586678784),
> real(0, 0)] ../source3/smbd/open.c:168(smbd_check_access_rights)
>   smbd_check_access_rights: file smb2-testsd/inheritance requesting 0x10000
> returning 0x40000 (NT_STATUS_ACCESS_DENIED)
> 
> If I change the above code to below line, this issue is resolved:
> 
> bits_remaining |= (owner_rights_denied & access_desired);
> 
> 
> Can you comment on the above fix?

OK, I've looked at this really carefully, and I believe
you are correct.

The code inside se_access_check() is incorrect and in this
case is OR'ing in the bits from any SEC_ACE_TYPE_ACCESS_DENIED type
ACE with sid S-1-3-4 (Owner Rights) if the requestor token matches
the object owner sid, without checking if the requestor actually
was requesting those bits at all.

We probably haven't noticed this before as it's unusual
to have an ACE type SEC_ACE_TYPE_ACCESS_DENIED and sid of
S-1-3-4 (Owner Rights) set on an object.

Can you send me a git-am patch containing your smb2.acl.ownerrights
code ? I'll need to put add this as a regression test for
this fix.

Cheers,

	Jeremy.