[PATCH] fix handling of zero length domain names in dn_expand

[PATCH] fix handling of zero length domain names in dn_expand

[Natanael Copa]

Copy a zero length string instead of returning error when trying to
expand a zero lentgh domain name (null terminator).

This fixes a regression introduced with 56b57f37a46dab432.
---
This should issue with kamailio handling NAPTR records for enum.

I have verified that this correcponds with how uclibc handles zero
length domain names.

I have also verified that none of the 2 places in musl that uses
__dn_expand should break. (I have read the code but not actually run
tested it)

 src/network/dn_expand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/network/dn_expand.c b/src/network/dn_expand.c
index 849df19..3264faf 100644
--- a/src/network/dn_expand.c
+++ b/src/network/dn_expand.c
@@ -6,7 +6,7 @@ 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;
+	if (p==end) return -1;
 	/* detect reference loop using an iteration counter */
 	for (i=0; i < end-base; i+=2) {
 		if (*p & 0xc0) {
-- 
2.0.4

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

* Natanael Copa <ncopa@alpinelinux.org> [2014-09-04 09:50:39 +0200]:
> On Wed, 13 Aug 2014 10:21:41 +0200
> Natanael Copa <ncopa@alpinelinux.org> wrote:
> 
> > Copy a zero length string instead of returning error when trying to
> > expand a zero lentgh domain name (null terminator).
> > 
> 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

[-- Attachment #2: 0001-fix-empty-name-handling-in-dn_expand.patch --]
[-- Type: text/x-diff, Size: 1753 bytes --]

From 49b96b160fdde9218bcaffa196ca9e6d5a233094 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 empty name handling in dn_expand

Empty name was rejected in dn_expand since commit
56b57f37a46dab432247bf29d96fcb11fbd02a6d
which is a regression as reported by Natanael Copa.

But it turns out only an "uncompressed" empty name was rejected,
if an offset pointer is used to represent an empty name then
dn_expand failed to null terminate the returned name.

this fix makes dn_expand accept and return empty names correctly.
---
 src/network/dn_expand.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/network/dn_expand.c b/src/network/dn_expand.c
index 849df19..4fa6af8 100644
--- a/src/network/dn_expand.c
+++ b/src/network/dn_expand.c
@@ -6,7 +6,7 @@ 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;
+	if (p==end) return -1;
 	/* 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;
+			j = *p++;
+			if (j >= end-p || j >= dend-dest) return -1;
+			while (j--) *dest++ = *p++;
+			if (*p) *dest++ = '.';
 		} else {
+			if (dest == dend) return -1;
+			*dest = 0;
 			if (len < 0) len = p+1-src;
 			return len;
 		}
-- 
1.7.10.4

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

* 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

[-- Attachment #2: 0001-fix-dn_expand-empty-name-handling-and-offsets-to-0.patch --]
[-- Type: text/x-diff, Size: 1708 bytes --]

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;
 	/* 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;
 		}
-- 
1.7.10.4

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 3527 bytes --]

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

[-- Attachment #2: ncopa_dn_expand.diff --]
[-- Type: text/plain, Size: 2302 bytes --]

From 25bf2bd6a61f72a1722f9a86d3f4c71a05e4fabe Mon Sep 17 00:00:00 2001
From: Natanael Copa <ncopa@alpinelinux.org>
Date: Thu, 4 Sep 2014 09:11:34 +0200
Subject: [PATCH 1/2] fix handling of zero length domain names in dn_expand

Copy a zero length string instead of returning error when trying to
expand a zero length domain name (null terminator).

This fixes a regression introduced with 56b57f37a46dab432.
---
 src/network/dn_expand.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/network/dn_expand.c b/src/network/dn_expand.c
index 849df19..eb0af1a 100644
--- a/src/network/dn_expand.c
+++ b/src/network/dn_expand.c
@@ -4,9 +4,14 @@
 int __dn_expand(const unsigned char *base, const unsigned char *end, const unsigned char *src, char *dest, int space)
 {
 	const unsigned char *p = src;
-	char *dend = dest + (space > 254 ? 254 : space);
+	char *dend;
 	int len = -1, i, j;
-	if (p==end || !*p) return -1;
+	if (p==end || size <= 0) return -1;
+	dend = dest + (space > 254 ? 254 : space);
+	if (!*p) {
+		*dest = 0;
+		return 1;
+	}
 	/* detect reference loop using an iteration counter */
 	for (i=0; i < end-base; i+=2) {
 		if (*p & 0xc0) {
-- 
2.1.0


From dc71eba3dc203b5765193569f6c361ff9c9ee1b1 Mon Sep 17 00:00:00 2001
From: Natanael Copa <ncopa@alpinelinux.org>
Date: Thu, 4 Sep 2014 09:14:20 +0200
Subject: [PATCH 2/2] fix bug in dn_expand when a dns packet terminates with a
 pointer

Make sure that output buffer is always terminated with '\0', even if
the dns packet terminates the name with a pointer.
---
 src/network/dn_expand.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/network/dn_expand.c b/src/network/dn_expand.c
index eb0af1a..9e36d73 100644
--- a/src/network/dn_expand.c
+++ b/src/network/dn_expand.c
@@ -21,12 +21,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;
+			j = 1 + *p++;
+			if (j>end-p || j>dend-dest) return -1;
+			while (--j) *dest++ = *p++;
+			*dest++ = '.';
 		} else {
 			if (len < 0) len = p+1-src;
+			dest[-1] = 0;
 			return len;
 		}
 	}
-- 
2.1.0

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2014-09-04 16:20:53 -0400]:
> >  	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

i assumed it to be ub

> 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,

the rfc is not very clear but i think this case should work

a name is a sequence of labels terminated by a 0 length label

a compressed name is a leading sequence of labels terminated
by a pointer that can be expanded to a name

if 'sequence of labels' can be empty and the conversion to
dotted string notation is done after concatenating the
leading and trailing sequence then there is no '..' issue

> -	char *dend = dest + (space > 254 ? 254 : space);
> +	char *dend;
>  	int len = -1, i, j;
> -	if (p==end || !*p) return -1;
> +	if (p==end || size <= 0) return -1;
> +	dend = dest + (space > 254 ? 254 : space);

ok this part is better than mine

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Rich Felker]

On Fri, Sep 05, 2014 at 12:15:52AM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2014-09-04 16:20:53 -0400]:
> > >  	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
> 
> i assumed it to be ub

Yes, technically it probably should be considered as such, but I was
thinking of crazy things like large buffer lengths silently getting
converted from size_t to int due to the poor choice of types for the
signature. I definitely wouldn't want to treat negative values as
large unsigned values -- this could overflow a small buffer, and real
negatives could happen if "remaining buffer length" was computed
poorly and not checked. One could argue that all of these
considerations are beyond the scope of what the function needs to do,
but checing is easy, and I'm so sick of bugs and potential vulns from
this function already...

> > 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,
> 
> the rfc is not very clear but i think this case should work
> 
> a name is a sequence of labels terminated by a 0 length label
> 
> a compressed name is a leading sequence of labels terminated
> by a pointer that can be expanded to a name
> 
> if 'sequence of labels' can be empty and the conversion to
> dotted string notation is done after concatenating the
> leading and trailing sequence then there is no '..' issue

OK, based on this explanation, I think both forms are valid. I always
get stuck in the trap of reasoning in the mechanism of conversion
rather than the "sequence of labels" abstraction.

> > -	char *dend = dest + (space > 254 ? 254 : space);
> > +	char *dend;
> >  	int len = -1, i, j;
> > -	if (p==end || !*p) return -1;
> > +	if (p==end || size <= 0) return -1;
> > +	dend = dest + (space > 254 ? 254 : space);
> 
> ok this part is better than mine

Yes, whatever else we do, I think this is a nice addition.

Rich

Re: [PATCH] fix handling of zero length domain names in dn_expand

[Natanael Copa]

On Thu, 4 Sep 2014 20:11:29 +0200
Szabolcs Nagy <nsz@port70.net> wrote:

> 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;

How about, instead of adding int first, we do:
	char *dest_start = dest;

> +	if (p==end || dest==dend) return -1;
>  	/* 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;

and instead of the 2 above:
			if (dest != dest_start) *dest++ = '.';

> +			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;
>  		}

-nc