From 62feea3df2242833133c4705ce1997811ddd4abf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 10 May 2018 12:28:43 +0200 Subject: [PATCH 1/2] s4:torture/smb2: new test for interaction between chown and SD flags This passes against Windows, but fails against Samba. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13432 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit 12f6d56c4814fca64e0e3c636018e70d71ad0be5) --- selftest/knownfail.d/samba3.smb2.acls | 1 + source4/torture/smb2/acls.c | 278 ++++++++++++++++++++++++++ 2 files changed, 279 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls new file mode 100644 index 00000000000..68966c951a9 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.acls @@ -0,0 +1 @@ +^samba3.smb2.acls.SDFLAGSVSCHOWN.* diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c index 7365554470c..6178e211034 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -1500,6 +1500,283 @@ done: return ret; } +/* + * This is basically a copy of test_inheritance_flags() with an additional twist + * to change the owner of the testfile, verifying that the security descriptor + * flags are not altered. + */ +static bool test_sd_flags_vs_chown(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 = {{0}}; + struct smb2_handle handle2 = {{0}}; + int i, j; + union smb_fileinfo q; + union smb_setfileinfo set; + struct security_descriptor *sd, *sd2, *sd_orig=NULL; + struct security_descriptor *owner_sd = NULL; + const char *owner_sid_string = NULL; + struct dom_sid *owner_sid = NULL; + struct dom_sid world_sid = global_sid_World; + struct { + uint32_t parent_set_sd_type; /* 3 options */ + uint32_t parent_set_ace_inherit; /* 1 option */ + uint32_t parent_get_sd_type; + uint32_t parent_get_ace_inherit; + uint32_t child_get_sd_type; + uint32_t child_get_ace_inherit; + } tflags[16] = {{0}}; /* 2^4 */ + + owner_sd = security_descriptor_dacl_create(tctx, + 0, + SID_WORLD, + NULL, + NULL); + torture_assert_not_null_goto(tctx, owner_sd, ret, done, + "security_descriptor_dacl_create failed\n"); + + for (i = 0; i < 15; i++) { + torture_comment(tctx, "i=%d:", i); + + if (i & 1) { + tflags[i].parent_set_sd_type |= + SEC_DESC_DACL_AUTO_INHERITED; + torture_comment(tctx, "AUTO_INHERITED, "); + } + if (i & 2) { + tflags[i].parent_set_sd_type |= + SEC_DESC_DACL_AUTO_INHERIT_REQ; + torture_comment(tctx, "AUTO_INHERIT_REQ, "); + } + if (i & 4) { + tflags[i].parent_set_sd_type |= + SEC_DESC_DACL_PROTECTED; + torture_comment(tctx, "PROTECTED, "); + tflags[i].parent_get_sd_type |= + SEC_DESC_DACL_PROTECTED; + } + if (i & 8) { + tflags[i].parent_set_ace_inherit |= + SEC_ACE_FLAG_INHERITED_ACE; + torture_comment(tctx, "INHERITED, "); + tflags[i].parent_get_ace_inherit |= + SEC_ACE_FLAG_INHERITED_ACE; + } + + if ((tflags[i].parent_set_sd_type & + (SEC_DESC_DACL_AUTO_INHERITED | SEC_DESC_DACL_AUTO_INHERIT_REQ)) == + (SEC_DESC_DACL_AUTO_INHERITED | SEC_DESC_DACL_AUTO_INHERIT_REQ)) { + tflags[i].parent_get_sd_type |= + SEC_DESC_DACL_AUTO_INHERITED; + tflags[i].child_get_sd_type |= + SEC_DESC_DACL_AUTO_INHERITED; + tflags[i].child_get_ace_inherit |= + SEC_ACE_FLAG_INHERITED_ACE; + torture_comment(tctx, " ... parent is AUTO INHERITED"); + } + + if (tflags[i].parent_set_ace_inherit & + SEC_ACE_FLAG_INHERITED_ACE) { + tflags[i].parent_get_ace_inherit = + SEC_ACE_FLAG_INHERITED_ACE; + torture_comment(tctx, " ... parent ACE is INHERITED"); + } + + torture_comment(tctx, "\n"); + } + + if (!smb2_util_setup_dir(tctx, tree, BASEDIR)) + return false; + + torture_comment(tctx, "TESTING ACL INHERITANCE FLAGS\n"); + + 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 = NTCREATEX_SHARE_ACCESS_MASK; + 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; + + torture_comment(tctx, "creating initial directory %s\n", dname); + status = smb2_create(tree, tctx, &io); + CHECK_STATUS(status, NT_STATUS_OK); + handle = io.out.file.handle; + + torture_comment(tctx, "getting 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 = sd_orig->owner_sid; + owner_sid_string = dom_sid_string(tctx, sd_orig->owner_sid); + torture_comment(tctx, "owner_sid is %s\n", owner_sid_string); + + for (i=0; i < ARRAY_SIZE(tflags); i++) { + torture_comment(tctx, "setting a new sd on directory, pass #%d\n", i); + + sd = security_descriptor_dacl_create(tctx, + tflags[i].parent_set_sd_type, + NULL, NULL, + owner_sid_string, + SEC_ACE_TYPE_ACCESS_ALLOWED, + SEC_FILE_WRITE_DATA | SEC_STD_WRITE_DAC, + SEC_ACE_FLAG_OBJECT_INHERIT | + SEC_ACE_FLAG_CONTAINER_INHERIT | + tflags[i].parent_set_ace_inherit, + SID_WORLD, + SEC_ACE_TYPE_ACCESS_ALLOWED, + SEC_FILE_ALL | SEC_STD_ALL, + 0, + NULL); + 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_OK); + + /* + * Check DACL we just set, except change the bits to what they + * should be. + */ + torture_comment(tctx, " checking new sd\n"); + + /* REQ bit should always be false. */ + sd->type &= ~SEC_DESC_DACL_AUTO_INHERIT_REQ; + + if ((tflags[i].parent_get_sd_type & SEC_DESC_DACL_AUTO_INHERITED) == 0) + sd->type &= ~SEC_DESC_DACL_AUTO_INHERITED; + + q.query_secdesc.in.file.handle = handle; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL; + status = smb2_getinfo_file(tree, tctx, &q); + CHECK_STATUS(status, NT_STATUS_OK); + CHECK_SECURITY_DESCRIPTOR(q.query_secdesc.out.sd, sd); + + /* Create file. */ + torture_comment(tctx, " creating file %s\n", fname1); + io.in.fname = fname1; + io.in.create_options = 0; + io.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + io.in.desired_access = SEC_RIGHTS_FILE_ALL; + io.in.create_disposition = NTCREATEX_DISP_CREATE; + status = smb2_create(tree, tctx, &io); + CHECK_STATUS(status, NT_STATUS_OK); + handle2 = io.out.file.handle; + CHECK_ACCESS_FLAGS(handle2, SEC_RIGHTS_FILE_ALL); + + q.query_secdesc.in.file.handle = handle2; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; + status = smb2_getinfo_file(tree, tctx, &q); + CHECK_STATUS(status, NT_STATUS_OK); + + torture_comment(tctx, " checking sd on file %s\n", fname1); + sd2 = security_descriptor_dacl_create(tctx, + tflags[i].child_get_sd_type, + owner_sid_string, NULL, + owner_sid_string, + SEC_ACE_TYPE_ACCESS_ALLOWED, + SEC_FILE_WRITE_DATA | SEC_STD_WRITE_DAC, + tflags[i].child_get_ace_inherit, + NULL); + CHECK_SECURITY_DESCRIPTOR(q.query_secdesc.out.sd, sd2); + + /* + * Set new sd on file ... prove that the bits have nothing to + * do with the parents bits when manually setting an ACL. The + * _AUTO_INHERITED bit comes directly from the ACL set. + */ + for (j = 0; j < ARRAY_SIZE(tflags); j++) { + torture_comment(tctx, " setting new file sd, pass #%d\n", j); + + /* Change sd type. */ + sd2->type &= ~(SEC_DESC_DACL_AUTO_INHERITED | + SEC_DESC_DACL_AUTO_INHERIT_REQ | + SEC_DESC_DACL_PROTECTED); + sd2->type |= tflags[j].parent_set_sd_type; + + sd2->dacl->aces[0].flags &= + ~SEC_ACE_FLAG_INHERITED_ACE; + sd2->dacl->aces[0].flags |= + tflags[j].parent_set_ace_inherit; + + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + set.set_secdesc.in.file.handle = handle2; + set.set_secdesc.in.secinfo_flags = SECINFO_DACL; + set.set_secdesc.in.sd = sd2; + status = smb2_setinfo_file(tree, &set); + CHECK_STATUS(status, NT_STATUS_OK); + + /* Check DACL we just set. */ + sd2->type &= ~SEC_DESC_DACL_AUTO_INHERIT_REQ; + if ((tflags[j].parent_get_sd_type & SEC_DESC_DACL_AUTO_INHERITED) == 0) + sd2->type &= ~SEC_DESC_DACL_AUTO_INHERITED; + + q.query_secdesc.in.file.handle = handle2; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; + status = smb2_getinfo_file(tree, tctx, &q); + CHECK_STATUS(status, NT_STATUS_OK); + + CHECK_SECURITY_DESCRIPTOR(q.query_secdesc.out.sd, sd2); + + /* + * Check that changing ownder doesn't affect SD flags. + * + * Do this by first changing ownder to world and then + * back to the original ownder. Afterwards compare SD, + * should be the same. + */ + owner_sd->owner_sid = &world_sid; + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + set.set_secdesc.in.file.handle = handle2; + set.set_secdesc.in.secinfo_flags = SECINFO_OWNER; + set.set_secdesc.in.sd = owner_sd; + status = smb2_setinfo_file(tree, &set); + CHECK_STATUS(status, NT_STATUS_OK); + + owner_sd->owner_sid = owner_sid; + set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC; + set.set_secdesc.in.file.handle = handle2; + set.set_secdesc.in.secinfo_flags = SECINFO_OWNER; + set.set_secdesc.in.sd = owner_sd; + status = smb2_setinfo_file(tree, &set); + CHECK_STATUS(status, NT_STATUS_OK); + + q.query_secdesc.in.file.handle = handle2; + q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER; + status = smb2_getinfo_file(tree, tctx, &q); + CHECK_STATUS(status, NT_STATUS_OK); + + CHECK_SECURITY_DESCRIPTOR(q.query_secdesc.out.sd, sd2); + torture_assert_goto(tctx, ret, ret, done, "CHECK_SECURITY_DESCRIPTOR failed\n"); + } + + smb2_util_close(tree, handle2); + smb2_util_unlink(tree, fname1); + } + +done: + smb2_util_close(tree, handle); + smb2_deltree(tree, BASEDIR); + smb2_tdis(tree); + smb2_logoff(tree->session); + return ret; +} + /* test dynamic acl inheritance Note: This test was copied from raw/acls.c. @@ -2098,6 +2375,7 @@ struct torture_suite *torture_smb2_acls_init(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "OWNER", test_owner_bits); 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, "SDFLAGSVSCHOWN", test_sd_flags_vs_chown); torture_suite_add_1smb2_test(suite, "DYNAMIC", test_inheritance_dynamic); #if 0 /* XXX This test does not work against XP or Vista. */ -- 2.17.0.441.gb46fe60e1d-goog From 8cca6dbd105ce3545201059ea5a9236c4607bcc6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 10 May 2018 12:29:35 +0200 Subject: [PATCH 2/2] s3:smbd: fix interaction between chown and SD flags A change ownership operation that doesn't set the NT ACLs must not touch the SD flags (type). Bug: https://bugzilla.samba.org/show_bug.cgi?id=13432 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Fri May 11 23:30:32 CEST 2018 on sn-devel-144 (cherry picked from commit ced55850034a3653525823bf9623912a4fcf18a0) --- selftest/knownfail.d/samba3.smb2.acls | 1 - source3/modules/vfs_acl_common.c | 7 +++++-- 2 files changed, 5 insertions(+), 3 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.acls diff --git a/selftest/knownfail.d/samba3.smb2.acls b/selftest/knownfail.d/samba3.smb2.acls deleted file mode 100644 index 68966c951a9..00000000000 --- a/selftest/knownfail.d/samba3.smb2.acls +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.acls.SDFLAGSVSCHOWN.* diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c index 546e97b9b5d..f2b2df19400 100644 --- a/source3/modules/vfs_acl_common.c +++ b/source3/modules/vfs_acl_common.c @@ -941,8 +941,11 @@ NTSTATUS fset_nt_acl_common( } psd->revision = orig_psd->revision; - /* All our SD's are self relative. */ - psd->type = orig_psd->type | SEC_DESC_SELF_RELATIVE; + if (security_info_sent & SECINFO_DACL) { + psd->type = orig_psd->type; + /* All our SD's are self relative. */ + psd->type |= SEC_DESC_SELF_RELATIVE; + } if ((security_info_sent & SECINFO_OWNER) && (orig_psd->owner_sid != NULL)) { if (!dom_sid_equal(orig_psd->owner_sid, psd->owner_sid)) { -- 2.17.0.441.gb46fe60e1d-goog