Re: Fwd: report 3/3: latent unterminated buffer in imap_wordcasecmp()

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 18 Jun 2026 11:01:52 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ajOxrVdDUnrf6hMY@devuan>
Hi Kevin, Kurt,

On 2026-06-18T09:44:14+0800, Kevin J. McCarthy wrote:
> On Wed, Jun 17, 2026 at 09:01:01PM -0400, Kurt Hackenberg wrote:
> > On Thu, Jun 18, 2026 at 08:19 +0800, Kevin J. McCarthy wrote:
> > 
> > > > Since Capabilities[x] elements are short fixed strings (e.g.,
> > > > "IMAP4rev1", "CONDSTORE"), ascii_strcasecmp() will return before
> > > > reading the uninitialized tmp[126] byte. However, the helper is
> > > > buggy in isolation.
> > > 
> > > This basically sums it up.  The comparison function is buggy, but
> > > since mutt controls the list of compared strings we won't hit the
> > > bug.
> > > 
> > > I don't want to accept an AI rewrite just before 2.4.0, so I'll try
> > > to look for alternative fixes.  Or if any of you want to take a look
> > > and propose a minimal fix for before I get to it, that would be
> > > great too.
> > 
> > All I know is your messages here, but offhand, I don't see a need for
> > any fix just before a release. A bug that won't happen can wait, can't
> > it?
> 
> :-) Yes, that's a good point.  I'll add a fix for this on to the future
> branch.
> 
> Right now it looks like a simpler fix is just this, but I'll take a closer
> look after the 2.4.0 release.
> 
> @@ -888,7 +888,7 @@ int imap_wordcasecmp(const char *a, const char *b)
>    int i;
> 
>    tmp[SHORT_STRING-1] = 0;

This line seems to be redundant with the removed 'tmp[i+1] = 0;', IIUC.
So, it's fine to remove the one below, as you did.  Am I reading
correctly?

> -  for (i=0;i < SHORT_STRING-2;i++,s++)
> +  for (i=0;i < SHORT_STRING-1;i++,s++)

Huh, why did we do -2 instead of -1?

>    {
>      if (!*s || IS_ASCII_WS(*s))
>      {
> @@ -897,7 +897,6 @@ int imap_wordcasecmp(const char *a, const char *b)
>      }
>      tmp[i] = *s;
>    }
> -  tmp[i+1] = 0;

This seemed to be for the case where the input string was too long, and
thus didn't contain a NUL or white space within the first SHORT_STRING-2
characters.

I wonder whether that truncation is correct.

>    return ascii_strcasecmp(a, tmp);
>  }

I still don't fully understand the previous code, the bug, or the fix.
The full code for the function is below, for context.

	$ grepc -tfd imap_wordcasecmp .
	./imap/util.c:int imap_wordcasecmp(const char *a, const char *b)
	{
	  char tmp[SHORT_STRING];
	  const char *s = b;
	  int i;

	  tmp[SHORT_STRING-1] = 0;
	  for (i=0;i < SHORT_STRING-2;i++,s++)
	  {
	    if (!*s || IS_ASCII_WS(*s))
	    {
	      tmp[i] = 0;
	      break;
	    }
	    tmp[i] = *s;
	  }
	  tmp[i+1] = 0;

	  return ascii_strcasecmp(a, tmp);
	}


Have a lovely day!
Alex

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