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
Description: PGP signature
