From 7c32d3d75aa31b868527d992e08e8d63fc76faac Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 12 Dec 2022 09:47:36 +1300 Subject: s4-dsdb: rework drs_ObjectIdentifier_to_dn() into drs_ObjectIdentifier_to_dn_and_nc_root() This make this funciton the gatekeeper between the wire format and the internal struct ldb_dn, checking if the DN exists and which NC it belongs to along the way, and presenting only a DB-returned DN for internal processing. BUG: https://bugzilla.samba.org/show_bug.cgi?id=10635 Signed-off-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher (cherry picked from commit aee2039e63ceeb5e69a0461fb77e0f18278e4dc4) --- selftest/knownfail.d/getncchanges | 4 +- source4/dsdb/common/dsdb_dn.c | 33 ++++++++++++++ source4/rpc_server/drsuapi/drsutil.c | 28 +++++++++--- source4/rpc_server/drsuapi/getncchanges.c | 72 +++++++++++++++++++++++++------ source4/rpc_server/drsuapi/updaterefs.c | 29 ++++++++----- 5 files changed, 133 insertions(+), 33 deletions(-) diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges index 317d78c41b1..7415f572710 100644 --- a/selftest/knownfail.d/getncchanges +++ b/selftest/knownfail.d/getncchanges @@ -6,12 +6,10 @@ samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegri samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_multivalued_links\(promoted_dc\) # New tests for GetNCChanges with a GUID and a bad DN, like Azure AD Cloud Sync ^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_InvalidDestDSA_and_GUID -^samba4.drs.getnc_exop.python\(.*\).getnc_exop.DrsReplicaSyncTestCase.test_InvalidNC_DummyDN_InvalidGUID +^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_InvalidNC_DummyDN_InvalidGUID_RID_ALLOC ^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_DummyDN_valid_GUID_full_repl ^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 e348ab6aa94..04aab1593b1 100644 --- a/source4/dsdb/common/dsdb_dn.c +++ b/source4/dsdb/common/dsdb_dn.c @@ -396,3 +396,36 @@ struct ldb_dn *drs_ObjectIdentifier_to_dn(TALLOC_CTX *mem_ctx, talloc_free(dn_string); return new_dn; } + +/* + * Safely convert a drsuapi_DsReplicaObjectIdentifier into a validated + * LDB DN of an existing DB entry, and/or find the NC root + * + * 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). + */ +int drs_ObjectIdentifier_to_dn_and_nc_root(TALLOC_CTX *mem_ctx, + struct ldb_context *ldb, + struct drsuapi_DsReplicaObjectIdentifier *nc, + struct ldb_dn **normalised_dn, + struct ldb_dn **nc_root) +{ + int ret; + struct ldb_dn *new_dn = NULL; + + new_dn = drs_ObjectIdentifier_to_dn(mem_ctx, + ldb, + nc); + if (new_dn == NULL) { + return LDB_ERR_INVALID_DN_SYNTAX; + } + + ret = dsdb_normalise_dn_and_find_nc_root(ldb, + mem_ctx, + new_dn, + normalised_dn, + nc_root); + TALLOC_FREE(new_dn); + return ret; +} diff --git a/source4/rpc_server/drsuapi/drsutil.c b/source4/rpc_server/drsuapi/drsutil.c index 7897c35d2e9..48423bb6655 100644 --- a/source4/rpc_server/drsuapi/drsutil.c +++ b/source4/rpc_server/drsuapi/drsutil.c @@ -191,8 +191,19 @@ WERROR drs_security_access_check(struct ldb_context *sam_ctx, struct drsuapi_DsReplicaObjectIdentifier *nc, const char *ext_right) { - struct ldb_dn *dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, nc); + struct ldb_dn *dn; WERROR werr; + int ret; + + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, + sam_ctx, + nc, + &dn, + NULL); + if (ret != LDB_SUCCESS) { + return WERR_DS_DRA_BAD_DN; + } + werr = drs_security_access_check_log(sam_ctx, mem_ctx, token, dn, ext_right); talloc_free(dn); return werr; @@ -207,17 +218,20 @@ WERROR drs_security_access_check_nc_root(struct ldb_context *sam_ctx, struct drsuapi_DsReplicaObjectIdentifier *nc, const char *ext_right) { - struct ldb_dn *dn, *nc_root; + struct ldb_dn *nc_root; WERROR werr; int ret; - dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, nc); - W_ERROR_HAVE_NO_MEMORY(dn); - ret = dsdb_find_nc_root(sam_ctx, dn, dn, &nc_root); + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, + sam_ctx, + nc, + NULL, + &nc_root); if (ret != LDB_SUCCESS) { - return WERR_DS_CANT_FIND_EXPECTED_NC; + return WERR_DS_DRA_BAD_NC; } + werr = drs_security_access_check_log(sam_ctx, mem_ctx, token, nc_root, ext_right); - talloc_free(dn); + talloc_free(nc_root); return werr; } diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c index 7f50587dd1e..b2dea15ac8e 100644 --- a/source4/rpc_server/drsuapi/getncchanges.c +++ b/source4/rpc_server/drsuapi/getncchanges.c @@ -1091,9 +1091,20 @@ static WERROR getncchanges_rid_alloc(struct drsuapi_bind_state *b_state, return WERR_DS_DRA_INTERNAL_ERROR; } - req_dn = drs_ObjectIdentifier_to_dn(mem_ctx, ldb, req10->naming_context); - if (!ldb_dn_validate(req_dn) || - ldb_dn_compare(req_dn, *rid_manager_dn) != 0) { + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, + ldb, + req10->naming_context, + &req_dn, + 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), + ldb_strerror(ret)); + ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; + return WERR_OK; + } + + 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))); @@ -1250,7 +1261,17 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, * Which basically means that if you have GET_ALL_CHANGES rights (~== RWDC) * then you can do EXOP_REPL_SECRETS */ - obj_dn = drs_ObjectIdentifier_to_dn(mem_ctx, b_state->sam_ctx_system, ncRoot); + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, + b_state->sam_ctx_system, + ncRoot, + &obj_dn, + NULL); + if (ret != LDB_SUCCESS) { + DBG_ERR("RevealSecretRequest for for invalid DN %s\n", + drs_ObjectIdentifier_to_string(mem_ctx, ncRoot)); + goto failed; + } + if (!ldb_dn_validate(obj_dn)) goto failed; if (has_get_all_changes) { @@ -1362,8 +1383,9 @@ static WERROR getncchanges_change_master(struct drsuapi_bind_state *b_state, - verify that we are the current master */ - req_dn = drs_ObjectIdentifier_to_dn(mem_ctx, ldb, req10->naming_context); - if (!ldb_dn_validate(req_dn)) { + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, ldb, req10->naming_context, + &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))); @@ -1389,8 +1411,16 @@ static WERROR getncchanges_change_master(struct drsuapi_bind_state *b_state, /* change the current master */ msg = ldb_msg_new(ldb); W_ERROR_HAVE_NO_MEMORY(msg); - msg->dn = drs_ObjectIdentifier_to_dn(msg, ldb, req10->naming_context); - W_ERROR_HAVE_NO_MEMORY(msg->dn); + ret = drs_ObjectIdentifier_to_dn_and_nc_root(msg, ldb, req10->naming_context, + &msg->dn, NULL); + 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), + ldb_strerror(ret)); + ctr6->extended_ret = DRSUAPI_EXOP_ERR_MISMATCH; + return WERR_OK; + } /* TODO: make sure ntds_dn is a valid nTDSDSA object */ ret = dsdb_find_dn_by_guid(ldb, msg, &req10->destination_dsa_guid, 0, &ntds_dn); @@ -2864,7 +2894,21 @@ allowed: /* see if a previous replication has been abandoned */ if (getnc_state) { - struct ldb_dn *new_dn = drs_ObjectIdentifier_to_dn(getnc_state, sam_ctx, ncRoot); + struct ldb_dn *new_dn; + ret = drs_ObjectIdentifier_to_dn_and_nc_root(getnc_state, + sam_ctx, + ncRoot, + &new_dn, + NULL); + if (ret != LDB_SUCCESS) { + /* + * This can't fail as we have done this above + * implicitly but not got the DN out + */ + DBG_ERR("Bad DN '%s'\n", + drs_ObjectIdentifier_to_string(mem_ctx, ncRoot)); + return WERR_DS_DRA_INVALID_PARAMETER; + } if (ldb_dn_compare(new_dn, getnc_state->ncRoot_dn) != 0) { DEBUG(0,(__location__ ": DsGetNCChanges 2nd replication on different DN %s %s (last_dn %s)\n", ldb_dn_get_linearized(new_dn), @@ -2899,9 +2943,13 @@ allowed: uint32_t nc_instanceType; struct ldb_dn *ncRoot_dn; - ncRoot_dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, ncRoot); - if (ncRoot_dn == NULL) { - return WERR_NOT_ENOUGH_MEMORY; + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, + sam_ctx, + ncRoot, + &ncRoot_dn, + NULL); + if (ret != LDB_SUCCESS) { + return WERR_DS_DRA_BAD_DN; } ret = dsdb_search_dn(sam_ctx, mem_ctx, &res, diff --git a/source4/rpc_server/drsuapi/updaterefs.c b/source4/rpc_server/drsuapi/updaterefs.c index 7450ddd3a31..5d2bc6e949c 100644 --- a/source4/rpc_server/drsuapi/updaterefs.c +++ b/source4/rpc_server/drsuapi/updaterefs.c @@ -195,7 +195,6 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, { WERROR werr; int ret; - struct ldb_dn *dn; struct ldb_dn *dn_normalised; struct ldb_dn *nc_root; struct ldb_context *sam_ctx = b_state->sam_ctx_system?b_state->sam_ctx_system:b_state->sam_ctx; @@ -226,14 +225,11 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, return WERR_DS_DRA_INVALID_PARAMETER; } - dn = drs_ObjectIdentifier_to_dn(mem_ctx, sam_ctx, req->naming_context); - W_ERROR_HAVE_NO_MEMORY(dn); - ret = dsdb_normalise_dn_and_find_nc_root(sam_ctx, dn, - dn, - &dn_normalised, - &nc_root); + ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, sam_ctx, req->naming_context, + &dn_normalised, &nc_root); if (ret != LDB_SUCCESS) { - DEBUG(2, ("Didn't find a nc for %s\n", ldb_dn_get_linearized(dn))); + DBG_WARNING("Didn't find a nc for %s\n", + ldb_dn_get_linearized(dn_normalised)); return WERR_DS_DRA_BAD_NC; } if (ldb_dn_compare(dn_normalised, nc_root) != 0) { @@ -249,7 +245,10 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, * This means that in the usual case, it will never open it and never * bother to refresh the dreplsrv. */ - werr = uref_check_dest(sam_ctx, mem_ctx, dn, &req->dest_dsa_guid, + werr = uref_check_dest(sam_ctx, + mem_ctx, + dn_normalised, + &req->dest_dsa_guid, req->options); if (W_ERROR_EQUAL(werr, WERR_DS_DRA_REF_ALREADY_EXISTS) || W_ERROR_EQUAL(werr, WERR_DS_DRA_REF_NOT_FOUND)) { @@ -266,7 +265,11 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, } if (req->options & DRSUAPI_DRS_DEL_REF) { - werr = uref_del_dest(sam_ctx, mem_ctx, dn, &req->dest_dsa_guid, req->options); + werr = uref_del_dest(sam_ctx, + mem_ctx, + dn_normalised, + &req->dest_dsa_guid, + req->options); if (!W_ERROR_IS_OK(werr)) { DEBUG(0,("Failed to delete repsTo for %s: %s\n", GUID_string(mem_ctx, &req->dest_dsa_guid), @@ -287,7 +290,11 @@ WERROR drsuapi_UpdateRefs(struct imessaging_context *msg_ctx, dest.source_dsa_obj_guid = req->dest_dsa_guid; dest.replica_flags = req->options; - werr = uref_add_dest(sam_ctx, mem_ctx, dn, &dest, req->options); + werr = uref_add_dest(sam_ctx, + mem_ctx, + dn_normalised, + &dest, + req->options); if (!W_ERROR_IS_OK(werr)) { DEBUG(0,("Failed to add repsTo for %s: %s\n", GUID_string(mem_ctx, &dest.source_dsa_obj_guid), -- cgit v1.2.1