Re: [PATCH] Fix IMAP auth_crm MD5 digest of secret to use memcpy().

"Kevin J. McCarthy" <[email protected]> Mon, 20 Apr 2026 13:33:41 +0800
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeW6tbDiwL6Qkmvy@qinghai>
On Mon, Apr 20, 2026 at 06:58:09AM +0200, Rene Kita wrote:
>On Mon, Apr 20, 2026 at 07:08:27AM +0800, Kevin J. McCarthy wrote:
>> On Sun, Apr 19, 2026 at 05:08:29PM +0200, Rene Kita wrote:
>> > On Sun, Apr 19, 2026 at 01:50:51PM +0800, Kevin J. McCarthy wrote:
>> > > For a secret longer that MD5_BLOCK_LEN, an MD5 digest is used
>> >
>> > s/that/than
>>
>> Ack.  Thank you.
>>
>> > The part after the else above is:
>> >  strfcpy((char *) secret, password, sizeof(secret));
>> >
>> > Are we dealing here with strings or with buffers of bytes/chars?
>>
>> Alex answered, but just to concur.  We are dealing with bytes for the output
>> of md5_buffer().  But the password parameter is a string.
>
>So depending on secret_len, secret will either contain raw bytes or a
>string (Yes, I know it's only a semantic difference)? That's ugly, but
>then the strfcpy here is probably correct and I have to apologize for
>the noise.

No need to apologize.  It is ugly, and confusing.  :-D

I'm going to alter the commit description slightly, because I realize I
left out an additional reason that this was broken.

strfcpy is passed the "size" of the storage, and it always sets 
dest[size-1] = '\0'.  But in this case, the result of md5_buffer() is 
MD5_DIGEST_LEN bytes.  So it was always truncating the result by one 
byte:

-    strfcpy ((char*) secret, (char*) hash_passwd, MD5_DIGEST_LEN);
+    memcpy(secret, hash_passwd, MD5_DIGEST_LEN);


The revised commit description is:

Fix IMAP auth_cram MD5 digest of secret to use memcpy().

For a secret longer than MD5_BLOCK_LEN, an MD5 digest is used instead.
However, mutt was incorrectly using strfcpy() instead of memcpy() on
the raw binary value returned by md5_buffer in hash_passwd.  If
hash_passwd contained an '\0' it would result in the value being
truncated.

Additionally, the strfcpy was truncating the hash_passwd by one byte
regardless, due to passing a "size" of MD5_DIGEST_LEN when the data
itself was length MD5_DIGEST_LEN.

This likely hasn't been a reported issue because:
1. CRAM-MD5 is not used much anymore
2. Most people likely don't have a password length greater than 64
    bytes.

Thanks to [email protected] for the security report.

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