From: Renaud Allard <renaud@allard.it>
Subject: Re: smtpd: 2 out-of-bounds bugs in unpack_dns.c and to.c
To: tech@openbsd.org
Date: Mon, 30 Mar 2026 18:48:15 +0200



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.
>