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

[Kevin J. McCarthy]

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 ((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 evilrab...@tutamail.com for the security report.

--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

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:

signature.asc
Description: PGP signature