From 884eed5f232926c814423422d9fc77b3d4ce7b78 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 12:29:49 +0100 Subject: [PATCH 1/4] s4:dsdb/acl_read: give some variables a better name Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/acl_read.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c index 92744f2..aa61c27 100644 --- a/source4/dsdb/samdb/ldb_modules/acl_read.c +++ b/source4/dsdb/samdb/ldb_modules/acl_read.c @@ -45,9 +45,9 @@ struct aclread_context { const char * const *attrs; const struct dsdb_schema *schema; uint32_t sd_flags; - bool sd; - bool instance_type; - bool object_sid; + bool added_nTSecurityDescriptor; + bool added_instanceType; + bool added_objectSid; bool indirsync; }; @@ -136,15 +136,15 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares) is_instancetype = ldb_attr_cmp("instanceType", msg->elements[i].name) == 0; /* these attributes were added to perform access checks and must be removed */ - if (is_objectsid && ac->object_sid) { + if (is_objectsid && ac->added_objectSid) { aclread_mark_inaccesslible(&msg->elements[i]); continue; } - if (is_instancetype && ac->instance_type) { + if (is_instancetype && ac->added_instanceType) { aclread_mark_inaccesslible(&msg->elements[i]); continue; } - if (is_sd && ac->sd) { + if (is_sd && ac->added_nTSecurityDescriptor) { aclread_mark_inaccesslible(&msg->elements[i]); continue; } @@ -275,6 +275,7 @@ static int aclread_search(struct ldb_module *module, struct ldb_request *req) uint32_t flags = ldb_req_get_custom_flags(req); struct ldb_result *res; struct aclread_private *p; + bool need_sd = false; bool is_untrusted = ldb_req_is_untrusted(req); const char * const *attrs = NULL; uint32_t instanceType; @@ -350,27 +351,28 @@ static int aclread_search(struct ldb_module *module, struct ldb_request *req) */ ac->sd_flags = dsdb_request_sd_flags(ac->req, NULL); - ac->sd = !(ldb_attr_in_list(req->op.search.attrs, "nTSecurityDescriptor")); + need_sd = !(ldb_attr_in_list(req->op.search.attrs, "nTSecurityDescriptor")); if (req->op.search.attrs && !ldb_attr_in_list(req->op.search.attrs, "*")) { if (!ldb_attr_in_list(req->op.search.attrs, "instanceType")) { - ac->instance_type = true; attrs = ldb_attr_list_copy_add(ac, req->op.search.attrs, "instanceType"); + ac->added_instanceType = true; } else { attrs = req->op.search.attrs; } if (!ldb_attr_in_list(req->op.search.attrs, "objectSid")) { - ac->object_sid = true; attrs = ldb_attr_list_copy_add(ac, attrs, "objectSid"); + ac->added_objectSid = true; } } - if (ac->sd) { + if (need_sd) { /* avoid replacing all attributes with nTSecurityDescriptor * if attribute list is empty */ if (!attrs) { attrs = ldb_attr_list_copy_add(ac, req->op.search.attrs, "*"); } attrs = ldb_attr_list_copy_add(ac, attrs, "nTSecurityDescriptor"); + ac->added_nTSecurityDescriptor = true; } ac->attrs = req->op.search.attrs; ret = ldb_build_search_req_ex(&down_req, -- 1.7.9.5 From f52dfdc578056eec5f2cb2e5395e2524c595160e Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 12:36:09 +0100 Subject: [PATCH 2/4] s4:dsdb/acl_read: return the nTSecurityDescriptor attr if the sd_flags control is given (bug #9470) Not returning the nTSecurityDescriptor causes a lot of problems, e.g. it lets the MMC Users and Computers crash. Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/acl_read.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c index aa61c27..5ab914f 100644 --- a/source4/dsdb/samdb/ldb_modules/acl_read.c +++ b/source4/dsdb/samdb/ldb_modules/acl_read.c @@ -276,6 +276,7 @@ static int aclread_search(struct ldb_module *module, struct ldb_request *req) struct ldb_result *res; struct aclread_private *p; bool need_sd = false; + bool explicit_sd_flags = false; bool is_untrusted = ldb_req_is_untrusted(req); const char * const *attrs = NULL; uint32_t instanceType; @@ -344,14 +345,16 @@ static int aclread_search(struct ldb_module *module, struct ldb_request *req) if (!ac->schema) { return ldb_operr(ldb); } - /* - * In theory we should also check for the SD control but control verification is - * expensive so we'd better had the ntsecuritydescriptor to the list of - * searched attribute and then remove it ! - */ - ac->sd_flags = dsdb_request_sd_flags(ac->req, NULL); - - need_sd = !(ldb_attr_in_list(req->op.search.attrs, "nTSecurityDescriptor")); + + ac->sd_flags = dsdb_request_sd_flags(ac->req, &explicit_sd_flags); + if (ldb_attr_in_list(req->op.search.attrs, "nTSecurityDescriptor")) { + need_sd = false; + } else if (explicit_sd_flags) { + need_sd = false; + } else { + need_sd = true; + } + if (req->op.search.attrs && !ldb_attr_in_list(req->op.search.attrs, "*")) { if (!ldb_attr_in_list(req->op.search.attrs, "instanceType")) { attrs = ldb_attr_list_copy_add(ac, req->op.search.attrs, "instanceType"); -- 1.7.9.5 From 702f6534934fc00ab554a26972912f9bd9e6d0f9 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 15:56:26 +0100 Subject: [PATCH 3/4] s4:dsdb/operational: fix stripping of the nTSecurityDescriptor attribute If the sd_flags control is specified, we should return nTSecurityDescriptor only if the client asked for all attributes. If there's a list of only explicit attribute names, we should ignore the sd_flags control. Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/operational.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/operational.c b/source4/dsdb/samdb/ldb_modules/operational.c index 4ce8b8f..c642ad8 100644 --- a/source4/dsdb/samdb/ldb_modules/operational.c +++ b/source4/dsdb/samdb/ldb_modules/operational.c @@ -721,10 +721,20 @@ static int operational_search_post_process(struct ldb_module *module, continue; } case OPERATIONAL_SD_FLAGS: - if (controls_flags->sd || - ldb_attr_in_list(attrs_from_user, operational_remove[i].attr)) { + if (ldb_attr_in_list(attrs_from_user, operational_remove[i].attr)) { continue; } + if (controls_flags->sd) { + if (attrs_from_user == NULL) { + continue; + } + if (attrs_from_user[0] == NULL) { + continue; + } + if (ldb_attr_in_list(attrs_from_user, "*")) { + continue; + } + } ldb_msg_remove_attr(msg, operational_remove[i].attr); break; } -- 1.7.9.5 From d95e08ba02a37b0f528d2f2c5f754278148a9573 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 14:04:47 +0100 Subject: [PATCH 4/4] s4:dsdb/tests/sec_descriptor: verify the nTSecurityDescriptor and sd_flags interaction This is a regression test for bug #9470. Signed-off-by: Stefan Metzmacher --- source4/dsdb/tests/python/sec_descriptor.py | 116 +++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/source4/dsdb/tests/python/sec_descriptor.py b/source4/dsdb/tests/python/sec_descriptor.py index aff6040..cf213ab 100755 --- a/source4/dsdb/tests/python/sec_descriptor.py +++ b/source4/dsdb/tests/python/sec_descriptor.py @@ -1848,6 +1848,122 @@ class SdFlagsDescriptorTests(DescriptorTests): self.assertFalse("S:" in desc_sddl) self.assertFalse("G:" in desc_sddl) + def test_311(self): + sd_flags = (SECINFO_OWNER | + SECINFO_GROUP | + SECINFO_DACL | + SECINFO_SACL) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + [], controls=None) + self.assertFalse("nTSecurityDescriptor" in res[0]) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["name"], controls=None) + self.assertFalse("nTSecurityDescriptor" in res[0]) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["name"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertFalse("nTSecurityDescriptor" in res[0]) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + [], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["*"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["nTSecurityDescriptor", "*"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["*", "nTSecurityDescriptor"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["nTSecurityDescriptor", "name"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["name", "nTSecurityDescriptor"], controls=["sd_flags:1:%d" % (sd_flags)]) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["nTSecurityDescriptor"], controls=None) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["name", "nTSecurityDescriptor"], controls=None) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) + + res = self.ldb_admin.search(self.base_dn, SCOPE_BASE, None, + ["nTSecurityDescriptor", "name"], controls=None) + self.assertTrue("nTSecurityDescriptor" in res[0]) + tmp = res[0]["nTSecurityDescriptor"][0] + sd = ndr_unpack(security.descriptor, tmp) + sddl = sd.as_sddl(self.sd_utils.domain_sid) + self.assertTrue("O:" in sddl) + self.assertTrue("G:" in sddl) + self.assertTrue("D:" in sddl) + self.assertTrue("S:" in sddl) class RightsAttributesTests(DescriptorTests): -- 1.7.9.5