RE: [PATCH] Update dn_skipname to work with utf-8 names

RE: [PATCH] Update dn_skipname to work with utf-8 names

[Ryan Fairfax]

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

> This is unsafe on untrusted input, in that p += *p + 1 may have
> undefined behavior. It should be possible to make it safe by comparing
> against end-p before doing it, and returning -1 in that case.
> 
> At some level this is a "theoretical" bug, since the amount of the
> increment is bounded by 256, and any valid pointer is bounded away
> from the end of address space (the obvious point where the comparison
> p<end would go wrong) by at least 4096. However allowing UB like that
> means that it's no longer safe to build the file with hardened tooling
> that traps on UB, which is why it should be fixed.
> 
> Rich

Excellent point.  A corrected patch that checks that the length we're skipping is less than (end-p) is attached.

[-- Attachment #2: 0001-Update-dn_skipname-to-work-with-utf-8.patch --]
[-- Type: application/octet-stream, Size: 1327 bytes --]

From 1cdc3ec11986b979320b3cb73faf3497cd0c776f Mon Sep 17 00:00:00 2001
From: Ryan Fairfax <rfairfax@microsoft.com>
Date: Thu, 7 Mar 2019 13:20:54 -0800
Subject: [PATCH] Update dn_skipname to work with utf-8.

The original logic considered each byte until it either found a 0 value
 or a value >= 192.  This means if a string segment contained
utf-8 where a byte was >= 192 it was interepretted as a
compressed segment marker even if it wasn't in a position
where it should be interpretted as such.

The fix is to adjust dn_skipname to increment by each
segments size rather than look at each character.  This
avoids misinterpretting string segment characters  by not
considering those bytes.
---
 src/network/dn_skipname.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/network/dn_skipname.c b/src/network/dn_skipname.c
index d54c2e5d..d573b5fb 100644
--- a/src/network/dn_skipname.c
+++ b/src/network/dn_skipname.c
@@ -2,11 +2,14 @@
 
 int dn_skipname(const unsigned char *s, const unsigned char *end)
 {
-	const unsigned char *p;
-	for (p=s; p<end; p++)
+	const unsigned char *p = s;
+	while (p < end)
 		if (!*p) return p-s+1;
 		else if (*p>=192)
 			if (p+1<end) return p-s+2;
 			else break;
+		else
+			if ((end-p) < (*p + 1)) return -1;
+			else p += *p + 1;
 	return -1;
 }
-- 
2.17.1

Re: RE: [PATCH] Update dn_skipname to work with utf-8 names

[Rich Felker]

On Thu, Mar 07, 2019 at 09:34:58PM +0000, Ryan Fairfax wrote:
> > This is unsafe on untrusted input, in that p += *p + 1 may have
> > undefined behavior. It should be possible to make it safe by comparing
> > against end-p before doing it, and returning -1 in that case.
> > 
> > At some level this is a "theoretical" bug, since the amount of the
> > increment is bounded by 256, and any valid pointer is bounded away
> > from the end of address space (the obvious point where the comparison
> > p<end would go wrong) by at least 4096. However allowing UB like that
> > means that it's no longer safe to build the file with hardened tooling
> > that traps on UB, which is why it should be fixed.
> > 
> > Rich
> 
> Excellent point. A corrected patch that checks that the length we're
> skipping is less than (end-p) is attached.

Thanks. Merging with a minor change for style consistency and edits to
the description since this is not encoding-specific but just pertains
to any 8-bit values in labels.

Rich

Re: [PATCH] Update dn_skipname to work with utf-8 names

[Rich Felker]

On Thu, Mar 07, 2019 at 08:37:01PM +0000, Ryan Fairfax wrote:
> dn_skipname incorrectly handles names that contain characters outside the ASCII range due to a bug where it looks at every byte in the name rather than just the designated length bytes.  Only dn_skipname has this issue - the methods that actually parse out and return the name correctly handle this case.  The attached patch fixes the logic to only inspect length bytes and not try to interpret any part of the string characters themselves while skipping the name.

> From d8bddef3f45fe64160643e6d99d0e89da2b53d37 Mon Sep 17 00:00:00 2001
> From: Ryan Fairfax <rfairfax@microsoft.com>
> Date: Wed, 6 Mar 2019 14:35:28 -0800
> Subject: [PATCH] Update dn_skipname to work with utf-8.
> 
> The original logic considered each byte until it either found a 0 value
> or a value >= 192.  This means if a string segment contained
> utf-8 where a byte was >= 192 it was interepretted as a
> compressed segment marker even if it wasn't in a position
> where it should be interpretted as such.
> 
> The fix is to adjust dn_skipname to increment by each
> segments size rather than look at each character.  This
> avoids misinterpretting string segment characters  by not
> considering those bytes.
> ---
>  src/network/dn_skipname.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network/dn_skipname.c b/src/network/dn_skipname.c
> index d54c2e5d..8a87b20a 100644
> --- a/src/network/dn_skipname.c
> +++ b/src/network/dn_skipname.c
> @@ -2,11 +2,12 @@
>  
>  int dn_skipname(const unsigned char *s, const unsigned char *end)
>  {
> -	const unsigned char *p;
> -	for (p=s; p<end; p++)
> +	const unsigned char *p = s;
> +	while (p < end)
>  		if (!*p) return p-s+1;
>  		else if (*p>=192)
>  			if (p+1<end) return p-s+2;
>  			else break;
> +		else p += *p + 1;
>  	return -1;
>  }

This is unsafe on untrusted input, in that p += *p + 1 may have
undefined behavior. It should be possible to make it safe by comparing
against end-p before doing it, and returning -1 in that case.

At some level this is a "theoretical" bug, since the amount of the
increment is bounded by 256, and any valid pointer is bounded away
from the end of address space (the obvious point where the comparison
p<end would go wrong) by at least 4096. However allowing UB like that
means that it's no longer safe to build the file with hardened tooling
that traps on UB, which is why it should be fixed.

Rich

[PATCH] Update dn_skipname to work with utf-8 names

[Ryan Fairfax]

[-- Attachment #1.1: Type: text/plain, Size: 466 bytes --]

dn_skipname incorrectly handles names that contain characters outside the ASCII range due to a bug where it looks at every byte in the name rather than just the designated length bytes.  Only dn_skipname has this issue - the methods that actually parse out and return the name correctly handle this case.  The attached patch fixes the logic to only inspect length bytes and not try to interpret any part of the string characters themselves while skipping the name.

[-- Attachment #1.2: Type: text/html, Size: 2136 bytes --]

[-- Attachment #2: 0001-Update-dn_skipname-to-work-with-utf-8.patch --]
[-- Type: application/octet-stream, Size: 1276 bytes --]

From d8bddef3f45fe64160643e6d99d0e89da2b53d37 Mon Sep 17 00:00:00 2001
From: Ryan Fairfax <rfairfax@microsoft.com>
Date: Wed, 6 Mar 2019 14:35:28 -0800
Subject: [PATCH] Update dn_skipname to work with utf-8.

The original logic considered each byte until it either found a 0 value
or a value >= 192.  This means if a string segment contained
utf-8 where a byte was >= 192 it was interepretted as a
compressed segment marker even if it wasn't in a position
where it should be interpretted as such.

The fix is to adjust dn_skipname to increment by each
segments size rather than look at each character.  This
avoids misinterpretting string segment characters  by not
considering those bytes.
---
 src/network/dn_skipname.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/network/dn_skipname.c b/src/network/dn_skipname.c
index d54c2e5d..8a87b20a 100644
--- a/src/network/dn_skipname.c
+++ b/src/network/dn_skipname.c
@@ -2,11 +2,12 @@
 
 int dn_skipname(const unsigned char *s, const unsigned char *end)
 {
-	const unsigned char *p;
-	for (p=s; p<end; p++)
+	const unsigned char *p = s;
+	while (p < end)
 		if (!*p) return p-s+1;
 		else if (*p>=192)
 			if (p+1<end) return p-s+2;
 			else break;
+		else p += *p + 1;
 	return -1;
 }
-- 
2.17.1