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 <abartlet@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
(cherry picked from commit aee2039e63ceeb5e69a0461fb77e0f18278e4dc4)

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),