Hello,

(moving to tech@)

Renaud Allard <renaud@allard.it> wrote:
> Hi,
> 
> I ran some fuzzers on OpenSMTPD code and I found 2 out of bounds bugs.
> I opened the issue on github 
> https://github.com/OpenSMTPD/OpenSMTPD/issues/1305 as suggested by 
> opensmtpd.org website, but the flow looks a little bit strange, so I am 
> sending it here too. I guess thunderbird will mangle the patches, but 
> they are quite trivial anyway.
> 
> 1) unpack_dns.c: heap buffer over-read in dname_expand()
> 
> After consuming a DNS label, offset is advanced past the label data 
> (line 179: offset += n + 1). When control returns to the for-loop 
> condition at line 156:
> 
>      for (; (n = data[offset]); ) {
> 
> if offset equals len exactly, data[offset] reads one byte out of bounds. 
> The bounds check at line 150 (if (offset >= len) return -1) only guards 
> the first iteration, not subsequent ones.
> 
> Impact: OOB read of 1 byte. Could crash on guard pages or leak heap data 
> when the DNS response is attacker-controlled.
> 
> Suggested fix:
> 
> --- a/usr.sbin/smtpd/unpack_dns.c
> +++ b/usr.sbin/smtpd/unpack_dns.c
> @@ -153,7 +153,7 @@
>          res = 0;
>          end = start = offset;
> -       for (; (n = data[offset]); ) {
> +       for (; offset < len && (n = data[offset]); ) {
>                  if ((n & 0xc0) == 0xc0) {
>                          if (offset + 2 > len)
>                                  return (-1);

agree on this one, although I'd prefer to spell it in a slightly more
readable way (imho), the code is already overly complicated, no need to
make it more so.

--- unpack_dns.c
+++ unpack_dns.c
@@ -147,13 +147,14 @@ dname_expand(const unsigned char *data, size_t len, si
 	size_t		 n, count, end, ptr, start;
 	ssize_t		 res;
 
-	if (offset >= len)
-		return (-1);
-
 	res = 0;
 	end = start = offset;
 
-	for (; (n = data[offset]); ) {
+	for (;;) {
+		if (offset >= len)
+			return (-1);
+
+		n = data[offset];
 		if ((n & 0xc0) == 0xc0) {
 			if (offset + 2 > len)
 				return (-1);


> 2) to.c: out-of-bounds array access in text_to_netaddr()
> 
> When the input string is "[", s is advanced past the bracket, then 
> strlcpy copies an empty string into buf, setting len = 0. Line 251 then 
> evaluates buf[len-1], which wraps to buf[SIZE_MAX] due to unsigned 
> arithmetic, causing a massive out-of-bounds read.
> 
> Impact: Undefined behavior / arbitrary stack memory read. Triggered by 
> any network address string that is exactly "[".
> 
> Suggested fix:
> 
> --- a/usr.sbin/smtpd/to.c
> +++ b/usr.sbin/smtpd/to.c
> @@ -248,7 +248,7 @@
>                          if ((len = strlcpy(buf, s, sizeof buf)) >= 
> sizeof buf)
>                                  return 0;
> -                       if (buf[len-1] != ']')
> +                       if (len == 0 || buf[len-1] != ']')
>                                  return 0;
>                          buf[len-1] = 0;

this instead looks ok to me.  I've done some quick checking and it
doesn't seem to be remotely triggable, because the name comes from
getsockname or from admin-controlled values in tables, afaik.