Re: Security findings

"Kevin J. McCarthy" <[email protected]> Sun, 19 Apr 2026 07:48:40 +0800
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeQYWFM_brkw0mI9@qinghai>
On Sat, Apr 18, 2026 at 08:27:26PM +0200, Alejandro Colomar via Mutt-dev wrote:
>On 2026-04-18T23:42:37+0800, Kevin J. McCarthy wrote:
>> On Sat, Apr 18, 2026 at 09:27:59PM +0800, Kevin J. McCarthy wrote:
>> > On Sat, Apr 18, 2026 at 02:14:53PM +0200, evilrabbit via Mutt-dev wrote:
>> > > Please find below a number of confirmed security findings in the mutt client.
>> > > None of these are significant but should probably be addressed.
>> >
>> > Thanks, I will start taking a look at these tomorrow.
>>
>> Just as a note to the other devs.  I started looking at some of these
>> tonight.
>>

Thanks Alex.  Just to clarify, these were all very quick patches, with 
quick commit messages done at the end of a long day :-).  I appreciate 
the feedback though.

I'll think about the stylistic change suggestions.  I undertstand some 
of the arguments, but I'm not sure stable security updates are the place 
to make stylistic pattern changes in the code.

Just wanted to make one important comment:

>>   kevin/stable-security-05        Fix IMAP auth_crm MD5 digest of secret to use memcpy().
>
>You (and the reporter) propose:
>
>	-    strfcpy ((char*) secret, (char*) hash_passwd, MD5_DIGEST_LEN);
>	+    memcpy (secret, hash_passwd, MD5_DIGEST_LEN);
>
>I propose:
>
>	-    strfcpy ((char*) secret, (char*) hash_passwd, MD5_DIGEST_LEN);
>	+    strfcpy ((char*) secret, (char*) hash_passwd, sizeof (secret));
>
>which would be equivalent to adding a +1 to the existing call.
>That's because we have declared earlier:
>
>	unsigned char secret[MD5_BLOCK_LEN+1];
>
>Thus, we have enough size to treat it as a C string, which makes it
>more consistent with the other branch.  Also, handling C strings is
>safer than handling raw bytes.  I think C strings should be fine,
>because MD5 hashes should not contain embedded NUL bytes (AFAIK);
>otherwise, they couldn't be stored in /etc/shadow at all.

/etc/shadow stores a readable ascii encoding, (base64?)  not the raw 
bytes themselves.  In this case, md5_buffer() actually generates binary 
data and stores it into hash_passwd.

I don't know the probability of generating a 0x00 in a md5 hash, but 
since it's theoretically possible, memcpy() is the correct way to copy 
it over to secret.

>>   kevin/stable-security-06        Fix imap_auth_gss() security_level size.
>
>I think the commit message should also say something about the change
>from long to uint32_t.  It's weird that it has been working, considering
>that long is usually 64-bits wide.  Was it really working by pure
>accident?

It's possible it isn't working at all.  The branch only happened for 
passwords > 64 characters.

>>   kevin/stable-security-07        Check for embedded nul in url_pct_decode().
>
>There seems to be an off-by-one indentation error in your patch?
>Or is it intentional?

This is because the stable branch did not undergo tab-to-space 
conversion yet (which is going to make merging to master fun).

I also need to go back and fix the dprints I added to use the newer 
dprintf and parenthesis style, which I'll do today.

>> These still need to be cleaned up, verified, and tested.
>>
>> For #8 I need to check the RFC myself, so if anyone with OpenSSL code
>> experience wants to confirm that would be welcome.
>>
>> For #3, I'm not sure if we really need to fix this.
>
>I'd say no.

I'm inclined to agree.

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