On Thu, Sep 04, 2014 at 08:11:29PM +0200, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2014-09-04 19:07:27 +0200]:
> > * Natanael Copa <ncopa@alpinelinux.org> [2014-09-04 09:50:39 +0200]:
> > > Rich pointed out to me on IRC that this will not write terminating '\0'
> > > when domain name length is zero. I'll send new patch.
> > > 
> > 
> > here is a patch
> 
> another try

> >From 1a068a048b64999f97add01ce8f5013a83b0e916 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@port70.net>
> Date: Thu, 4 Sep 2014 18:29:16 +0200
> Subject: [PATCH] fix dn_expand empty name handling and offsets to 0
> 
> Empty name was rejected in dn_expand since commit
> 56b57f37a46dab432247bf29d96fcb11fbd02a6d
> which is a regression as reported by Natanael Copa.
> 
> Furthermore if an offset pointer in a compressed name
> pointed to a terminating 0 byte (instead of a label)
> the returned name was not null terminated.
> ---
>  src/network/dn_expand.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/network/dn_expand.c b/src/network/dn_expand.c
> index 849df19..d1ebebf 100644
> --- a/src/network/dn_expand.c
> +++ b/src/network/dn_expand.c
> @@ -5,8 +5,8 @@ int __dn_expand(const unsigned char *base, const unsigned char *end, const unsig
>  {
>  	const unsigned char *p = src;
>  	char *dend = dest + (space > 254 ? 254 : space);
> -	int len = -1, i, j;
> -	if (p==end || !*p) return -1;
> +	int len = -1, i, j, first = 1;
> +	if (p==end || dest==dend) return -1;

Note that this does nothing to handle negative space, whereas ncopa's
patch did fix that case too. I'm not clear on whether negative space
should be an error or UB, but error is probably nicer. In that case,
it needs to be caught _before_ the invalid pointer arithmetic above.

>  	/* detect reference loop using an iteration counter */
>  	for (i=0; i < end-base; i+=2) {
>  		if (*p & 0xc0) {
> @@ -16,11 +16,13 @@ int __dn_expand(const unsigned char *base, const unsigned char *end, const unsig
>  			if (j >= end-base) return -1;
>  			p = base+j;
>  		} else if (*p) {
> -			j = *p+1;
> -			if (j>=end-p || j>dend-dest) return -1;
> -			while (--j) *dest++ = *++p;
> -			*dest++ = *++p ? '.' : 0;
> +			if (!first) *dest++ = '.';
> +			first = 0;
> +			j = *p++;
> +			if (j >= end-p || j >= dend-dest) return -1;
> +			while (j--) *dest++ = *p++;
>  		} else {
> +			*dest = 0;
>  			if (len < 0) len = p+1-src;
>  			return len;
>  		}

This seems like a somewhat reasonable approach. But I'd want to check
over it several times to make sure there are no further logic errors
in the length...

I tend to like the version ncopa and I worked out somewhat better just
because it makes minimal logic changes. But yours is probably a
cleaner "final destination" so if I'm convinced it's correct and safe
we could probably go with it. ncopa's version is here:

http://sprunge.us/DOCB

and attached in case the paste rots.

Alternatively, we we discussed on IRC, using a pointer to terminate
_may_ just be an error, in which case we should just report it as
such. Semantically it seems to be a zero-length component at the end
(corresponding to "example.com.." rather than "example.com.", in the
notation where final dots are not elided). Understanding whether it's
legal or not probably requires some language-lawyering with RFC 1035,
but it might be worthwhile if we could just reject a form that's
obviously malicious and non-useful (it's always larger than the
correct way of representing the name).

Rich