s4/rpc_server/dnsserver: Allow parsing of dnsProperty to fail gracefully

On (eg) the

DC=_msdcs.X.Y,CN=MicrosoftDNS,DC=ForestDnsZones,DC=X,DC=Y

record, in domains that have had a Microsoft Windows DC an attribute:

dNSProperty:: AAAAAAAAAAAAAAAAAQAAAJIAAAAAAAAA

000000 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00  >................<
000010 92 00 00 00 00 00 00 00                          >........<
000018

We, until samba 4.12, would parse this as:

pull returned Success
    dnsp_DnsProperty: struct dnsp_DnsProperty
        wDataLength              : 0x00000000 (0)
        namelength               : 0x00000000 (0)
        flag                     : 0x00000000 (0)
        version                  : 0x00000001 (1)
        id                       : DSPROPERTY_ZONE_NS_SERVERS_DA (146)
        data                     : union dnsPropertyData(case 0)
        name                     : 0x00000000 (0)
dump OK

However, the wDataLength is 0.  There is not anything in
[MS-DNSP] 2.3.2.1 dnsProperty to describe any special behaviour
for when the id suggests that there is a value, but wDataLength is 0.

https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dnsp/445c7843-e4a1-4222-8c0f-630c230a4c80

We now fail to parse it, because we expect an entry with id DSPROPERTY_ZONE_NS_SERVERS_DA
to therefore have a valid DNS_ADDR_ARRAY (section 2.2.3.2.3).

As context we changed it in our commit fee5c6a4247aeac71318186bbff7708d25de5912
because of bug https://bugzilla.samba.org/show_bug.cgi?id=14206
which was due to the artificial environment of the fuzzer.

Microsoft advises that Windows also fails to parse this, but
instead of failing the operation, the value is ignored.

Reported by Alex MacCuish.  Many thanks for your assistance in
tracking down the issue.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14310

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
Autobuild-Date(master): Fri May 15 07:29:17 UTC 2020 on sn-devel-184

diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns
index 8ba1f4623ac163285094477dbb4a8c4fa8ae488c..fee2f2ae32244195c2450d3d8bb3a773c90c92c0 100644 (file)
--- a/selftest/knownfail.d/dns
+++ b/selftest/knownfail.d/dns
@@ -81,14 +81,9 @@ samba.tests.dns.__main__.TestSimpleQueries.test_one_SOA_query\(rodc:local\)
 ^samba.tests.dns.__main__.TestComplexQueries.test_cname_any_query\(vampire_dc:local\)
 ^samba.tests.dns.__main__.TestComplexQueries.test_cname_any_query\(rodc:local\)
 
-# Tests for the dnsProperty parse issue
-^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_dnsProperty_zero_length\(fl2003dc:local\)
-^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_other_dnsProperty_zero_length\(fl2003dc:local\)
-^samba.tests.dns.__main__.TestZones.test_update_while_dnsProperty_zero_length\(fl2003dc:local\)
+# Tests for the dnsProperty parse issue do not pass here, but do against fl2003dc
 ^samba.tests.dns.__main__.TestZones.test_enum_zones_while_dnsProperty_zero_length\(rodc:local\)
 ^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_dnsProperty_zero_length\(rodc:local\)
 ^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_other_dnsProperty_zero_length\(rodc:local\)
 ^samba.tests.dns.__main__.TestZones.test_update_while_dnsProperty_zero_length\(rodc:local\)
-^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_dnsProperty_zero_length\(vampire_dc:local\)
-^samba.tests.dns.__main__.TestZones.test_rpc_zone_update_while_other_dnsProperty_zero_length\(vampire_dc:local\)
 ^samba.tests.dns.__main__.TestZones.test_update_while_dnsProperty_zero_length\(vampire_dc:local\)
\ No newline at end of file

diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c
index 420d141202e8f476a75e8f397d0b2f4c12581f0f..8635d075be8060c21a789c60a5020e4ae740b69b 100644 (file)
--- a/source4/dns_server/dnsserver_common.c
+++ b/source4/dns_server/dnsserver_common.c
@@ -901,7 +901,14 @@ WERROR dns_get_zone_properties(struct ldb_context *samdb,
                    prop,
                    (ndr_pull_flags_fn_t)ndr_pull_dnsp_DnsProperty);
                if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
-                       return DNS_ERR(SERVER_FAILURE);
+                       /*
+                        * If we can't pull it, then there is no valid
+                        * data to load into the zone, so ignore this
+                        * as Micosoft does.  Windows can load an
+                        * invalid property with a zero length into
+                        * the dnsProperty attribute.
+                        */
+                       continue;
                }
 
                valid_property =

diff --git a/source4/rpc_server/dnsserver/dnsdb.c b/source4/rpc_server/dnsserver/dnsdb.c
index 9ee50d8ff3950168032a3c23062c1cd25a423e74..798c869efe5c7eff8bcc38a7ecef3552125b331b 100644 (file)
--- a/source4/rpc_server/dnsserver/dnsdb.c
+++ b/source4/rpc_server/dnsserver/dnsdb.c
@@ -156,7 +156,24 @@ struct dnsserver_zone *dnsserver_db_enumerate_zones(TALLOC_CTX *mem_ctx,
                                        (ndr_pull_flags_fn_t)
                                                ndr_pull_dnsp_DnsProperty);
                                if (!NDR_ERR_CODE_IS_SUCCESS(err)){
-                                       goto failed;
+                                       /*
+                                        * Per Microsoft we must
+                                        * ignore invalid data here
+                                        * and continue as a Windows
+                                        * server can put in a
+                                        * structure with an invalid
+                                        * length.
+                                        *
+                                        * We can safely fill in an
+                                        * extra empty property here
+                                        * because
+                                        * dns_zoneinfo_load_zone_property()
+                                        * just ignores
+                                        * DSPROPERTY_ZONE_EMPTY
+                                        */
+                                       ZERO_STRUCT(props[j]);
+                                       props[j].id = DSPROPERTY_ZONE_EMPTY;
+                                       continue;
                                }
                        }
                        z->tmp_props = props;
@@ -812,12 +829,53 @@ WERROR dnsserver_db_do_reset_dword(struct ldb_context *samdb,
                        prop,
                        (ndr_pull_flags_fn_t)ndr_pull_dnsp_DnsProperty);
                if (!NDR_ERR_CODE_IS_SUCCESS(err)){
-                       DBG_ERR("dnsserver: couldn't PULL dns property id "
-                               "%d in zone %s\n",
-                               prop->id,
-                               ldb_dn_get_linearized(z->zone_dn));
-                       TALLOC_FREE(tmp_ctx);
-                       return WERR_INTERNAL_DB_ERROR;
+                       /*
+                        * If we can't pull it then try again parsing
+                        * it again.  A Windows server in the domain
+                        * will permit the addition of an invalidly
+                        * formed property with a 0 length and cause a
+                        * failure here
+                        */
+                       struct dnsp_DnsProperty_short
+                               *short_property
+                               = talloc_zero(element,
+                                             struct dnsp_DnsProperty_short);
+                       if (short_property == NULL) {
+                               TALLOC_FREE(tmp_ctx);
+                               return WERR_NOT_ENOUGH_MEMORY;
+                       }
+                       err = ndr_pull_struct_blob_all(
+                               &(element->values[i]),
+                               tmp_ctx,
+                               short_property,
+                               (ndr_pull_flags_fn_t)ndr_pull_dnsp_DnsProperty_short);
+                       if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
+                               /*
+                                * Unknown invalid data should be
+                                * ignored and logged to match Windows
+                                * behaviour
+                                */
+                               DBG_NOTICE("dnsserver: couldn't PULL "
+                                          "dnsProperty value#%d in "
+                                          "zone %s while trying to "
+                                          "reset id %d\n",
+                                          i,
+                                          ldb_dn_get_linearized(z->zone_dn),
+                                          prop_id);
+                               continue;
+                       }
+
+                       /*
+                        * Initialise the parts of the property not
+                        * overwritten by value() in the IDL for
+                        * re-push
+                        */
+                       *prop = (struct dnsp_DnsProperty){
+                               .namelength = short_property->namelength,
+                               .id = short_property->id,
+                               .name = short_property->name
+                               /* .data will be filled in below */
+                       };
                }
 
                if (prop->id == prop_id) {