smb2.acls.ownerrights failure
Shilpa K
shilpa.krishnareddy at gmail.com
Thu Dec 1 09:13:43 UTC 2016
Hi Jeremy,
I have pasted below the git diff for samba-master branch.
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