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

"Kevin J. McCarthy" <[email protected]> Thu, 18 Jun 2026 18:25:01 +0800
Newsgroups	gmane.mail.mutt.devel
Message-ID	<[email protected]>
On Thu, Jun 18, 2026 at 11:01:52AM +0200, Alejandro Colomar via Mutt-dev wrote:
>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?

Yes, that's right.  It was redundant once the loop comparison was fixed.

I believe I alternatively could remove the
   tmp[SHORT_STRING-1] = 0
before the loop, along with the
   tmp[i] = 0
inside the loop, and change the assignment after the loop to
   tmp[i] = 0
I think the final assignment would cover both exit cases.

But, to me, it feels a little better seeing the first and middle 
assignments.  Otherwise I have to think more carefully about the two 
cases of i: one because of the "break" and the other because it failed 
the loop condition.

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

Yes, this was the weird part.  I have no idea.  It's a straight-forward 
function, so I'm guessing it was just a think-o.  The only caller is 
looking at the CAPABILITY list coming from the IMAP server, word by 
word, and comparing it to the Capability array defined in 
imap/command.c.

The "bug" was if b was of length 127, the loop would terminate copying 
bytes 0..125.  The final for loop increment would set i=126,Then, 
"tmp[i+1] = 0" would nul-terminate the string at byte 127.  So byte 126 
would be unset.

None of the capabilities we have in the mutt array are that long, so 
this wouldn't matter, but it's still a bug.

-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA
signature.asc (application/pgp-signature, 833 B)