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

Alejandro Colomar via Mutt-dev <[email protected]> Mon, 20 Apr 2026 11:46:26 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeX0yea8I-FumTbz@debian>
Hi Kevin,

On 2026-04-20T13:33:41+0800, Kevin J. McCarthy wrote:
> 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.

Agree.

This reminds me that whenever possible, strfcpy() should be called with
_Countof() --or something equivalent; the Linux kernel has
ARRAY_SIZE()--, to avoid precisely this mistake.

And because of that, it's also wise to have a dedicated macro that will
do it for us:

	#ifndef countof
	#define countof(a)  (sizeof(a) / sizeof((a)[0]))
	#endif

	// strfcpy_a - strfcpy array
	#define strfcpy_a(dst, src)  strfcpy(dst, src, countof(dst))

sizeof() could work, but it may be dangerous, as one may use it with a
pointer, and get crazy results.

When the input is not an array, there's no good solution at the moment.
And that's why I'm trying to have the _Countof() operator work with
array parameters just as well in C2y.  I have some patches for GCC
ready, but need to convince the committee before merging them in GCC.


Have a lovely day!
Alex

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



-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)