From 73f3ece8b2b44ac4b3323a08fb969f29bf2b0380 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 31 Jan 2023 13:29:05 +1300 Subject: s4-drs: Make drs_ObjectIdentifier_to_dn() safer and able to cope with DummyDN values We want to totally ignore the string DN if there is a GUID, as clients like "Microsoft Azure AD connect cloud sync" will set a literal "DummyDN" string. BUG: https://bugzilla.samba.org/show_bug.cgi?id=10635 Signed-off-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- selftest/knownfail.d/getncchanges | 3 - source4/dsdb/common/dsdb_dn.c | 150 +++++++++++++++++++++++++++--- source4/rpc_server/drsuapi/getncchanges.c | 30 +++--- source4/rpc_server/drsuapi/updaterefs.c | 14 ++- 4 files changed, 165 insertions(+), 32 deletions(-) diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges index 7415f572710..1ced3870b95 100644 --- a/selftest/knownfail.d/getncchanges +++ b/selftest/knownfail.d/getncchanges @@ -9,7 +9,4 @@ samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegri ^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_InvalidNC_DummyDN_InvalidGUID_REPL_SECRET ^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_InvalidNC_DummyDN_InvalidGUID_REPL_OBJ ^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_InvalidDestDSA_and_GUID_RID_ALLOC -^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_DummyDN_valid_GUID_REPL_OBJ ^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_DummyDN_valid_GUID_REPL_SECRET -^samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_InvalidNC_DummyDN_InvalidGUID_full_repl -^samba4.drs.repl_rodc.python\(.*\).repl_rodc.DrsRodcTestCase.test_admin_repl_secrets_DummyDN_GUID diff --git a/source4/dsdb/common/dsdb_dn.c b/source4/dsdb/common/dsdb_dn.c index 04aab1593b1..c86848fd697 100644 --- a/source4/dsdb/common/dsdb_dn.c +++ b/source4/dsdb/common/dsdb_dn.c @@ -359,10 +359,12 @@ int dsdb_dn_string_comparison(struct ldb_context *ldb, void *mem_ctx, } /* - format a drsuapi_DsReplicaObjectIdentifier naming context as a string + * format a drsuapi_DsReplicaObjectIdentifier naming context as a string for debugging + * + * When forming a DN for DB access you must use drs_ObjectIdentifier_to_dn() */ -char *drs_ObjectIdentifier_to_string(TALLOC_CTX *mem_ctx, - struct drsuapi_DsReplicaObjectIdentifier *nc) +char *drs_ObjectIdentifier_to_debug_string(TALLOC_CTX *mem_ctx, + struct drsuapi_DsReplicaObjectIdentifier *nc) { char *ret = NULL; TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); @@ -386,21 +388,147 @@ char *drs_ObjectIdentifier_to_string(TALLOC_CTX *mem_ctx, return ret; } -struct ldb_dn *drs_ObjectIdentifier_to_dn(TALLOC_CTX *mem_ctx, - struct ldb_context *ldb, - struct drsuapi_DsReplicaObjectIdentifier *nc) +/* + * Safely convert a drsuapi_DsReplicaObjectIdentifier into an LDB DN + * + * We need to have GUID and SID prority and not allow extended + * components in the DN. + * + * We must also totally honour the prority even if the string DN is not valid or able to parse as a DN. + */ +static struct ldb_dn *drs_ObjectIdentifier_to_dn(TALLOC_CTX *mem_ctx, + struct ldb_context *ldb, + struct drsuapi_DsReplicaObjectIdentifier *nc) { - char *dn_string = drs_ObjectIdentifier_to_string(mem_ctx, nc); - struct ldb_dn *new_dn; - new_dn = ldb_dn_new(mem_ctx, ldb, dn_string); - talloc_free(dn_string); - return new_dn; + struct ldb_dn *new_dn = NULL; + + if (!GUID_all_zero(&nc->guid)) { + struct GUID_txt_buf buf; + char *guid = GUID_buf_string(&nc->guid, &buf); + + new_dn = ldb_dn_new_fmt(mem_ctx, + ldb, + "", + guid); + if (new_dn == NULL) { + DBG_ERR("Failed to prepare drs_ObjectIdentifier " + "GUID %s into a DN\n", + guid); + return NULL; + } + + return new_dn; + } + + if (nc->__ndr_size_sid != 0 && nc->sid.sid_rev_num != 0) { + struct dom_sid_buf buf; + char *sid = dom_sid_str_buf(&nc->sid, &buf); + + new_dn = ldb_dn_new_fmt(mem_ctx, + ldb, + "", + sid); + if (new_dn == NULL) { + DBG_ERR("Failed to prepare drs_ObjectIdentifier " + "SID %s into a DN\n", + sid); + return NULL; + } + return new_dn; + } + + if (nc->__ndr_size_dn != 0 && nc->dn) { + int dn_comp_num = 0; + bool new_dn_valid = false; + + new_dn = ldb_dn_new(mem_ctx, ldb, nc->dn); + if (new_dn == NULL) { + /* Set to WARNING as this is user-controlled, don't print the value into the logs */ + DBG_WARNING("Failed to parse string DN in " + "drs_ObjectIdentifier into an LDB DN\n"); + return NULL; + } + + new_dn_valid = ldb_dn_validate(new_dn); + if (!new_dn_valid) { + /* + * Set to WARNING as this is user-controlled, + * but can print the value into the logs as it + * parsed a bit + */ + DBG_WARNING("Failed to validate string DN [%s] in " + "drs_ObjectIdentifier as an LDB DN\n", + ldb_dn_get_linearized(new_dn)); + return NULL; + } + + dn_comp_num = ldb_dn_get_comp_num(new_dn); + if (dn_comp_num <= 0) { + /* + * Set to WARNING as this is user-controlled, + * but can print the value into the logs as it + * parsed a bit + */ + DBG_WARNING("DN [%s] in drs_ObjectIdentifier " + "must have 1 or more components\n", + ldb_dn_get_linearized(new_dn)); + return NULL; + } + + if (ldb_dn_is_special(new_dn)) { + /* + * Set to WARNING as this is user-controlled, + * but can print the value into the logs as it + * parsed a bit + */ + DBG_WARNING("New string DN [%s] in " + "drs_ObjectIdentifier is a " + "special LDB DN\n", + ldb_dn_get_linearized(new_dn)); + return NULL; + } + + /* + * We want this just to be a string DN, extended + * components are manually handled above + */ + if (ldb_dn_has_extended(new_dn)) { + /* + * Set to WARNING as this is user-controlled, + * but can print the value into the logs as it + * parsed a bit + */ + DBG_WARNING("Refusing to parse New string DN [%s] in " + "drs_ObjectIdentifier as an " + "extended LDB DN " + "(GUIDs and SIDs should be in the " + ".guid and .sid IDL elelements, " + "not in the string\n", + ldb_dn_get_extended_linearized(mem_ctx, + new_dn, + 1)); + return NULL; + } + return new_dn; + } + + DBG_WARNING("Refusing to parse empty string DN " + "(and no GUID or SID) " + "drs_ObjectIdentifier into a empty " + "(eg RootDSE) LDB DN\n"); + return NULL; } /* * Safely convert a drsuapi_DsReplicaObjectIdentifier into a validated * LDB DN of an existing DB entry, and/or find the NC root * + * We need to have GUID and SID prority and not allow extended + * components in the DN. + * + * We must also totally honour the prority even if the string DN is + * not valid or able to parse as a DN. + * * Finally, we must return the DN as found in the DB, as otherwise a * subsequence ldb_dn_compare(dn, nc_root) will fail (as this is based * on the string components). diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c index b2dea15ac8e..02a6dd3f803 100644 --- a/source4/rpc_server/drsuapi/getncchanges.c +++ b/source4/rpc_server/drsuapi/getncchanges.c @@ -1098,7 +1098,7 @@ static WERROR getncchanges_rid_alloc(struct drsuapi_bind_state *b_state, NULL); if (ret != LDB_SUCCESS) { DBG_ERR("RID Alloc request for invalid DN %s: %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, req10->naming_context), + drs_ObjectIdentifier_to_debug_string(mem_ctx, req10->naming_context), ldb_strerror(ret)); ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; return WERR_OK; @@ -1106,8 +1106,9 @@ static WERROR getncchanges_rid_alloc(struct drsuapi_bind_state *b_state, if (ldb_dn_compare(req_dn, *rid_manager_dn) != 0) { /* that isn't the RID Manager DN */ - DEBUG(0,(__location__ ": RID Alloc request for wrong DN %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, req10->naming_context))); + DBG_ERR("RID Alloc request for wrong DN %s\n", + drs_ObjectIdentifier_to_debug_string(mem_ctx, + req10->naming_context)); ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; return WERR_OK; } @@ -1200,7 +1201,7 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, WERROR werr; DEBUG(3,(__location__ ": DRSUAPI_EXOP_REPL_SECRET extended op on %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot))); + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot))); /* * we need to work out if we will allow this DC to @@ -1268,7 +1269,7 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, NULL); if (ret != LDB_SUCCESS) { DBG_ERR("RevealSecretRequest for for invalid DN %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot)); + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot)); goto failed; } @@ -1353,7 +1354,7 @@ static WERROR getncchanges_repl_obj(struct drsuapi_bind_state *b_state, struct drsuapi_DsReplicaObjectIdentifier *ncRoot = req10->naming_context; DEBUG(3,(__location__ ": DRSUAPI_EXOP_REPL_OBJ extended op on %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot))); + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot))); ctr6->extended_ret = DRSUAPI_EXOP_ERR_SUCCESS; return WERR_OK; @@ -1387,8 +1388,9 @@ static WERROR getncchanges_change_master(struct drsuapi_bind_state *b_state, &req_dn, NULL); if (ret != LDB_SUCCESS) { /* that is not a valid dn */ - DEBUG(0,(__location__ ": FSMO role transfer request for invalid DN %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, req10->naming_context))); + DBG_ERR("FSMO role transfer request for invalid DN %s: %s\n", + drs_ObjectIdentifier_to_debug_string(mem_ctx, req10->naming_context), + ldb_strerror(ret)); ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; return WERR_OK; } @@ -1416,7 +1418,7 @@ static WERROR getncchanges_change_master(struct drsuapi_bind_state *b_state, if (ret != LDB_SUCCESS) { /* that is not a valid dn */ DBG_ERR("FSMO role transfer request for invalid DN %s: %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, req10->naming_context), + drs_ObjectIdentifier_to_debug_string(mem_ctx, req10->naming_context), ldb_strerror(ret)); ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; return WERR_OK; @@ -2906,7 +2908,7 @@ allowed: * implicitly but not got the DN out */ DBG_ERR("Bad DN '%s'\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot)); + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot)); return WERR_DS_DRA_INVALID_PARAMETER; } if (ldb_dn_compare(new_dn, getnc_state->ncRoot_dn) != 0) { @@ -3063,7 +3065,7 @@ allowed: if (!ldb_dn_validate(getnc_state->ncRoot_dn) || ldb_dn_is_null(getnc_state->ncRoot_dn)) { DEBUG(0,(__location__ ": Bad DN '%s'\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot))); + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot))); return WERR_DS_DRA_INVALID_PARAMETER; } @@ -3502,7 +3504,8 @@ allowed: &ureq); if (!W_ERROR_IS_OK(werr)) { DEBUG(0,(__location__ ": Failed UpdateRefs on %s for %s in DsGetNCChanges - %s\n", - drs_ObjectIdentifier_to_string(mem_ctx, ncRoot), ureq.dest_dsa_dns_name, + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), + ureq.dest_dsa_dns_name, win_errstr(werr))); } } @@ -3632,7 +3635,8 @@ allowed: DEBUG(r->out.ctr->ctr6.more_data?4:2, ("DsGetNCChanges with uSNChanged >= %llu flags 0x%08x on %s gave %u objects (done %u/%u) %u links (done %u/%u (as %s))\n", (unsigned long long)(req10->highwatermark.highest_usn+1), - req10->replica_flags, drs_ObjectIdentifier_to_string(mem_ctx, ncRoot), + req10->replica_flags, + drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), r->out.ctr->ctr6.object_count, i, r->out.ctr->ctr6.more_data?getnc_state->num_records:i, r->out.ctr->ctr6.linked_attributes_count, diff --git a/source4/rpc_server/drsuapi/updaterefs.c b/source4/rpc_server/drsuapi/updaterefs.c index 5d2bc6e949c..0be675ffe21 100644 --- a/source4/rpc_server/drsuapi/updaterefs.c +++ b/source4/rpc_server/drsuapi/updaterefs.c @@ -206,7 +206,7 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, DEBUG(4,("DsReplicaUpdateRefs for host '%s' with GUID %s options 0x%08x nc=%s\n", req->dest_dsa_dns_name, GUID_string(mem_ctx, &req->dest_dsa_guid), req->options, - drs_ObjectIdentifier_to_string(mem_ctx, req->naming_context))); + drs_ObjectIdentifier_to_debug_string(mem_ctx, req->naming_context))); /* * 4.1.26.2 Server Behavior of the IDL_DRSUpdateRefs Method @@ -228,14 +228,18 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, sam_ctx, req->naming_context, &dn_normalised, &nc_root); if (ret != LDB_SUCCESS) { - DBG_WARNING("Didn't find a nc for %s\n", - ldb_dn_get_linearized(dn_normalised)); + DBG_WARNING("Didn't find a nc for %s: %s\n", + drs_ObjectIdentifier_to_debug_string(mem_ctx, + req->naming_context), + ldb_errstring(sam_ctx)); return WERR_DS_DRA_BAD_NC; } if (ldb_dn_compare(dn_normalised, nc_root) != 0) { - DBG_NOTICE("dn %s is not equal to %s\n", + DBG_NOTICE("dn %s is not equal to %s (from %s)\n", ldb_dn_get_linearized(dn_normalised), - ldb_dn_get_linearized(nc_root)); + ldb_dn_get_linearized(nc_root), + drs_ObjectIdentifier_to_debug_string(mem_ctx, + req->naming_context)); return WERR_DS_DRA_BAD_NC; } -- cgit v1.2.1