From 28e2a518ff3233f49f1b61210754d044c670087b Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 15 Aug 2018 10:44:03 +1200 Subject: [PATCH] dns_server: Avoid ldb_dn_add_child_fmt() on untrusted input By using the new ldb_dn_add_child_val() we ensure that the user-controlled values are not parsed as DN seperators. Additionally, the casefold DN is obtained before the search to trigger a full parse of the DN before being handled to the LDB search. This is not normally required but is done here due to the nature of the untrusted input. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13466 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall --- source4/dns_server/dlz_bind9.c | 156 ++++++++++++++++++++++-- source4/dns_server/dnsserver_common.c | 13 +- source4/rpc_server/dnsserver/dnsdb.c | 11 +- source4/rpc_server/dnsserver/dnsutils.c | 14 ++- 4 files changed, 172 insertions(+), 22 deletions(-) diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c index 5f9a71dd741..054f13e6103 100644 --- a/source4/dns_server/dlz_bind9.c +++ b/source4/dns_server/dlz_bind9.c @@ -776,8 +776,11 @@ static isc_result_t b9_find_zone_dn(struct dlz_bind9_data *state, const char *zo int i; for (i=0; zone_prefixes[i]; i++) { + const char *casefold; struct ldb_dn *dn; struct ldb_result *res; + struct ldb_val zone_name_val + = data_blob_string_const(zone_name); dn = ldb_dn_copy(tmp_ctx, ldb_get_default_basedn(state->samdb)); if (dn == NULL) { @@ -785,11 +788,40 @@ static isc_result_t b9_find_zone_dn(struct dlz_bind9_data *state, const char *zo return ISC_R_NOMEMORY; } - if (!ldb_dn_add_child_fmt(dn, "DC=%s,%s", zone_name, zone_prefixes[i])) { + /* + * This dance ensures that it is not possible to put + * (eg) an extra DC=x, into the DNS name being + * queried + */ + + if (!ldb_dn_add_child_fmt(dn, + "DC=X,%s", + zone_prefixes[i])) { + talloc_free(tmp_ctx); + return ISC_R_NOMEMORY; + } + + ret = ldb_dn_set_component(dn, + 0, + "DC", + zone_name_val); + if (ret != LDB_SUCCESS) { talloc_free(tmp_ctx); return ISC_R_NOMEMORY; } + /* + * Check if this is a plausibly valid DN early + * (time spent here will be saved during the + * search due to an internal cache) + */ + casefold = ldb_dn_get_casefold(dn); + + if (casefold == NULL) { + talloc_free(tmp_ctx); + return ISC_R_NOTFOUND; + } + ret = ldb_search(state->samdb, tmp_ctx, &res, dn, LDB_SCOPE_BASE, attrs, "objectClass=dnsZone"); if (ret == LDB_SUCCESS) { if (zone_dn != NULL) { @@ -820,19 +852,42 @@ static isc_result_t b9_find_name_dn(struct dlz_bind9_data *state, const char *na isc_result_t result; result = b9_find_zone_dn(state, p, mem_ctx, dn); if (result == ISC_R_SUCCESS) { + const char *casefold; + /* we found a zone, now extend the DN to get * the full DN */ bool ret; if (p == name) { ret = ldb_dn_add_child_fmt(*dn, "DC=@"); + if (ret == false) { + talloc_free(*dn); + return ISC_R_NOMEMORY; + } } else { - ret = ldb_dn_add_child_fmt(*dn, "DC=%.*s", (int)(p-name)-1, name); + struct ldb_val name_val + = data_blob_const(name, + (int)(p-name)-1); + + if (!ldb_dn_add_child_val(*dn, + "DC", + name_val)) { + talloc_free(*dn); + return ISC_R_NOMEMORY; + } } - if (!ret) { - talloc_free(*dn); - return ISC_R_NOMEMORY; + + /* + * Check if this is a plausibly valid DN early + * (time spent here will be saved during the + * search due to an internal cache) + */ + casefold = ldb_dn_get_casefold(*dn); + + if (casefold == NULL) { + return ISC_R_NOTFOUND; } + return ISC_R_SUCCESS; } p = strchr(p, '.'); @@ -874,19 +929,63 @@ static isc_result_t dlz_lookup_types(struct dlz_bind9_data *state, WERROR werr = WERR_DNS_ERROR_NAME_DOES_NOT_EXIST; struct dnsp_DnssrvRpcRecord *records = NULL; uint16_t num_records = 0, i; + struct ldb_val zone_name_val + = data_blob_string_const(zone); + struct ldb_val name_val + = data_blob_string_const(name); for (i=0; zone_prefixes[i]; i++) { + int ret; + const char *casefold; dn = ldb_dn_copy(tmp_ctx, ldb_get_default_basedn(state->samdb)); if (dn == NULL) { talloc_free(tmp_ctx); return ISC_R_NOMEMORY; } - if (!ldb_dn_add_child_fmt(dn, "DC=%s,DC=%s,%s", name, zone, zone_prefixes[i])) { + /* + * This dance ensures that it is not possible to put + * (eg) an extra DC=x, into the DNS name being + * queried + */ + + if (!ldb_dn_add_child_fmt(dn, + "DC=X,DC=X,%s", + zone_prefixes[i])) { + talloc_free(tmp_ctx); + return ISC_R_NOMEMORY; + } + + ret = ldb_dn_set_component(dn, + 1, + "DC", + zone_name_val); + if (ret != LDB_SUCCESS) { talloc_free(tmp_ctx); return ISC_R_NOMEMORY; } + ret = ldb_dn_set_component(dn, + 0, + "DC", + name_val); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ISC_R_NOMEMORY; + } + + /* + * Check if this is a plausibly valid DN early + * (time spent here will be saved during the + * search due to an internal cache) + */ + casefold = ldb_dn_get_casefold(dn); + + if (casefold == NULL) { + talloc_free(tmp_ctx); + return ISC_R_NOTFOUND; + } + werr = dns_common_wildcard_lookup(state->samdb, tmp_ctx, dn, &records, &num_records); if (W_ERROR_IS_OK(werr)) { @@ -953,19 +1052,50 @@ _PUBLIC_ isc_result_t dlz_allnodes(const char *zone, void *dbdata, struct ldb_dn *dn; struct ldb_result *res; TALLOC_CTX *tmp_ctx = talloc_new(state); + struct ldb_val zone_name_val = data_blob_string_const(zone); for (i=0; zone_prefixes[i]; i++) { + const char *casefold; + dn = ldb_dn_copy(tmp_ctx, ldb_get_default_basedn(state->samdb)); if (dn == NULL) { talloc_free(tmp_ctx); return ISC_R_NOMEMORY; } - if (!ldb_dn_add_child_fmt(dn, "DC=%s,%s", zone, zone_prefixes[i])) { + /* + * This dance ensures that it is not possible to put + * (eg) an extra DC=x, into the DNS name being + * queried + */ + + if (!ldb_dn_add_child_fmt(dn, + "DC=X,%s", + zone_prefixes[i])) { + talloc_free(tmp_ctx); + return ISC_R_NOMEMORY; + } + + ret = ldb_dn_set_component(dn, + 0, + "DC", + zone_name_val); + if (ret != LDB_SUCCESS) { talloc_free(tmp_ctx); return ISC_R_NOMEMORY; } + /* + * Check if this is a plausibly valid DN early + * (time spent here will be saved during the + * search due to an internal cache) + */ + casefold = ldb_dn_get_casefold(dn); + + if (casefold == NULL) { + return ISC_R_NOTFOUND; + } + ret = ldb_search(state->samdb, tmp_ctx, &res, dn, LDB_SCOPE_SUBTREE, attrs, "objectClass=dnsNode"); if (ret == LDB_SUCCESS) { @@ -1118,8 +1248,18 @@ static bool b9_has_soa(struct dlz_bind9_data *state, struct ldb_dn *dn, const ch WERROR werr; struct dnsp_DnssrvRpcRecord *records = NULL; uint16_t num_records = 0, i; + struct ldb_val zone_name_val + = data_blob_string_const(zone); + + /* + * This dance ensures that it is not possible to put + * (eg) an extra DC=x, into the DNS name being + * queried + */ - if (!ldb_dn_add_child_fmt(dn, "DC=@,DC=%s", zone)) { + if (!ldb_dn_add_child_val(dn, + "DC", + zone_name_val)) { talloc_free(tmp_ctx); return false; } diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 1204fc6bbcf..1a032b4aa9f 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -1059,7 +1059,6 @@ WERROR dns_common_name2dn(struct ldb_context *samdb, struct ldb_val host_part; WERROR werr; bool ok; - int ret; const char *casefold = NULL; if (name == NULL) { @@ -1116,17 +1115,11 @@ WERROR dns_common_name2dn(struct ldb_context *samdb, return WERR_NOT_ENOUGH_MEMORY; } - ok = ldb_dn_add_child_fmt(dn, "DC=X"); - - if (ok == false) { - TALLOC_FREE(dn); - return WERR_NOT_ENOUGH_MEMORY; - } - host_part = data_blob_const(name, host_part_len); - ret = ldb_dn_set_component(dn, 0, "DC", host_part); - if (ret != LDB_SUCCESS) { + ok = ldb_dn_add_child_val(dn, "DC", host_part); + + if (ok == false) { TALLOC_FREE(dn); return WERR_NOT_ENOUGH_MEMORY; } diff --git a/source4/rpc_server/dnsserver/dnsdb.c b/source4/rpc_server/dnsserver/dnsdb.c index 899c7ecedb6..81af5f1ceef 100644 --- a/source4/rpc_server/dnsserver/dnsdb.c +++ b/source4/rpc_server/dnsserver/dnsdb.c @@ -383,6 +383,7 @@ WERROR dnsserver_db_add_empty_node(TALLOC_CTX *mem_ctx, struct ldb_result *res; struct ldb_dn *dn; char *encoded_name = ldb_binary_encode_string(mem_ctx, name); + struct ldb_val name_val = data_blob_string_const(name); int ret; ret = ldb_search(samdb, mem_ctx, &res, z->zone_dn, LDB_SCOPE_BASE, attrs, @@ -400,7 +401,7 @@ WERROR dnsserver_db_add_empty_node(TALLOC_CTX *mem_ctx, dn = ldb_dn_copy(mem_ctx, z->zone_dn); W_ERROR_HAVE_NO_MEMORY(dn); - if (!ldb_dn_add_child_fmt(dn, "DC=%s", name)) { + if (!ldb_dn_add_child_val(dn, "DC", name_val)) { return WERR_NOT_ENOUGH_MEMORY; } @@ -1018,6 +1019,7 @@ WERROR dnsserver_db_create_zone(struct ldb_context *samdb, struct dnsp_DnssrvRpcRecord *dns_rec; struct dnsp_soa soa; char *tmpstr, *server_fqdn, *soa_email; + struct ldb_val name_val = data_blob_string_const(zone->name); /* We only support primary zones for now */ if (zone->zoneinfo->dwZoneType != DNS_ZONE_TYPE_PRIMARY) { @@ -1043,7 +1045,12 @@ WERROR dnsserver_db_create_zone(struct ldb_context *samdb, dn = ldb_dn_copy(tmp_ctx, p->partition_dn); W_ERROR_HAVE_NO_MEMORY_AND_FREE(dn, tmp_ctx); - if(!ldb_dn_add_child_fmt(dn, "DC=%s,CN=MicrosoftDNS", zone->name)) { + if (!ldb_dn_add_child_fmt(dn, "CN=MicrosoftDNS")) { + talloc_free(tmp_ctx); + return WERR_NOT_ENOUGH_MEMORY; + } + + if (!ldb_dn_add_child_val(dn, "DC", name_val)) { talloc_free(tmp_ctx); return WERR_NOT_ENOUGH_MEMORY; } diff --git a/source4/rpc_server/dnsserver/dnsutils.c b/source4/rpc_server/dnsserver/dnsutils.c index a1c749074af..b3d8949f8ab 100644 --- a/source4/rpc_server/dnsserver/dnsutils.c +++ b/source4/rpc_server/dnsserver/dnsutils.c @@ -371,6 +371,8 @@ struct ldb_dn *dnsserver_name_to_dn(TALLOC_CTX *mem_ctx, struct dnsserver_zone * { struct ldb_dn *dn; bool ret; + struct ldb_val name_val = + data_blob_string_const(name); dn = ldb_dn_copy(mem_ctx, z->zone_dn); if (dn == NULL) { @@ -378,9 +380,17 @@ struct ldb_dn *dnsserver_name_to_dn(TALLOC_CTX *mem_ctx, struct dnsserver_zone * } if (strcasecmp(name, z->name) == 0) { ret = ldb_dn_add_child_fmt(dn, "DC=@"); - } else { - ret = ldb_dn_add_child_fmt(dn, "DC=%s", name); + if (!ret) { + talloc_free(dn); + return NULL; + } + return dn; } + + ret = ldb_dn_add_child_val(dn, + "DC", + name_val); + if (!ret) { talloc_free(dn); return NULL; -- 2.34.1