Re: [PATCH v1] imap/auth_cram.c: Don't use strfcpy() where we don't want to generate a C string

Alejandro Colomar via Mutt-dev <[email protected]> Mon, 20 Apr 2026 12:02:11 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeX5a4mcK9ihasek@debian>
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 (application/pgp-signature, 833 B)