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