[PATCH] Internal DNS, RPC server & samba-tool tests & fixes

Thu Dec 8 22:31:24 UTC 2016

On Fri, Dec 09, 2016 at 10:44:05AM +1300, Bob Campbell wrote:
> Hi,
> 
> Here are a bunch of patches to test "samba-tool dns", extend testing for
> DNS over RPC directly, and fix some problems found through the tests.
> 
> *samba-tool issues found:*
> 
> - There is unnecessary input validation, which means that DNS records
> marked with DNS_RANK_NONE can't be deleted, even though they can on the
> RPC server. Since we've now proved input validation on the RPC server,
> we don't need this validation, so we removed it where possible.
> 
> - Error catching is generic (it just catches runtime errors and gives
> the same message), so we made it more specific and changes some messages
> to allow for easier debugging.
> 
> *backend issues found:*
> 
> - There is no validation of DNS names over RPC on Samba, but there is on
> Windows. We've fixed this by adding a simple check whenever doing
> anything to a record except deleting it (as we don't want deletes to
> fail against existing entries, even if their DN is invalid). We also
> added this check on DNS, and when converting a string to an ldb_dn
> structure.
> 
> - ACLs on some zones, in particular default ones, are weird and
> inconsistent with Windows in places, but this is mostly because the
> default owners are different. We've marked these tests as knownfails,
> because we're not sure that there's actually a problem here.
> 
> - DNS records in the _msdcs zone wouldn't have the dNSProperty attribute
> (which may have been a root cause of entries with DNS_RANK_NONE).
> 
> These patches are applied at:
> http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/dnscmdtest
> 
> Please review.

Not commenting much on the python (sorry, I'm still learning), but
a few C comments follow inline.

Cheers,

Jeremy.

> From 020c77cc6bf279d969641e8af533107d7b94e3b7 Mon Sep 17 00:00:00 2001
> From: Bob Campbell <bobcampbell at catalyst.net.nz>
> Date: Tue, 6 Dec 2016 15:34:23 +1300
> Subject: [PATCH 07/12] dnsserver: add dns name checking
> 
> This may also prevent deletion of existing corrupted records through
> DNS, but should be resolvable through RPC, or at worst LDAP.
> 
> Signed-off-by: Bob Campbell <bobcampbell at catalyst.net.nz>
> Peer-programmed-with: Garming Sam <garming at catalyst.net.nz>
> Reviewed-by: Garming Sam <garming at catalyst.net.nz>
> ---
>  selftest/knownfail                       |   2 -
>  source4/dns_server/dnsserver_common.c    |  87 ++++++++++++++++++++++
>  source4/dns_server/dnsserver_common.h    |   4 +-
>  source4/rpc_server/dnsserver/dnsdata.c   | 122 ++++++++++++++++++++++++-------
>  source4/rpc_server/dnsserver/dnsdb.c     |  33 ++++++---
>  source4/rpc_server/dnsserver/dnsserver.h |   4 +-
>  source4/rpc_server/wscript_build         |   2 +-
>  7 files changed, 212 insertions(+), 42 deletions(-)
> 
> diff --git a/selftest/knownfail b/selftest/knownfail
> index 7141f43..97ec6ef 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -309,5 +309,3 @@
>  ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_add_duplicate_different_type.*
>  ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_rank_none.*
>  ^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_security_descriptor.*
> -^samba.tests.dcerpc.dnsserver.samba.tests.dcerpc.dnsserver.DnsserverTests.test_reject_invalid_commands
> -^samba.tests.samba_tool.dnscmd.samba.tests.samba_tool.dnscmd.DnsCmdTestCase.test_reject_invalid_commands
> diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c
> index db42bcb..e6534c3 100644
> --- a/source4/dns_server/dnsserver_common.c
> +++ b/source4/dns_server/dnsserver_common.c
> @@ -214,6 +214,87 @@ static int rec_cmp(const struct dnsp_DnssrvRpcRecord *r1,
>  	return r1->dwTimeStamp - r2->dwTimeStamp;
>  }
>  
> +/*
> + * Check for valid DNS names. These are names which are non-empty, do not
> + * start with a dot and do not have any empty segments.
> + */
> +WERROR dns_name_check(TALLOC_CTX *mem_ctx, int len, const char *name)

len here should be size_t, not int. It comes from a strlen() which
returns size_t.

> +{
> +	int i;

i should also be size_t.

> +	if (len == 0) {
> +		return WERR_DS_INVALID_DN_SYNTAX;
> +	}
> +
> +	for (i = 0; i < len - 1; i++) {
> +		if (name[i] == '.' && name[i+1] == '.') {
> +			return WERR_DS_INVALID_DN_SYNTAX;
> +		}
> +	}
> +
> +	if (len > 1 && name[0] == '.') {
> +		return WERR_DS_INVALID_DN_SYNTAX;
> +	}
> +
> +	return WERR_OK;
> +}
> +
> +static WERROR check_name_list(TALLOC_CTX *mem_ctx, uint16_t rec_count,
> +			      struct dnsp_DnssrvRpcRecord *records)
> +{
> +	WERROR werr;
> +	int i, len;

i should be uint16_t, as that's the type of rec_count.
len should be size_t as it comes from strlen().

> +	struct dnsp_DnssrvRpcRecord record;
> +
> +	werr = WERR_OK;
> +	for(i=0; i<rec_count; i++){
> +		record = records[i];
> +
> +		switch (record.wType) {
> +
> +		case DNS_TYPE_NS:
> +			len = strlen(record.data.ns);
> +			werr = dns_name_check(mem_ctx, len, record.data.ns);
> +			break;
> +		case DNS_TYPE_CNAME:
> +			len = strlen(record.data.cname);
> +			werr = dns_name_check(mem_ctx, len, record.data.cname);
> +			break;
> +		case DNS_TYPE_SOA:
> +			len = strlen(record.data.soa.mname);
> +			werr = dns_name_check(mem_ctx, len, record.data.soa.mname);
> +			if (!W_ERROR_IS_OK(werr)) {
> +				break;
> +			}
> +			len = strlen(record.data.soa.rname);
> +			werr = dns_name_check(mem_ctx, len, record.data.soa.rname);
> +			break;
> +		case DNS_TYPE_PTR:
> +			len = strlen(record.data.ptr);
> +			werr = dns_name_check(mem_ctx, len, record.data.ptr);
> +			break;
> +		case DNS_TYPE_MX:
> +			len = strlen(record.data.mx.nameTarget);
> +			werr = dns_name_check(mem_ctx, len, record.data.mx.nameTarget);
> +			break;
> +		case DNS_TYPE_SRV:
> +			len = strlen(record.data.srv.nameTarget);
> +			werr = dns_name_check(mem_ctx, len,
> +					      record.data.srv.nameTarget);
> +			break;
> +		// In the default case, the record doesn't have a DN, so it must be ok.

Use C style comments, not C++.

> +		default:
> +			break;
> +		}
> +
> +		if (!W_ERROR_IS_OK(werr)) {
> +			return werr;
> +		}
> +	}
> +
> +	return WERR_OK;
> +}
> +
>  WERROR dns_common_replace(struct ldb_context *samdb,
>  			  TALLOC_CTX *mem_ctx,
>  			  struct ldb_dn *dn,
> @@ -225,6 +306,7 @@ WERROR dns_common_replace(struct ldb_context *samdb,
>  	struct ldb_message_element *el;
>  	uint16_t i;
>  	int ret;
> +	WERROR werr;
>  	struct ldb_message *msg = NULL;
>  	bool was_tombstoned = false;
>  	bool become_tombstoned = false;
> @@ -234,6 +316,11 @@ WERROR dns_common_replace(struct ldb_context *samdb,
>  
>  	msg->dn = dn;
>  
> +	werr = check_name_list(mem_ctx, rec_count, records);
> +	if (!W_ERROR_IS_OK(werr)) {
> +		return werr;
> +	}
> +
>  	ret = ldb_msg_add_empty(msg, "dnsRecord", LDB_FLAG_MOD_REPLACE, &el);
>  	if (ret != LDB_SUCCESS) {
>  		return DNS_ERR(SERVER_FAILURE);
> diff --git a/source4/dns_server/dnsserver_common.h b/source4/dns_server/dnsserver_common.h
> index ad91f61..bea3019 100644
> --- a/source4/dns_server/dnsserver_common.h
> +++ b/source4/dns_server/dnsserver_common.h
> @@ -46,7 +46,9 @@ WERROR dns_common_lookup(struct ldb_context *samdb,
>  			 struct dnsp_DnssrvRpcRecord **records,
>  			 uint16_t *num_records,
>  			 bool *tombstoned);
> -
> +WERROR dns_name_check(TALLOC_CTX *mem_ctx,
> +		      int len,
> +		      const char *name);
>  WERROR dns_common_replace(struct ldb_context *samdb,
>  			  TALLOC_CTX *mem_ctx,
>  			  struct ldb_dn *dn,
> diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c
> index ccea0d7..c2ca56e 100644
> --- a/source4/rpc_server/dnsserver/dnsdata.c
> +++ b/source4/rpc_server/dnsserver/dnsdata.c
> @@ -21,6 +21,7 @@
>  
>  #include "includes.h"
>  #include "dnsserver.h"
> +#include "dns_server/dnsserver_common.h"
>  #include "lib/replace/system/network.h"
>  #include "librpc/gen_ndr/ndr_dnsp.h"
>  #include "librpc/gen_ndr/ndr_dnsserver.h"
> @@ -412,15 +413,17 @@ void dnsp_to_dns_copy(TALLOC_CTX *mem_ctx, struct dnsp_DnssrvRpcRecord *dnsp,
>  
>  }
>  
> -
> -struct dnsp_DnssrvRpcRecord *dns_to_dnsp_copy(TALLOC_CTX *mem_ctx, struct DNS_RPC_RECORD *dns)
> +WERROR dns_to_dnsp_convert(TALLOC_CTX *mem_ctx, struct DNS_RPC_RECORD *dns,
> +			   struct dnsp_DnssrvRpcRecord **out_dnsp, bool check_name)
>  {
> +	WERROR res;
>  	int i, len;
> -	struct dnsp_DnssrvRpcRecord *dnsp;
> +	const char *name;
> +	struct dnsp_DnssrvRpcRecord *dnsp = NULL;
>  
>  	dnsp = talloc_zero(mem_ctx, struct dnsp_DnssrvRpcRecord);
>  	if (dnsp == NULL) {
> -		return NULL;
> +		return WERR_NOT_ENOUGH_MEMORY;
>  	}
>  
>  	dnsp->wDataLength = dns->wDataLength;
> @@ -442,21 +445,41 @@ struct dnsp_DnssrvRpcRecord *dns_to_dnsp_copy(TALLOC_CTX *mem_ctx, struct DNS_RP
>  		break;
>  
>  	case DNS_TYPE_NS:
> +		name = dns->data.name.str;
>  		len = dns->data.name.len;

Where does dns->data.name.len come from ?
If it comes from a remote source then there
must be range checks around any arithmetic
(len - 1) etc.

> -		if (dns->data.name.str[len-1] == '.') {
> -			dnsp->data.ns = talloc_strndup(mem_ctx, dns->data.name.str, len-1);
> +
> +		if (check_name) {
> +			res = dns_name_check(mem_ctx, len, name);
> +			if (!W_ERROR_IS_OK(res)) {
> +				return res;
> +			}
> +		}
> +
> +		if (name[len-1] == '.') {
> +			dnsp->data.ns = talloc_strndup(mem_ctx, name, len-1);
>  		} else {
> -			dnsp->data.ns = talloc_strdup(mem_ctx, dns->data.name.str);
> +			dnsp->data.ns = talloc_strdup(mem_ctx, name);
>  		}