On 2026-04-20T11:52:40+0200, Alejandro Colomar wrote: > Hi Kevin, > > On 2026-04-20T07:04:02+0800, Kevin J. McCarthy wrote: > > On Sun, Apr 19, 2026 at 08:24:01PM +0200, Alejandro Colomar via Mutt-dev > > wrote: > > > strfcpy() ensures it creates a string. But that's not what we want. > > > Here, we just want raw bytes in the output. It's only the input which > > > is a string (password). > > > > > > strncpy(3) is quite appropriate for this specific use. It's a function > > > that takes a C string as input, and fills a fixed-size buffer with bytes > > > from it, zeroing the unused remainder part of the buffer. > > > > > > Because of this use of strncpy(3), we can remove the memset(3) calls. > > > They're now entirely redundant. (The other branch fills the entire > > > buffer, so it was only meaningful in the branch we're touching.) > > > > It's early and my brain isn't fully awake. But I believe this is incorrect. > > The other branch only fills to MD5_DIGEST_LEN. Removing the memset() in > > that case, and copying the entire secret buffer, instead of secret_len, > > would break the algorithm. > > Oh, I had misread and confused MD_DIGEST_LEN by MD5_BLOCK_LEN, and > thought there was only one size being used everywhere. Yup; please > discard my patch. > > I'll have another look today.
I think this is correct:
diff --git i/imap/auth_cram.c w/imap/auth_cram.c
index 9844f444..b42eacb1 100644
--- i/imap/auth_cram.c
+++ w/imap/auth_cram.c
@@ -135,8 +135,8 @@ static void hmac_md5(const char *password, char
*challenge,
unsigned char *response)
{
struct md5_ctx ctx;
+ char secret[MD5_BLOCK_LEN];
unsigned char ipad[MD5_BLOCK_LEN], opad[MD5_BLOCK_LEN];
- unsigned char secret[MD5_BLOCK_LEN+1];
unsigned char hash_passwd[MD5_DIGEST_LEN];
size_t secret_len, chal_len;
int i;
@@ -149,16 +149,15 @@ static void hmac_md5(const char *password, char
*challenge,
if (secret_len > MD5_BLOCK_LEN)
{
md5_buffer(password, secret_len, hash_passwd);
+ memset(secret, 0, sizeof(secret));
memcpy(secret, hash_passwd, MD5_DIGEST_LEN);
secret_len = MD5_DIGEST_LEN;
}
else
- strfcpy((char *) secret, password, sizeof(secret));
+ strncpy(secret, password, sizeof(secret));
- memset(ipad, 0, sizeof(ipad));
- memset(opad, 0, sizeof(opad));
- memcpy(ipad, secret, secret_len);
- memcpy(opad, secret, secret_len);
+ memcpy(ipad, secret, sizeof(ipad));
+ memcpy(opad, secret, sizeof(opad));
for (i = 0; i < MD5_BLOCK_LEN; i++)
{
I'll think a bit more about it later, and will send it as a patch.
Cheers,
Alex
--
<https://www.alejandro-colomar.es>
signature.asc
Description: PGP signature
