Re: Fwd: report 1/3: buffer under-read in rfc2047.c lwslen()

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 18 Jun 2026 23:05:31 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ajRcgzGiKX8d2L1G@devuan>
Hi Kevin,

On 2026-06-18T11:28:00+0800, Kevin J. McCarthy wrote:
> On Thu, Jun 18, 2026 at 08:13:35AM +0800, Kevin J. McCarthy wrote:
> > On Thu, Jun 18, 2026 at 08:09:01AM +0800, Kevin J. McCarthy wrote:
> > > From: Acts1631 <[email protected]>
> > > The lwslen() helper calculates the length of linear whitespace at the beginning of a string. It scans until the first non-whitespace character, then evaluates *(p - 1):
> > > 
> > > for (; p < s + n; p++)
> > >   if (!strchr(" \t\r\n", *p))
> > >   {
> > >     len = (size_t)(p - s);
> > >     break;
> > >   }
> > > if (strchr("\r\n", *(p-1)))
> > >   len = (size_t)0;
> > 
> > This relies on 1) a invalid rfc2047 encodeed word (because the spec says
> > there must be LWN) and 2) an obscure option being set, which defaults
> > off.
> > 
> > However, the bug report is valid.  Mutt will under-read the buffer.
> 
> I've taken a closer look.  It looks like this will not read outside of the
> buffer "*pd" passed into rfc2047_decode() because the call to lwnlen() is
> only triggered after an encoded word is found: meaning s > *pd.
> 
> Furthermore the underread will simply see the last character in the previous
> encoded word: '?' and so won't change the logic.
> 
> In this case, I think the proposed patch is the best solution:
> 
> diff --git a/rfc2047.c b/rfc2047.c
> index d72ae997..2bdd8553 100644
> --- a/rfc2047.c
> +++ b/rfc2047.c
> @@ -815,7 +815,7 @@ static size_t lwslen(const char *s, size_t n)
>        len = (size_t)(p - s);
>        break;
>      }
> -  if (strchr("\r\n", *(p-1))) /* LWS doesn't end with CRLF */
> +  if (len > 0 && strchr("\r\n", *(p-1))) /* LWS doesn't end with CRLF */
>      len = (size_t)0;
>    return len;
>  }

Reviewed-by: Alejandro Colomar <[email protected]>

> 
> I could instead bump p after the faiing strchr inside the loop, but this is
> less elegant because the strchr after the loop would run even if len were 0:

I agree the below is less elegant (and IMO harder to read).


Have a lovely night!
Alex

> 
>   for (; p < s + n; p++)
>     if (!strchr(" \t\r\n", *p))
>     {
>       len = (size_t)(p - s);
> +     p++;
>       break;
>     }
>   if (strchr("\r\n", *(p-1))) /* LWS doesn't end with CRLF */
>     len = (size_t)0;
>   return len;
> 
> However, again, since this is not reading outside of allocated memory, I
> will also delay this until after the 2.4.0 release.
> 
> -- 
> Kevin J. McCarthy
> GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA



-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)