[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 7 Mar 2019 15:55:23 -0500
From: Rich Felker <dalias@...c.org>
To: Ryan Fairfax <rfairfax@...rosoft.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: [PATCH] Update dn_skipname to work with utf-8 names
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@...rosoft.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
