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