Re: Security findings

Alejandro Colomar via Mutt-dev <[email protected]> Sat, 18 Apr 2026 20:27:26 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeO_W8F6_Zn1CJ5q@devuan>
Hi Kevin,

On 2026-04-18T23:42:37+0800, Kevin J. McCarthy wrote:
> On Sat, Apr 18, 2026 at 09:27:59PM +0800, Kevin J. McCarthy wrote:
> > On Sat, Apr 18, 2026 at 02:14:53PM +0200, evilrabbit via Mutt-dev wrote:
> > > Please find below a number of confirmed security findings in the mutt client.
> > > None of these are significant but should probably be addressed.
> > 
> > Thanks, I will start taking a look at these tomorrow.
> 
> Just as a note to the other devs.  I started looking at some of these
> tonight.
> 
> I've pushed up to gitlab some (very quickly made) branches:
>   kevin/stable-security-01        Fix NULL dereference in show_sig_summary().

You proposed:

	-      time_t at = key->subkeys->expires ? key->subkeys->expires : 0;
	+      time_t at = 0;
	+      if (key && key->subkeys)
	+        at = key->subkeys->expires;

I propose keeping the ternary operator, which reduces the diff:

	-      time_t at = key->subkeys->expires ? key->subkeys->expires : 0;
	+      time_t at = key && key->subkeys ? key->subkeys->expires : 0;

>   kevin/stable-security-02        Fix infinite loop in gpgme data_object_to_stream().

You proposed:

	-  while ((nread = gpgme_data_read (data, buf, sizeof (buf))))
	+  while ((nread = gpgme_data_read (data, buf, sizeof (buf))) > 0)

I propose moving the comparison forward:

	-  while ((nread = gpgme_data_read (data, buf, sizeof (buf))))
	+  while (0 < (nread = gpgme_data_read (data, buf, sizeof (buf))))

Rationale: Most of the time, I agree that having the variable part of
the comparison first results in better readability.  However, in cases
where the assignment operator ('=') is used within an if or a while
condition, it's dangerous to have the '=' before the comparison.
Consider the following code:

	while ((foo = bar) > 42)

	while ((foo = bar > 42))

That's a pretty easy typo that one can write, and the compiler wouldn't
notice.  At the end of the expression, we have a mess of parentheses
that allows making this mistake by accident.

	alx@devuan:~/tmp$ cat comp.c 
	int
	main(int argc, char *argv[])
	{
		if ((argc = argv[0][0]) > 0)
			return 1;
		if ((argc = argv[0][0] > 0))
			return 2;
	}
	alx@devuan:~/tmp$ gcc -Wall -Wextra comp.c 
	alx@devuan:~/tmp$ 

However, if the order is reversed, it's impossible to make that mistake:

	while (42 < (foo = bar))

	while ((42 < foo = bar))  // This results in a compiler error

The mistake is first of all, less likely, because at the beginning of
the expression, there's still not a mess of parentheses, unlike at the
end of it.  But also, because < has more precedence than =, the
comparison is performed first, and then the LHS of the = is not an
lvalue, which is a constraint violation (compiler error).

>   kevin/stable-security-05        Fix IMAP auth_crm MD5 digest of secret to use memcpy().

You (and the reporter) propose:

	-    strfcpy ((char*) secret, (char*) hash_passwd, MD5_DIGEST_LEN);
	+    memcpy (secret, hash_passwd, MD5_DIGEST_LEN);

I propose:

	-    strfcpy ((char*) secret, (char*) hash_passwd, MD5_DIGEST_LEN);
	+    strfcpy ((char*) secret, (char*) hash_passwd, sizeof (secret));

which would be equivalent to adding a +1 to the existing call.
That's because we have declared earlier:

	unsigned char secret[MD5_BLOCK_LEN+1];

Thus, we have enough size to treat it as a C string, which makes it
more consistent with the other branch.  Also, handling C strings is
safer than handling raw bytes.  I think C strings should be fine,
because MD5 hashes should not contain embedded NUL bytes (AFAIK);
otherwise, they couldn't be stored in /etc/shadow at all.

>   kevin/stable-security-06        Fix imap_auth_gss() security_level size.

I think the commit message should also say something about the change
from long to uint32_t.  It's weird that it has been working, considering
that long is usually 64-bits wide.  Was it really working by pure
accident?  I get confused when it comes to endianness, so an explanation
in the commit message of how much it was broken, and why we didn't
notice earlier would be nice.

>   kevin/stable-security-07        Check for embedded nul in url_pct_decode().

There seems to be an off-by-one indentation error in your patch?
Or is it intentional?

Also, maybe the comment should be removed.  It seems redundant with the
code and commit message.  I tend to think the commit message is enough
for clarifying those details, and it will appear with git-blame(1)
easily.  But I guess it's up to your preference.

>   kevin/stable-security-08        Abort if there are DNS entries but don't match.

I didn't check this one.

> 
> These still need to be cleaned up, verified, and tested.
> 
> For #8 I need to check the RFC myself, so if anyone with OpenSSL code
> experience wants to confirm that would be welcome.
> 
> For #3, I'm not sure if we really need to fix this.

I'd say no.

> 
> For #4, I welcome input from others about this.  Randomizers are not my
> thing.  If the algorithm is insufficient for what we are using it for, then
> I welcome help in implementing something better.

I didn't check this.


Have a lovely night!
Alex

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