Fixed some major bugs in inheritance and access checks.

Fixed sd creation not working on LDAP modify.
Fixed incorrect replacement of CO and CG.
Fixed incorrect access check on modify for SD modification.
Fixed failing sec_descriptor test and enabled it.
Fixed failing sd add test in ldap.python

5 files changed, 274 insertions, 120 deletions

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index f96d4294c58..13e71e5b65f 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -257,6 +257,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 	struct ldb_context *ldb = ldb_module_get_ctx(module);
 	const struct dsdb_schema *schema = dsdb_get_schema(ldb);
 	int i;
+	bool modify_sd = false;
 	const struct GUID *guid;
 	uint32_t access_granted;
 	struct object_tree *root = NULL;
@@ -315,41 +316,65 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 			attr = dsdb_attribute_by_lDAPDisplayName(schema,
 								 req->op.mod.message->elements[i].name);
 		}
+		if (strcmp("nTSecurityDescriptor", req->op.mod.message->elements[i].name) == 0) {
+			modify_sd = true;
+		} else {
 
-		if (!attr) {
-			DEBUG(10, ("acl_modify: cannot find attribute %s\n",
-				   req->op.mod.message->elements[i].name));
-			goto fail;
-		}
-		if (!insert_in_object_tree(tmp_ctx,
-					   &attr->attributeSecurityGUID, SEC_ADS_WRITE_PROP, 
-					   &new_node, &new_node)) {
-			DEBUG(10, ("acl_modify: cannot add to object tree securityGUID\n"));
-			goto fail;
+			if (!attr) {
+				DEBUG(10, ("acl_modify: cannot find attribute %s\n",
+					   req->op.mod.message->elements[i].name));
+				goto fail;
+			}
+			if (!insert_in_object_tree(tmp_ctx,
+						   &attr->attributeSecurityGUID, SEC_ADS_WRITE_PROP,
+						   &new_node, &new_node)) {
+				DEBUG(10, ("acl_modify: cannot add to object tree securityGUID\n"));
+				goto fail;
+			}
+
+			if (!insert_in_object_tree(tmp_ctx,
+						   &attr->schemaIDGUID, SEC_ADS_WRITE_PROP, &new_node, &new_node)) {
+				DEBUG(10, ("acl_modify: cannot add to object tree attributeGUID\n"));
+				goto fail;
+			}
 		}
+	}
 
-		if (!insert_in_object_tree(tmp_ctx,
-					   &attr->schemaIDGUID, SEC_ADS_WRITE_PROP, &new_node, &new_node)) {
-			DEBUG(10, ("acl_modify: cannot add to object tree attributeGUID\n"));
-			goto fail;
+	if (root->num_of_children > 0) {
+		status = sec_access_check_ds(sd, acl_user_token(module),
+					     SEC_ADS_WRITE_PROP,
+					     &access_granted,
+					     root);
+
+		if (!NT_STATUS_IS_OK(status)) {
+			DEBUG(10, ("Object %s nas no write property access\n",
+				  ldb_dn_get_linearized(req->op.mod.message->dn)));
+			acl_debug(sd,
+				  acl_user_token(module),
+				  req->op.mod.message->dn,
+				  true,
+				  10);
+			talloc_free(tmp_ctx);
+			return LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
 		}
 	}
-
-	status = sec_access_check_ds(sd, acl_user_token(module),
-				     SEC_ADS_WRITE_PROP,
+	if (modify_sd) {
+		status = sec_access_check_ds(sd, acl_user_token(module),
+				     SEC_STD_WRITE_DAC,
 				     &access_granted,
-				     root);
+				     NULL);
 
-	if (!NT_STATUS_IS_OK(status)) {
-		DEBUG(10, ("Object %s nas no write property access\n",
-			   ldb_dn_get_linearized(req->op.mod.message->dn)));
-		acl_debug(sd,
-			  acl_user_token(module),
-			  req->op.mod.message->dn,
-			  true,
-			  10);
-		talloc_free(tmp_ctx);
-		return LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
+		if (!NT_STATUS_IS_OK(status)) {
+			DEBUG(10, ("Object %s nas no write dacl access\n",
+				   ldb_dn_get_linearized(req->op.mod.message->dn)));
+			acl_debug(sd,
+				  acl_user_token(module),
+				  req->op.mod.message->dn,
+				  true,
+				  10);
+			talloc_free(tmp_ctx);
+			return LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS;
+		}
 	}
 
 	talloc_free(tmp_ctx);
diff --git a/source4/dsdb/samdb/ldb_modules/descriptor.c b/source4/dsdb/samdb/ldb_modules/descriptor.c
index f1c010c2bfa..6a575603145 100644
--- a/source4/dsdb/samdb/ldb_modules/descriptor.c
+++ b/source4/dsdb/samdb/ldb_modules/descriptor.c
@@ -47,10 +47,13 @@ struct descriptor_data {
 };
 
 struct descriptor_context {
-		struct ldb_module *module;
-		struct ldb_request *req;
-		struct ldb_reply *search_res;
-		int (*step_fn)(struct descriptor_context *);
+	struct ldb_module *module;
+	struct ldb_request *req;
+	struct ldb_reply *search_res;
+	struct ldb_reply *search_oc_res;
+	struct ldb_val *parentsd_val;
+	struct ldb_val *sd_val;
+	int (*step_fn)(struct descriptor_context *);
 };
 
 static const struct dsdb_class * get_last_structural_class(const struct dsdb_schema *schema, struct ldb_message_element *element)
@@ -212,7 +215,7 @@ static DATA_BLOB *get_new_descriptor(struct ldb_module *module,
 
 
 	sddl_sd = sddl_encode(mem_ctx, new_sd, domain_sid);
-	DEBUG(10, ("Object %s created with desriptor %s", ldb_dn_get_linearized(dn), sddl_sd));
+	DEBUG(10, ("Object %s created with desriptor %s\n\n", ldb_dn_get_linearized(dn), sddl_sd));
 
 	linear_sd = talloc(mem_ctx, DATA_BLOB);
 	if (!linear_sd) {
@@ -272,12 +275,12 @@ static int get_search_callback(struct ldb_request *req, struct ldb_reply *ares)
 
 	switch (ares->type) {
 	case LDB_REPLY_ENTRY:
-		if (ac->search_res != NULL) {
+			if (ac->search_res != NULL) {
 			ldb_set_errstring(ldb, "Too many results");
 			talloc_free(ares);
 			return ldb_module_done(ac->req, NULL, NULL,
 						LDB_ERR_OPERATIONS_ERROR);
-		}
+						}
 
 		ac->search_res = talloc_steal(ac, ares);
 		break;
@@ -298,6 +301,58 @@ static int get_search_callback(struct ldb_request *req, struct ldb_reply *ares)
 
 	return LDB_SUCCESS;
 }
+
+static int get_search_oc_callback(struct ldb_request *req, struct ldb_reply *ares)
+{
+	struct ldb_context *ldb;
+	struct descriptor_context *ac;
+	int ret;
+
+	ac = talloc_get_type(req->context, struct descriptor_context);
+	ldb = ldb_module_get_ctx(ac->module);
+
+	if (!ares) {
+		return ldb_module_done(ac->req, NULL, NULL,
+					LDB_ERR_OPERATIONS_ERROR);
+	}
+	if (ares->error != LDB_SUCCESS &&
+	    ares->error != LDB_ERR_NO_SUCH_OBJECT) {
+		return ldb_module_done(ac->req, ares->controls,
+					ares->response, ares->error);
+	}
+
+	ldb_reset_err_string(ldb);
+
+	switch (ares->type) {
+	case LDB_REPLY_ENTRY:
+			if (ac->search_oc_res != NULL) {
+			ldb_set_errstring(ldb, "Too many results");
+			talloc_free(ares);
+			return ldb_module_done(ac->req, NULL, NULL,
+						LDB_ERR_OPERATIONS_ERROR);
+						}
+
+		ac->search_oc_res = talloc_steal(ac, ares);
+		break;
+
+	case LDB_REPLY_REFERRAL:
+		/* ignore */
+		talloc_free(ares);
+		break;
+
+	case LDB_REPLY_DONE:
+		talloc_free(ares);
+		ret = ac->step_fn(ac);
+		if (ret != LDB_SUCCESS) {
+			return ldb_module_done(ac->req, NULL, NULL, ret);
+		}
+		break;
+	}
+
+	return LDB_SUCCESS;
+}
+
+
 static int descriptor_op_callback(struct ldb_request *req, struct ldb_reply *ares)
 {
 	struct descriptor_context *ac;
@@ -323,6 +378,58 @@ static int descriptor_op_callback(struct ldb_request *req, struct ldb_reply *are
 				ares->response, ares->error);
 }
 
+static int descriptor_do_mod(struct descriptor_context *ac)
+{
+	struct ldb_context *ldb;
+	const struct dsdb_schema *schema;
+	struct ldb_request *mod_req;
+	struct ldb_message_element *objectclass_element, *tmp_element;
+	int ret;
+	DATA_BLOB *sd;
+	const struct dsdb_class *objectclass;
+	struct ldb_message *msg;
+	int flags = 0;
+
+	ldb = ldb_module_get_ctx(ac->module);
+	schema = dsdb_get_schema(ldb);
+
+	msg = ldb_msg_copy_shallow(ac, ac->req->op.mod.message);
+	objectclass_element = ldb_msg_find_element(ac->search_oc_res->message, "objectClass");
+	objectclass = get_last_structural_class(schema, objectclass_element);
+
+	if (!objectclass) {
+		ldb_asprintf_errstring(ldb, "No last structural objectclass found on %s",
+				       ldb_dn_get_linearized(ac->search_oc_res->message->dn));
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
+	sd = get_new_descriptor(ac->module, msg->dn, ac, objectclass,
+				ac->parentsd_val, ac->sd_val);
+	if (ac->sd_val) {
+		tmp_element = ldb_msg_find_element(msg, "ntSecurityDescriptor");
+		flags = tmp_element->flags;
+		ldb_msg_remove_attr(msg, "nTSecurityDescriptor");
+	}
+
+	if (sd) {
+		ret = ldb_msg_add_steal_value(msg, "nTSecurityDescriptor", sd);
+		if (ret != LDB_SUCCESS) {
+			return ret;
+		}
+		tmp_element = ldb_msg_find_element(msg, "ntSecurityDescriptor");
+		tmp_element->flags = flags;
+	}
+	ret = ldb_build_mod_req(&mod_req, ldb, ac,
+				msg,
+				ac->req->controls,
+				ac, descriptor_op_callback,
+				ac->req);
+	if (ret != LDB_SUCCESS) {
+		return ret;
+	}
+	return ldb_next_request(ac->module, mod_req);
+}
+
 static int descriptor_do_add(struct descriptor_context *ac)
 {
 	struct ldb_context *ldb;
@@ -332,94 +439,132 @@ static int descriptor_do_add(struct descriptor_context *ac)
 	struct ldb_message *msg;
 	TALLOC_CTX *mem_ctx;
 	int ret;
-	struct ldb_val *sd_val = NULL;
-	const struct ldb_val *parentsd_val = NULL;
 	DATA_BLOB *sd;
 	const struct dsdb_class *objectclass;
+	static const char *const attrs[] = { "objectClass", NULL };
+	struct ldb_request *search_req;
 
 	ldb = ldb_module_get_ctx(ac->module);
 	schema = dsdb_get_schema(ldb);
-
 	mem_ctx = talloc_new(ac);
 	if (mem_ctx == NULL) {
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
 
-	msg = ldb_msg_copy_shallow(ac, ac->req->op.add.message);
+	switch (ac->req->operation) {
+	case LDB_ADD:
+		msg = ldb_msg_copy_shallow(ac, ac->req->op.add.message);
+		objectclass_element = ldb_msg_find_element(msg, "objectClass");
+		objectclass = get_last_structural_class(schema, objectclass_element);
 
-	/* get the security descriptor values*/
-	sd_element = ldb_msg_find_element(msg, "nTSecurityDescriptor");
-	objectclass_element = ldb_msg_find_element(msg, "objectClass");
-	objectclass = get_last_structural_class(schema, objectclass_element);
-
-	if (!objectclass) {
-		ldb_asprintf_errstring(ldb, "No last structural objectclass found on %s", ldb_dn_get_linearized(msg->dn));
+		if (!objectclass) {
+			ldb_asprintf_errstring(ldb, "No last structural objectclass found on %s", ldb_dn_get_linearized(msg->dn));
+			return LDB_ERR_OPERATIONS_ERROR;
+		}
+		break;
+	case LDB_MODIFY:
+		msg = ldb_msg_copy_shallow(ac, ac->req->op.mod.message);
+		break;
+	default:
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
 
-	if (sd_element)
-		sd_val = &sd_element->values[0];
+
+	/* get the security descriptor values*/
+	sd_element = ldb_msg_find_element(msg, "nTSecurityDescriptor");
+	if (sd_element) {
+		ac->sd_val = talloc_memdup(ac, &sd_element->values[0], sizeof(struct ldb_val));
+	}
 	/* NC's have no parent */
 	if ((ldb_dn_compare(msg->dn, (ldb_get_schema_basedn(ldb))) == 0) ||
 	    (ldb_dn_compare(msg->dn, (ldb_get_config_basedn(ldb))) == 0) ||
 	    (ldb_dn_compare(msg->dn, (ldb_get_root_basedn(ldb))) == 0)) {
-		parentsd_val = NULL;
-	} else if (ac->search_res != NULL){
-		parentsd_val = ldb_msg_find_ldb_val(ac->search_res->message, "nTSecurityDescriptor");
+		ac->parentsd_val = NULL;
+	} else if (ac->search_res != NULL) {
+		struct ldb_message_element *parent_element = ldb_msg_find_element(ac->search_res->message, "nTSecurityDescriptor");
+		if (parent_element) {
+			ac->parentsd_val = talloc_memdup(ac, &parent_element->values[0], sizeof(struct ldb_val));
+		}
 	}
 
+	if (ac->req->operation == LDB_ADD) {
 	/* get the parent descriptor and the one provided. If not provided, get the default.*/
 	/* convert to security descriptor and calculate */
-	sd = get_new_descriptor(ac->module, msg->dn, mem_ctx, objectclass,
-			parentsd_val, sd_val);
-	if (sd_val) {
-		ldb_msg_remove_attr(msg, "nTSecurityDescriptor");
-	}
+		sd = get_new_descriptor(ac->module, msg->dn, mem_ctx, objectclass,
+					ac->parentsd_val, ac->sd_val);
+		if (ac->sd_val) {
+			ldb_msg_remove_attr(msg, "nTSecurityDescriptor");
+		}
 
-	if (sd) {
-		ret = ldb_msg_add_steal_value(msg, "nTSecurityDescriptor", sd);
-		if (ret != LDB_SUCCESS) {
-			return ret;
+		if (sd) {
+			ret = ldb_msg_add_steal_value(msg, "nTSecurityDescriptor", sd);
+			if (ret != LDB_SUCCESS) {
+				return ret;
+			}
 		}
-	}
 
-	talloc_free(mem_ctx);
-	ret = ldb_msg_sanity_check(ldb, msg);
+		talloc_free(mem_ctx);
+		ret = ldb_msg_sanity_check(ldb, msg);
 
-	if (ret != LDB_SUCCESS) {
-		ldb_asprintf_errstring(ldb, "No last structural objectclass found on %s", ldb_dn_get_linearized(msg->dn));
-		return ret;
-	}
+		if (ret != LDB_SUCCESS) {
+			ldb_asprintf_errstring(ldb, "No last structural objectclass found on %s",
+					       ldb_dn_get_linearized(msg->dn));
+			return ret;
+		}
 
-	ret = ldb_build_add_req(&add_req, ldb, ac,
-				msg,
-				ac->req->controls,
-				ac, descriptor_op_callback,
-				ac->req);
-	if (ret != LDB_SUCCESS) {
-		return ret;
+		ret = ldb_build_add_req(&add_req, ldb, ac,
+					msg,
+					ac->req->controls,
+					ac, descriptor_op_callback,
+					ac->req);
+		if (ret != LDB_SUCCESS) {
+			return ret;
+		}
+		return ldb_next_request(ac->module, add_req);
+	} else {
+		ret = ldb_build_search_req(&search_req, ldb,
+				   ac, msg->dn, LDB_SCOPE_BASE,
+				   "(objectClass=*)", attrs,
+				   NULL,
+				   ac, get_search_oc_callback,
+				   ac->req);
+		if (ret != LDB_SUCCESS) {
+			return ret;
+		}
+		ac->step_fn = descriptor_do_mod;
+		return ldb_next_request(ac->module, search_req);
 	}
-
-	/* perform the add */
-	return ldb_next_request(ac->module, add_req);
 }
 
-static int descriptor_add(struct ldb_module *module, struct ldb_request *req)
+static int descriptor_change(struct ldb_module *module, struct ldb_request *req)
 {
 	struct ldb_context *ldb;
 	struct ldb_request *search_req;
 	struct descriptor_context *ac;
-	struct ldb_dn *parent_dn;
+	struct ldb_dn *parent_dn, *dn;
+	struct ldb_message_element *sd_element;
 	int ret;
-	struct descriptor_data *data;
 	static const char * const descr_attrs[] = { "nTSecurityDescriptor", NULL };
 
-	data = talloc_get_type(ldb_module_get_private(module), struct descriptor_data);
 	ldb = ldb_module_get_ctx(module);
 
-	ldb_debug(ldb, LDB_DEBUG_TRACE,"descriptor_add: %s\n", ldb_dn_get_linearized(req->op.add.message->dn));
+	switch (req->operation) {
+	case LDB_ADD:
+		dn = req->op.add.message->dn;
+		break;
+	case LDB_MODIFY:
+		dn = req->op.mod.message->dn;
+		sd_element = ldb_msg_find_element(req->op.mod.message, "nTSecurityDescriptor");
+		if (!sd_element) {
+			return ldb_next_request(module, req);
+		}
+		break;
+	default:
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+	ldb_debug(ldb, LDB_DEBUG_TRACE,"descriptor_change: %s\n", ldb_dn_get_linearized(dn));
 
-	if (ldb_dn_is_special(req->op.add.message->dn)) {
+	if (ldb_dn_is_special(dn)) {
 		return ldb_next_request(module, req);
 	}
 
@@ -429,12 +574,12 @@ static int descriptor_add(struct ldb_module *module, struct ldb_request *req)
 	}
 
 	/* If there isn't a parent, just go on to the add processing */
-	if (ldb_dn_get_comp_num(ac->req->op.add.message->dn) == 1) {
+	if (ldb_dn_get_comp_num(dn) == 1) {
 		return descriptor_do_add(ac);
 	}
 
 	/* get copy of parent DN */
-	parent_dn = ldb_dn_get_parent(ac, ac->req->op.add.message->dn);
+	parent_dn = ldb_dn_get_parent(ac, dn);
 	if (parent_dn == NULL) {
 		ldb_oom(ldb);
 		return LDB_ERR_OPERATIONS_ERROR;
@@ -455,13 +600,7 @@ static int descriptor_add(struct ldb_module *module, struct ldb_request *req)
 
 	return ldb_next_request(ac->module, search_req);
 }
-/* TODO */
-static int descriptor_modify(struct ldb_module *module, struct ldb_request *req)
-{
-	struct ldb_context *ldb = ldb_module_get_ctx(module);
-	ldb_debug(ldb, LDB_DEBUG_TRACE,"descriptor_modify: %s\n", ldb_dn_get_linearized(req->op.mod.message->dn));
-	return ldb_next_request(module, req);
-}
+
 /* TODO */
 static int descriptor_rename(struct ldb_module *module, struct ldb_request *req)
 {
@@ -472,25 +611,14 @@ static int descriptor_rename(struct ldb_module *module, struct ldb_request *req)
 
 static int descriptor_init(struct ldb_module *module)
 {
-	struct ldb_context *ldb;
-	struct descriptor_data *data;
-
-	ldb = ldb_module_get_ctx(module);
-	data = talloc(module, struct descriptor_data);
-	if (data == NULL) {
-		ldb_oom(ldb);
-		return LDB_ERR_OPERATIONS_ERROR;
-	}
-
-	ldb_module_set_private(module, data);
 	return ldb_next_init(module);
 }
 
 
 _PUBLIC_ const struct ldb_module_ops ldb_descriptor_module_ops = {
 	.name		   = "descriptor",
-	.add           = descriptor_add,
-	.modify        = descriptor_modify,
+	.add           = descriptor_change,
+	.modify        = descriptor_change,
 	.rename        = descriptor_rename,
 	.init_context  = descriptor_init
 };
diff --git a/source4/libcli/security/create_descriptor.c b/source4/libcli/security/create_descriptor.c
index 82433fc02d1..d5bc7cba402 100644
--- a/source4/libcli/security/create_descriptor.c
+++ b/source4/libcli/security/create_descriptor.c
@@ -145,10 +145,8 @@ static bool postprocess_acl(struct security_acl *acl,
 	}
 	co = dom_sid_parse_talloc(tmp_ctx,  SID_CREATOR_OWNER);
 	cg = dom_sid_parse_talloc(tmp_ctx,  SID_CREATOR_GROUP);
-	for (i=0; i < acl->num_aces; i++){
+	for (i=0; i < acl->num_aces; i++) {
 		struct security_ace *ace = &acl->aces[i];
-		if (!(ace->flags == 0 || ace->flags & SEC_ACE_FLAG_INHERITED_ACE))
-			continue;
 		if (ace->flags & SEC_ACE_FLAG_INHERIT_ONLY)
 			continue;
 		if (dom_sid_equal(&ace->trustee, co)){
@@ -187,7 +185,7 @@ static struct security_acl *calculate_inherited_from_parent(TALLOC_CTX *mem_ctx,
 	for (i=0; i < acl->num_aces; i++){
 		struct security_ace *ace = &acl->aces[i];
 		if ((ace->flags & SEC_ACE_FLAG_CONTAINER_INHERIT) ||
-		    (ace->flags & SEC_ACE_FLAG_OBJECT_INHERIT)){
+		    (ace->flags & SEC_ACE_FLAG_OBJECT_INHERIT)) {
 			tmp_acl->aces = talloc_realloc(tmp_acl, tmp_acl->aces, struct security_ace,
 						       tmp_acl->num_aces+1);
 			if (tmp_acl->aces == NULL) {
@@ -212,7 +210,7 @@ static struct security_acl *calculate_inherited_from_parent(TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	if (is_container){
+	if (is_container) {
 		for (i=0; i < acl->num_aces; i++){
 			struct security_ace *ace = &acl->aces[i];
 
@@ -230,13 +228,13 @@ static struct security_acl *calculate_inherited_from_parent(TALLOC_CTX *mem_ctx,
 					return NULL;
 				}
 				inh_acl->aces[inh_acl->num_aces] = *ace;
-				inh_acl->aces[inh_acl->num_aces].flags |= SEC_ACE_FLAG_INHERIT_ONLY;
+				inh_acl->aces[inh_acl->num_aces].flags &= ~SEC_ACE_FLAG_INHERIT_ONLY;
 				inh_acl->aces[inh_acl->num_aces].flags |= SEC_ACE_FLAG_INHERITED_ACE;
 				inh_acl->num_aces++;
 			}
 		}
 	}
-	new_acl = security_acl_concatenate(mem_ctx,tmp_acl, inh_acl);
+	new_acl = security_acl_concatenate(mem_ctx, inh_acl, tmp_acl);
 	if (new_acl)
 		new_acl->revision = acl->revision;
 	talloc_free(tmp_ctx);
@@ -360,6 +358,12 @@ static bool compute_acl(int acl_type,
 						       object_list);
 			if (*new_acl == NULL)
 				goto final;
+			if (acl_type == SEC_DESC_DACL_PRESENT && new_sd->dacl)
+				new_sd->type |= SEC_DESC_DACL_AUTO_INHERITED;
+
+			if (acl_type == SEC_DESC_SACL_PRESENT && new_sd->sacl)
+				new_sd->type |= SEC_DESC_SACL_AUTO_INHERITED;
+
 			if (!postprocess_acl(*new_acl, new_sd->owner_sid,
 					     new_sd->group_sid, generic_map))
 				return false;
@@ -370,7 +374,7 @@ static bool compute_acl(int acl_type,
 			}
 		}
 		if (c_acl && !(inherit_flags & SEC_DEFAULT_DESCRIPTOR)){
-			struct security_acl *pr_acl, *tmp_acl, *tpr_acl;
+			struct security_acl *pr_acl = NULL, *tmp_acl = NULL, *tpr_acl = NULL;
 			tpr_acl = preprocess_creator_acl(new_sd, c_acl);
 			tmp_acl = calculate_inherited_from_creator(new_sd,
 						      tpr_acl,
@@ -380,27 +384,26 @@ static bool compute_acl(int acl_type,
 			cr_descr_log_acl(tmp_acl, __location__"Inherited from creator", level);
 			/* Todo some refactoring here! */
 			if (acl_type == SEC_DESC_DACL_PRESENT &&
-			    !(creator_sd->type & SECINFO_PROTECTED_DACL) &&
-			    (inherit_flags & SEC_DACL_AUTO_INHERIT)){
+			    !(creator_sd->type & SEC_DESC_DACL_PROTECTED) &&
+			    (inherit_flags & SEC_DACL_AUTO_INHERIT)) {
 				pr_acl = calculate_inherited_from_parent(new_sd,
 							     p_acl,
 							     is_container,
 							     object_list);
 				cr_descr_log_acl(pr_acl, __location__"Inherited from parent", level);
-				*new_acl = security_acl_concatenate(new_sd, tmp_acl, pr_acl);
 				new_sd->type |= SEC_DESC_DACL_AUTO_INHERITED;
 			}
 			else if (acl_type == SEC_DESC_SACL_PRESENT &&
-			    !(creator_sd->type & SECINFO_PROTECTED_SACL) &&
+			    !(creator_sd->type & SEC_DESC_SACL_PROTECTED) &&
 			    (inherit_flags & SEC_SACL_AUTO_INHERIT)){
 				pr_acl = calculate_inherited_from_parent(new_sd,
 							     p_acl,
 							     is_container,
 							     object_list);
 				cr_descr_log_acl(pr_acl, __location__"Inherited from parent", level);
-				*new_acl = security_acl_concatenate(new_sd, tmp_acl, pr_acl);
 				new_sd->type |= SEC_DESC_SACL_AUTO_INHERITED;
 			}
+			*new_acl = security_acl_concatenate(new_sd, tmp_acl, pr_acl);
 		}
 		if (*new_acl == NULL)
 			goto final;
diff --git a/source4/selftest/knownfail b/source4/selftest/knownfail
index dafb57d9473..527f2cd111a 100644
--- a/source4/selftest/knownfail
+++ b/source4/selftest/knownfail
@@ -57,7 +57,6 @@ samba4.winbind.struct.*.LOOKUP_NAME_SID   # Not yet working in winbind
 ^samba4.*base.delaywrite.*update of write time using SET_END_OF_FILE$
 ^samba4.*base.delaywrite.*update of write time using SET_ALLOCATION_SIZE$
 ^samba4.ldap.python \(dc\).Test add_ldif\(\) with BASE64 security descriptor input using WRONG domain SID$
-^samba4.ldap.python \(dc\).Testing ldb.add_ldif\(\) for nTSecurityDescriptor
 # some operations don't work over the CIFS NTVFS backend yet (eg. root_fid)
 samba4.ntvfs.cifs.base.createx_access 
 samba4.ntvfs.cifs.base.createx_sharemodes_dir
diff --git a/source4/selftest/skip b/source4/selftest/skip
index bd3263d21e7..2875acca877 100644
--- a/source4/selftest/skip
+++ b/source4/selftest/skip
@@ -65,7 +65,6 @@ samba4.rpc.countcalls # this is not useful now we have full IDL
 samba4.rap.scan # same thing here - we have docs now
 samba4.gensec.python # not finished
 bench # don't run benchmarks in our selftest
-samba4.ldap.secdesc.python # this is expected to fail until the acl module is complete
 trans2.scan # uses huge number of file descriptors
 base.scan.ioctl # bad idea in make test
 base.scan.pipe_number # bad idea in make test