From: "Omar Polo" <op@omarpolo.com>
Subject: Re: smtpd: 2 out-of-bounds bugs in unpack_dns.c and to.c
To: Renaud Allard <renaud@allard.it>
Cc: tech@openbsd.org
Date: Wed, 01 Apr 2026 13:37:05 +0200

both diffs were committed, thanks!

Renaud Allard <renaud@allard.it> wrote:
> 
> 
> On 30/03/2026 18:16, Omar Polo wrote:
> > 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);
> > 
> > 
> 
> I agree, it's way more readable that way.
> 
> >> 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.
> > 
>