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 <abartlet@samba.org>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>

diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c
index 5f9a71dd7412c0034fc6317369de1776d90c0ce2..054f13e6103becb315e6decc977827690d19931d 100644 (file)
--- 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 1204fc6bbcf1a2ce95a183253a088c6453faa107..1a032b4aa9f980920e066dd43942211e0dc03e0c 100644 (file)
--- 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 899c7ecedb663e0a263c6bc50d136fcd347cf864..81af5f1ceefcd62abf90645345b8048ca7ca7133 100644 (file)
--- 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 a1c749074af8555715b2b9e71c26136aa31b4d21..b3d8949f8abd079213271885348b75eeb1e43603 100644 (file)
--- 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;