smb2.acls.ownerrights failure
Jeremy Allison
jra at samba.org
Thu Dec 1 18:34:07 UTC 2016
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).
Cheers,
Jeremy.
> Regards,
> Shilpa
>
> diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c
> index beb5abf..192d403 100644
> --- a/source4/torture/smb2/acls.c
> +++ b/source4/torture/smb2/acls.c
> @@ -2087,6 +2087,142 @@ done:
> return ret;
> }
>
> +static bool test_owner_rights(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + NTSTATUS status;
> + struct smb2_create io;
> + const char *dname = BASEDIR "\\inheritance";
> + const char *fname1 = BASEDIR "\\inheritance\\testfile";
> + bool ret = true;
> + struct smb2_handle handle, handle2;
> + union smb_fileinfo q;
> + union smb_setfileinfo set;
> + struct security_descriptor *sd, *sd_orig=NULL;
> + const char *owner_sid;
> + const char *owner_rights_sid = "S-1-3-4";
> +
> + torture_comment(tctx, "TESTING OWNER RIGHTS\n");
> +
> + if (!smb2_util_setup_dir(tctx, tree, BASEDIR))
> + return false;
> +
> + ZERO_STRUCT(io);
> + io.level = RAW_OPEN_SMB2;
> + io.in.create_flags = 0;
> + io.in.desired_access = SEC_RIGHTS_FILE_ALL;
> + io.in.create_options = NTCREATEX_OPTIONS_DIRECTORY;
> + io.in.file_attributes = FILE_ATTRIBUTE_DIRECTORY;
> + io.in.share_access = 0;
> + io.in.alloc_size = 0;
> + io.in.create_disposition = NTCREATEX_DISP_CREATE;
> + io.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS;
> + io.in.security_flags = 0;
> + io.in.fname = dname;
> +
> + status = smb2_create(tree, tctx, &io);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + handle = io.out.file.handle;
> +
> + torture_comment(tctx, "get the original sd\n");
> + q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
> + q.query_secdesc.in.file.handle = handle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + sd_orig = q.query_secdesc.out.sd;
> +
> + owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
> +
> + torture_comment(tctx, "owner_sid is %s\n", owner_sid);
> + int i = 0;
> + 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,
> + },
> + };
> + for (i=0;i<ARRAY_SIZE(test_perm);i++) {
> + 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);
> + //sd->type |= SEC_DESC_DACL_AUTO_INHERITED |
> SEC_DESC_DACL_AUTO_INHERIT_REQ;
> +
> + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
> + set.set_secdesc.in.file.handle = handle;
> + set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
> + set.set_secdesc.in.sd = sd;
> + status = smb2_setinfo_file(tree, &set);
> + printf("Iteration is %d\n", i);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + q.query_secdesc.in.file.handle = handle;
> + q.query_secdesc.in.secinfo_flags = SECINFO_DACL;
> + status = smb2_getinfo_file(tree, tctx, &q);
> + 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);
> + }
> +
> + 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);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + handle = io.out.file.handle;
> +
> + sd = q.query_secdesc.out.sd;
> + sd->dacl->aces[1].access_mask =
> SEC_RIGHTS_FILE_READ;
> + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
> + set.set_secdesc.in.file.handle = handle;
> + set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
> + set.set_secdesc.in.sd = sd;
> + status = smb2_setinfo_file(tree, &set);
> + CHECK_STATUS(status, NT_STATUS_ACCESS_DENIED);
> + }
> + }
> +
> +done:
> + torture_comment(tctx, "put back original sd\n");
> + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
> + set.set_secdesc.in.file.handle = handle;
> + set.set_secdesc.in.secinfo_flags = SECINFO_DACL;
> + set.set_secdesc.in.sd = sd_orig;
> + status = smb2_setinfo_file(tree, &set);
> + smb2_util_close(tree, handle);
> + smb2_util_rmdir(tree, dname);
> + smb2_deltree(tree, BASEDIR);
> + smb2_tdis(tree);
> + smb2_logoff(tree->session);
> +
> + return ret;
> +}
> +
> /*
> basic testing of SMB2 ACLs
> */
> @@ -2100,6 +2236,7 @@ struct torture_suite *torture_smb2_acls_init(void)
> torture_suite_add_1smb2_test(suite, "INHERITANCE",
> test_inheritance);
> torture_suite_add_1smb2_test(suite, "INHERITFLAGS",
> test_inheritance_flags);
> torture_suite_add_1smb2_test(suite, "DYNAMIC",
> test_inheritance_dynamic);
> + torture_suite_add_1smb2_test(suite, "OWNERRIGHTS",
> test_owner_rights);
> #if 0
> /* XXX This test does not work against XP or Vista. */
> torture_suite_add_1smb2_test(suite, "GETSET", test_sd_get_set);
>
>
> On Thu, Dec 1, 2016 at 5:56 AM, Jeremy Allison <jra at samba.org> wrote:
>
> > 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.
> >
More information about the samba-technical
mailing list