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 <abartlet@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>

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=%s>",
+					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=%s>",
+					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;
 	}