Re: Security findings

Alejandro Colomar via Mutt-dev <[email protected]> Sun, 19 Apr 2026 08:42:48 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<aeR2SfdjEm-sgyGE@devuan>
Hi Kevin,

On 2026-04-19T07:48:40+0800, Kevin J. McCarthy wrote:
> On Sat, Apr 18, 2026 at 08:27:26PM +0200, Alejandro Colomar via Mutt-dev wrote:
> > 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.
> > > 
> 
> Thanks Alex.  Just to clarify, these were all very quick patches, with quick
> commit messages done at the end of a long day :-).  I appreciate the
> feedback though.
> 
> I'll think about the stylistic change suggestions.  I undertstand some of
> the arguments, but I'm not sure stable security updates are the place to
> make stylistic pattern changes in the code.

Since they only affect the new code, not old one, I think we could do
them.

> Just wanted to make one important comment:
> 
> > >   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.
> 
> /etc/shadow stores a readable ascii encoding, (base64?)  not the raw bytes
> themselves.  In this case, md5_buffer() actually generates binary data and
> stores it into hash_passwd.

As far as I know (I could be wrong), it stores the raw bytes.  I've
checked the source code of the shadow project, and didn't find anything
that seems to encode the data in any way.  It stores directly whatever
that crypt(3) produces.  Unless crypt(3) itself already gives us an
encoded version of the hash, of course.

Maybe crypt(3) produces characters for the hex digits and here in
mutt(1) we have the hex values as binary?  This could be a difference.
I did't check how it's done here.  Could you please confirm whether our
buffer will hold the same encoding that crypt(3) produces (when using an
MD5 salt) or not?

Then, maybe we should have two commits: one that increases the buffer
size to have the full buffer, and another that switches to memcpy(3), to
handle the binary, so that each commit is well documented.

> I don't know the probability of generating a 0x00 in a md5 hash, but since
> it's theoretically possible, memcpy() is the correct way to copy it over to
> secret.
> 
> > >   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?
> 
> It's possible it isn't working at all.  The branch only happened for
> passwords > 64 characters.

Hmmm.

> > >   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?
> 
> This is because the stable branch did not undergo tab-to-space conversion
> yet (which is going to make merging to master fun).
> 
> I also need to go back and fix the dprints I added to use the newer dprintf
> and parenthesis style, which I'll do today.
> 
> > > 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.
> 
> I'm inclined to agree.


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)