From 16b377276ee82c04d069666e53deaa95a7633dd4 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 12 Dec 2019 14:44:57 +1300 Subject: CVE-2019-14902 dsdb: Change basis of descriptor module deferred processing to be GUIDs We can not process on the basis of a DN, as the DN may have changed in a rename, not only that this module can see, but also from repl_meta_data below. Therefore remove all the complex tree-based change processing, leaving only a tree-based sort of the possible objects to be changed, and a single stopped_dn variable containing the DN to stop processing below (after a no-op change). BUG: https://bugzilla.samba.org/show_bug.cgi?id=12497 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/repl_secdesc | 1 - source4/dsdb/samdb/ldb_modules/acl_util.c | 4 +- source4/dsdb/samdb/ldb_modules/descriptor.c | 296 ++++++++++++------------ source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 7 +- source4/dsdb/samdb/samdb.h | 2 +- 5 files changed, 156 insertions(+), 154 deletions(-) delete mode 100644 selftest/knownfail.d/repl_secdesc diff --git a/selftest/knownfail.d/repl_secdesc b/selftest/knownfail.d/repl_secdesc deleted file mode 100644 index 9dd632d99ed..00000000000 --- a/selftest/knownfail.d/repl_secdesc +++ /dev/null @@ -1 +0,0 @@ -^samba4.drs.repl_secdesc.python\(.*\).repl_secdesc.ReplAclTestCase.test_acl_inheirt_renamed_child_object diff --git a/source4/dsdb/samdb/ldb_modules/acl_util.c b/source4/dsdb/samdb/ldb_modules/acl_util.c index 6d645b10fe2..b9931795e19 100644 --- a/source4/dsdb/samdb/ldb_modules/acl_util.c +++ b/source4/dsdb/samdb/ldb_modules/acl_util.c @@ -286,7 +286,7 @@ uint32_t dsdb_request_sd_flags(struct ldb_request *req, bool *explicit) int dsdb_module_schedule_sd_propagation(struct ldb_module *module, struct ldb_dn *nc_root, - struct ldb_dn *dn, + struct GUID guid, bool include_self) { struct ldb_context *ldb = ldb_module_get_ctx(module); @@ -299,7 +299,7 @@ int dsdb_module_schedule_sd_propagation(struct ldb_module *module, } op->nc_root = nc_root; - op->dn = dn; + op->guid = guid; op->include_self = include_self; ret = dsdb_module_extended(module, op, NULL, diff --git a/source4/dsdb/samdb/ldb_modules/descriptor.c b/source4/dsdb/samdb/ldb_modules/descriptor.c index b9f465fc36f..daa08c2ebc7 100644 --- a/source4/dsdb/samdb/ldb_modules/descriptor.c +++ b/source4/dsdb/samdb/ldb_modules/descriptor.c @@ -46,9 +46,8 @@ struct descriptor_changes { struct descriptor_changes *prev, *next; - struct descriptor_changes *children; struct ldb_dn *nc_root; - struct ldb_dn *dn; + struct GUID guid; bool force_self; bool force_children; struct ldb_dn *stopped_dn; @@ -771,7 +770,8 @@ static int descriptor_modify(struct ldb_module *module, struct ldb_request *req) current_attrs, DSDB_FLAG_NEXT_MODULE | DSDB_FLAG_AS_SYSTEM | - DSDB_SEARCH_SHOW_RECYCLED, + DSDB_SEARCH_SHOW_RECYCLED | + DSDB_SEARCH_SHOW_EXTENDED_DN, req); if (ret != LDB_SUCCESS) { ldb_debug(ldb, LDB_DEBUG_ERROR,"descriptor_modify: Could not find %s\n", @@ -832,7 +832,7 @@ static int descriptor_modify(struct ldb_module *module, struct ldb_request *req) user_sd = old_sd; } - sd = get_new_descriptor(module, dn, req, + sd = get_new_descriptor(module, current_res->msgs[0]->dn, req, objectclass, parent_sd, user_sd, old_sd, sd_flags); if (sd == NULL) { @@ -869,18 +869,32 @@ static int descriptor_modify(struct ldb_module *module, struct ldb_request *req) return ldb_oom(ldb); } } else if (cmp_ret != 0) { + struct GUID guid; struct ldb_dn *nc_root; + NTSTATUS status; - ret = dsdb_find_nc_root(ldb, msg, dn, &nc_root); + ret = dsdb_find_nc_root(ldb, + msg, + current_res->msgs[0]->dn, + &nc_root); if (ret != LDB_SUCCESS) { return ldb_oom(ldb); } + status = dsdb_get_extended_dn_guid(current_res->msgs[0]->dn, + &guid, + "GUID"); + if (!NT_STATUS_IS_OK(status)) { + return ldb_operr(ldb); + } + /* * Force SD propagation on children of this record */ - ret = dsdb_module_schedule_sd_propagation(module, nc_root, - dn, false); + ret = dsdb_module_schedule_sd_propagation(module, + nc_root, + guid, + false); if (ret != LDB_SUCCESS) { return ldb_operr(ldb); } @@ -963,20 +977,31 @@ static int descriptor_rename(struct ldb_module *module, struct ldb_request *req) if (ldb_dn_compare(olddn, newdn) != 0) { struct ldb_dn *nc_root; + struct GUID guid; ret = dsdb_find_nc_root(ldb, req, newdn, &nc_root); if (ret != LDB_SUCCESS) { return ldb_oom(ldb); } - /* - * Force SD propagation on this record (get a new - * inherited SD from the potentially new parent - */ - ret = dsdb_module_schedule_sd_propagation(module, nc_root, - newdn, true); - if (ret != LDB_SUCCESS) { - return ldb_operr(ldb); + ret = dsdb_module_guid_by_dn(module, + olddn, + &guid, + req); + if (ret == LDB_SUCCESS) { + /* + * Without disturbing any errors if the olddn + * does not exit, force SD propagation on + * this record (get a new inherited SD from + * the potentially new parent + */ + ret = dsdb_module_schedule_sd_propagation(module, + nc_root, + guid, + true); + if (ret != LDB_SUCCESS) { + return ldb_operr(ldb); + } } } @@ -992,9 +1017,7 @@ static int descriptor_extended_sec_desc_propagation(struct ldb_module *module, struct ldb_context *ldb = ldb_module_get_ctx(module); struct dsdb_extended_sec_desc_propagation_op *op; TALLOC_CTX *parent_mem = NULL; - struct descriptor_changes *parent_change = NULL; struct descriptor_changes *c; - int ret; op = talloc_get_type(req->op.extended.data, struct dsdb_extended_sec_desc_propagation_op); @@ -1011,32 +1034,6 @@ static int descriptor_extended_sec_desc_propagation(struct ldb_module *module, parent_mem = descriptor_private->trans_mem; - for (c = descriptor_private->changes; c; c = c->next) { - ret = ldb_dn_compare(c->nc_root, op->nc_root); - if (ret != 0) { - continue; - } - - ret = ldb_dn_compare(c->dn, op->dn); - if (ret == 0) { - if (op->include_self) { - c->force_self = true; - } else { - c->force_children = true; - } - return ldb_module_done(req, NULL, NULL, LDB_SUCCESS); - } - - ret = ldb_dn_compare_base(c->dn, op->dn); - if (ret != 0) { - continue; - } - - parent_mem = c; - parent_change = c; - break; - } - c = talloc_zero(parent_mem, struct descriptor_changes); if (c == NULL) { return ldb_module_oom(module); @@ -1045,21 +1042,14 @@ static int descriptor_extended_sec_desc_propagation(struct ldb_module *module, if (c->nc_root == NULL) { return ldb_module_oom(module); } - c->dn = ldb_dn_copy(c, op->dn); - if (c->dn == NULL) { - return ldb_module_oom(module); - } + c->guid = op->guid; if (op->include_self) { c->force_self = true; } else { c->force_children = true; } - if (parent_change != NULL) { - DLIST_ADD_END(parent_change->children, c); - } else { - DLIST_ADD_END(descriptor_private->changes, c); - } + DLIST_ADD_END(descriptor_private->changes, c); return ldb_module_done(req, NULL, NULL, LDB_SUCCESS); } @@ -1179,41 +1169,75 @@ static int descriptor_sd_propagation_msg_sort(struct ldb_message **m1, return ldb_dn_compare(dn2, dn1); } -static int descriptor_sd_propagation_dn_sort(struct ldb_dn *dn1, - struct ldb_dn *dn2) -{ - /* - * This sorts in tree order, parents first - */ - return ldb_dn_compare(dn2, dn1); -} - static int descriptor_sd_propagation_recursive(struct ldb_module *module, struct descriptor_changes *change) { - struct ldb_context *ldb = ldb_module_get_ctx(module); + struct ldb_result *guid_res = NULL; struct ldb_result *res = NULL; unsigned int i; const char * const no_attrs[] = { "@__NONE__", NULL }; - struct descriptor_changes *c; - struct descriptor_changes *stopped_stack = NULL; - enum ldb_scope scope; + struct ldb_dn *stopped_dn = NULL; + struct GUID_txt_buf guid_buf; int ret; + bool stop = false; /* - * First confirm this object has children, or exists (depending on change->force_self) + * First confirm this object has children, or exists + * (depending on change->force_self) * * LDB_SCOPE_SUBTREE searches are expensive. * - * Note: that we do not search for deleted/recycled objects - * * We know this is safe against a rename race as we are in the * prepare_commit(), so must be in a transaction. */ + + /* Find the DN by GUID, as this is stable under rename */ + ret = dsdb_module_search(module, + change, + &guid_res, + change->nc_root, + LDB_SCOPE_SUBTREE, + no_attrs, + DSDB_FLAG_NEXT_MODULE | + DSDB_FLAG_AS_SYSTEM | + DSDB_SEARCH_SHOW_DELETED | + DSDB_SEARCH_SHOW_RECYCLED, + NULL, /* parent_req */ + "(objectGUID=%s)", + GUID_buf_string(&change->guid, + &guid_buf)); + + if (ret != LDB_SUCCESS) { + return ret; + } + + if (guid_res->count != 1) { + /* + * We were just given this GUID during the same + * transaction, if it is missing this is a big + * problem. + * + * Cleanup of tombstones does not trigger this module + * as it just does a delete. + */ + ldb_asprintf_errstring(ldb_module_get_ctx(module), + "failed to find GUID %s under %s " + "for transaction-end SD inheritance: %d results", + GUID_buf_string(&change->guid, + &guid_buf), + ldb_dn_get_linearized(change->nc_root), + guid_res->count); + return LDB_ERR_OPERATIONS_ERROR; + } + + /* + * OK, so there was a parent, are there children? Note: that + * this time we do not search for deleted/recycled objects + */ ret = dsdb_module_search(module, change, &res, - change->dn, + guid_res->msgs[0]->dn, LDB_SCOPE_ONELEVEL, no_attrs, DSDB_FLAG_NEXT_MODULE | @@ -1221,26 +1245,55 @@ static int descriptor_sd_propagation_recursive(struct ldb_module *module, NULL, /* parent_req */ "(objectClass=*)"); if (ret != LDB_SUCCESS) { + /* + * LDB_ERR_NO_SUCH_OBJECT, say if the DN was a deleted + * object, is ignored by the caller + */ return ret; } if (res->count == 0 && !change->force_self) { + /* All done, no children */ TALLOC_FREE(res); return LDB_SUCCESS; - } else if (res->count == 0 && change->force_self) { - scope = LDB_SCOPE_BASE; - } else { - scope = LDB_SCOPE_SUBTREE; } /* + * First, if we are in force_self mode (eg renamed under new + * parent) then apply the SD to the top object + */ + if (change->force_self) { + ret = descriptor_sd_propagation_object(module, + guid_res->msgs[0], + &stop); + if (ret != LDB_SUCCESS) { + TALLOC_FREE(guid_res); + return ret; + } + + if (stop == true && !change->force_children) { + /* There was no change, nothing more to do */ + TALLOC_FREE(guid_res); + return LDB_SUCCESS; + } + + if (res->count == 0) { + /* All done! */ + TALLOC_FREE(guid_res); + return LDB_SUCCESS; + } + } + + /* + * Look for children + * * Note: that we do not search for deleted/recycled objects */ ret = dsdb_module_search(module, change, &res, - change->dn, - scope, + guid_res->msgs[0]->dn, + LDB_SCOPE_SUBTREE, no_attrs, DSDB_FLAG_NEXT_MODULE | DSDB_FLAG_AS_SYSTEM, @@ -1253,90 +1306,39 @@ static int descriptor_sd_propagation_recursive(struct ldb_module *module, TYPESAFE_QSORT(res->msgs, res->count, descriptor_sd_propagation_msg_sort); - for (c = change->children; c; c = c->next) { - struct ldb_message *msg = NULL; - - BINARY_ARRAY_SEARCH_P(res->msgs, res->count, dn, c->dn, - descriptor_sd_propagation_dn_sort, - msg); - - if (msg == NULL) { - ldb_debug(ldb, LDB_DEBUG_WARNING, - "descriptor_sd_propagation_recursive: " - "%s not found under %s", - ldb_dn_get_linearized(c->dn), - ldb_dn_get_linearized(change->dn)); - continue; - } - - msg->elements = (struct ldb_message_element *)c; - } - - DLIST_ADD(stopped_stack, change); - - if (change->force_self) { - i = 0; - } else { - i = 1; - } - - for (; i < res->count; i++) { - struct descriptor_changes *cur; - bool stop = false; - - cur = talloc_get_type(res->msgs[i]->elements, - struct descriptor_changes); - res->msgs[i]->elements = NULL; - res->msgs[i]->num_elements = 0; - - if (cur != NULL) { - DLIST_REMOVE(change->children, cur); - } else if (i == 0) { + /* We start from 1, the top object has been done */ + for (i = 1; i < res->count; i++) { + /* + * ldb_dn_compare_base() does not match for NULL but + * this is clearer + */ + if (stopped_dn != NULL) { + ret = ldb_dn_compare_base(stopped_dn, + res->msgs[i]->dn); /* - * in the change->force_self case - * res->msgs[0]->elements was not overwritten, - * so set cur here + * Skip further processing of this + * sub-subtree */ - cur = change; - } - - for (c = stopped_stack; c; c = stopped_stack) { - ret = ldb_dn_compare_base(c->dn, - res->msgs[i]->dn); - if (ret == 0) { - break; - } - - c->stopped_dn = NULL; - DLIST_REMOVE(stopped_stack, c); - } - - if (cur != NULL) { - DLIST_ADD(stopped_stack, cur); - } - - if (stopped_stack->stopped_dn != NULL) { - ret = ldb_dn_compare_base(stopped_stack->stopped_dn, - res->msgs[i]->dn); if (ret == 0) { continue; } - stopped_stack->stopped_dn = NULL; } - - ret = descriptor_sd_propagation_object(module, res->msgs[i], + ret = descriptor_sd_propagation_object(module, + res->msgs[i], &stop); if (ret != LDB_SUCCESS) { return ret; } - if (cur != NULL && cur->force_children) { - continue; - } - if (stop) { - stopped_stack->stopped_dn = res->msgs[i]->dn; - continue; + /* + * If this child didn't change, then nothing + * under it needs to change + * + * res has been sorted into tree order so the + * next few entries can be skipped + */ + stopped_dn = res->msgs[i]->dn; } } diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index e67c3b0281e..a2a6bcc98f3 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -5538,7 +5538,8 @@ static int replmd_replicated_apply_add(struct replmd_replicated_request *ar) */ ret = dsdb_module_schedule_sd_propagation(ar->module, ar->objs->partition_dn, - msg->dn, true); + ar->objs->objects[ar->index_current].object_guid, + true); if (ret != LDB_SUCCESS) { return replmd_replicated_request_error(ar, ret); } @@ -6323,7 +6324,7 @@ static int replmd_replicated_apply_merge(struct replmd_replicated_request *ar) */ ret = dsdb_module_schedule_sd_propagation(ar->module, ar->objs->partition_dn, - msg->dn, + ar->objs->objects[ar->index_current].object_guid, true); if (ret != LDB_SUCCESS) { return ldb_operr(ldb); @@ -6343,7 +6344,7 @@ static int replmd_replicated_apply_merge(struct replmd_replicated_request *ar) */ ret = dsdb_module_schedule_sd_propagation(ar->module, ar->objs->partition_dn, - msg->dn, + ar->objs->objects[ar->index_current].object_guid, false); if (ret != LDB_SUCCESS) { return ldb_operr(ldb); diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index e1b0e4aa4e3..3f47b863a83 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -338,7 +338,7 @@ struct dsdb_extended_allocate_rid { #define DSDB_EXTENDED_SEC_DESC_PROPAGATION_OID "1.3.6.1.4.1.7165.4.4.7" struct dsdb_extended_sec_desc_propagation_op { struct ldb_dn *nc_root; - struct ldb_dn *dn; + struct GUID guid; bool include_self; }; -- cgit v1.2.1