From 884eed5f232926c814423422d9fc77b3d4ce7b78 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 12:29:49 +0100 Subject: [PATCH 1/7] 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 754d4430fd7dea2f551f408875ce89fe0b70e751 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 7 Dec 2012 11:02:49 +0100 Subject: [PATCH 2/7] s4:dsdb/acl_read: keep the ldb_message of the sub search (bug #9470) Some modules might not allocate values on the correct memory context. Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/acl_read.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source4/dsdb/samdb/ldb_modules/acl_read.c b/source4/dsdb/samdb/ldb_modules/acl_read.c index aa61c27..4c2529e 100644 --- a/source4/dsdb/samdb/ldb_modules/acl_read.c +++ b/source4/dsdb/samdb/ldb_modules/acl_read.c @@ -245,6 +245,11 @@ static int aclread_callback(struct ldb_request *req, struct ldb_reply *ares) k++; } } + /* + * This should not be needed, but some modules + * may allocate values on the wrong context... + */ + talloc_steal(ret_msg->elements, msg); } else { ret_msg->elements = NULL; } -- 1.7.9.5 From 559b354fee7b0f6f9d765c33f41858a4f29d3c20 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 12:36:09 +0100 Subject: [PATCH 3/7] 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 4c2529e..e3768d1 100644 --- a/source4/dsdb/samdb/ldb_modules/acl_read.c +++ b/source4/dsdb/samdb/ldb_modules/acl_read.c @@ -281,6 +281,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; @@ -349,14 +350,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 672f94e5bfcacd455223bd72cd9df7051fbd81d8 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 7 Dec 2012 10:08:14 +0000 Subject: [PATCH 4/7] s4:dsdb/schema_data.c: correctly move the CN=Aggregate attributes to msg->elements[i].values (bug #9470) We should keep the talloc hierarchy sane. Signed-off-by: Stefan Metzmacher --- source4/dsdb/samdb/ldb_modules/schema_data.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/schema_data.c b/source4/dsdb/samdb/ldb_modules/schema_data.c index bc9488b..3ce7ef9 100644 --- a/source4/dsdb/samdb/ldb_modules/schema_data.c +++ b/source4/dsdb/samdb/ldb_modules/schema_data.c @@ -405,7 +405,11 @@ static int generate_objectClasses(struct ldb_context *ldb, struct ldb_message *m int ret; for (sclass = schema->classes; sclass; sclass = sclass->next) { - ret = ldb_msg_add_string(msg, "objectClasses", schema_class_to_description(msg, sclass)); + char *v = schema_class_to_description(msg, sclass); + if (v == NULL) { + return ldb_oom(ldb); + } + ret = ldb_msg_add_steal_string(msg, "objectClasses", v); if (ret != LDB_SUCCESS) { return ret; } @@ -417,9 +421,13 @@ static int generate_attributeTypes(struct ldb_context *ldb, struct ldb_message * { const struct dsdb_attribute *attribute; int ret; - + for (attribute = schema->attributes; attribute; attribute = attribute->next) { - ret = ldb_msg_add_string(msg, "attributeTypes", schema_attribute_to_description(msg, attribute)); + char *v = schema_attribute_to_description(msg, attribute); + if (v == NULL) { + return ldb_oom(ldb); + } + ret = ldb_msg_add_steal_string(msg, "attributeTypes", v); if (ret != LDB_SUCCESS) { return ret; } @@ -461,7 +469,7 @@ static int generate_extendedAttributeInfo(struct ldb_context *ldb, return ldb_oom(ldb); } - ret = ldb_msg_add_string(msg, "extendedAttributeInfo", val); + ret = ldb_msg_add_steal_string(msg, "extendedAttributeInfo", val); if (ret != LDB_SUCCESS) { return ret; } @@ -483,7 +491,7 @@ static int generate_extendedClassInfo(struct ldb_context *ldb, return ldb_oom(ldb); } - ret = ldb_msg_add_string(msg, "extendedClassInfo", val); + ret = ldb_msg_add_steal_string(msg, "extendedClassInfo", val); if (ret != LDB_SUCCESS) { return ret; } @@ -521,7 +529,11 @@ static int generate_possibleInferiors(struct ldb_context *ldb, struct ldb_messag } for (i=0;possibleInferiors[i];i++) { - ret = ldb_msg_add_string(msg, "possibleInferiors", possibleInferiors[i]); + char *v = talloc_strdup(msg, possibleInferiors[i]); + if (v == NULL) { + return ldb_oom(ldb); + } + ret = ldb_msg_add_steal_string(msg, "possibleInferiors", v); if (ret != LDB_SUCCESS) { return ret; } -- 1.7.9.5 From 203f2bd3df85218f2c6d48c890b00b8a6a0e6d84 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 7 Dec 2012 10:34:58 +0100 Subject: [PATCH 5/7] s4:dsdb/schema: fix dsdb_schema_set_el_from_ldb_msg() (bug #9470) We should always update the ts_last_change. Signed-off-by: Stefan Metzmacher --- source4/dsdb/schema/schema_set.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c index e226118..31862a4 100644 --- a/source4/dsdb/schema/schema_set.c +++ b/source4/dsdb/schema/schema_set.c @@ -676,13 +676,6 @@ WERROR dsdb_schema_set_el_from_ldb_msg(struct ldb_context *ldb, struct dsdb_sche { const char* tstring; time_t ts; - if (samdb_find_attribute(ldb, msg, - "objectclass", "attributeSchema") != NULL) { - return dsdb_set_attribute_from_ldb(ldb, schema, msg); - } else if (samdb_find_attribute(ldb, msg, - "objectclass", "classSchema") != NULL) { - return dsdb_set_class_from_ldb(schema, msg); - } tstring = ldb_msg_find_attr_as_string(msg, "whenChanged", NULL); /* keep a trace of the ts of the most recently changed object */ if (tstring) { @@ -691,6 +684,13 @@ WERROR dsdb_schema_set_el_from_ldb_msg(struct ldb_context *ldb, struct dsdb_sche schema->ts_last_change = ts; } } + if (samdb_find_attribute(ldb, msg, + "objectclass", "attributeSchema") != NULL) { + return dsdb_set_attribute_from_ldb(ldb, schema, msg); + } else if (samdb_find_attribute(ldb, msg, + "objectclass", "classSchema") != NULL) { + return dsdb_set_class_from_ldb(schema, msg); + } /* Don't fail on things not classes or attributes */ return WERR_OK; } -- 1.7.9.5 From 5b0d5755fd18c94405ea3dd47b68f1ccb888dba6 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 15:56:26 +0100 Subject: [PATCH 6/7] 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 99a407a0aca1c84062d584e568f320014ad04767 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 6 Dec 2012 14:04:47 +0100 Subject: [PATCH 7/7] 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