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