Re: mutt/2195: double free in rfc822_free_address() when using S/MIME encryption
The following reply was made to PR mutt/2195; it has been noted by GNATS.
From: Christoph Ludwig <ludwig@xxxxxxxxxxx>
To: bug-any@xxxxxxxxxxxxx
Cc:
Subject: Re: mutt/2195: double free in rfc822_free_address() when using S/MIME
encryption
Date: Fri, 7 Jul 2006 11:22:58 +0200
Hi,
On Thu, Jul 06, 2006 at 06:25:03PM +0200, Rocco Rutte wrote:
> * ludwig@xxxxxxxxxxx [06-03-09 10:08:07 +0100] wrote:
>
> >>Environment:
> >cludwig@castellio:~> mutt -v
> [...]
> >patch-1.5.6-ow.smime-encrypt-self.2
>
> Okay, I assume you use your hacked up version of the S/MIME-encrypt-self
> patch. I'm refering to a mail sent to mutt-dev:
>
> <http://www.does-not-exist.info/mail-archives/mutt-dev/msg02435.html>
>
Not quite. The patch I posted in the message you refer to is built upon
patch-1.5.6-ow.smime-encrypt-self.2. later on, I was told that I can achieve
the same by simply adding 'encrypt-to 0x912C2B2CB9449EFE' to
~/.gnupg/gpgsm.conf, so I did not apply my patch to recent mutt checkouts. (I
sometimes still need to use mutt's openssl crypto backend, that's why I kept
the original patch that works with openssl only.)
But the original patch manipulates (new_)keylist exactly the same way as my
later hack does, so your observations are probably still valid.
> I bet you can reproduce the bug if you don't decide to encrypt the
> messages to yourself or have no default S/MIME key set up.
Hm, yes, since I now usually rely on gpgsm to encrypt the FCC with my key, I
set smime_encrypt_self to no in .muttrc.
> To me, the problem seems to be the hack for mutt_protect() in that
> patch.
>
> First, you do:
>
> char* new_keylist = keylist;
>
> with keylist being the problematic double-free()'d pointer later in
> ci_send_message() of crypt.c. Next comes the query_quadoption() call for
> whether to encrypt to self.
[...]
> if you answer no:
>
> *** Nothing happens and new_keylist points _still_ to keylist. ***
>
> After the crypt_smime_build_smime_entity() call, you do:
>
> safe_free((void**)&new_keylist);
>
> which, if you answered no to the quad-option, means:
>
> safe_free((void**)&keylist);
>
> as new_keylist still points to keylist.
Good call! (I wonder why this bug only recently started to cause segfaults.)
> A solution would be to rewrite that part, or only call FREE() (not
> safe_free(), btw) if new_keylist!=keylist to not free() the parameter.
Are there guidelines available which convenience / safety functions and macros
should be used in mutt code?
>
> At least I read the code like that. That means that in your hacked up
> mutt_protect() routine, you end up free()'ing the keylist parameter and
> your system complains about it later.
>
> So my conclusion is that it's a problem with a patch, not mutt itself.
> valgrind(1) understandably complains about it so IHMO it's not a mutt
> bug since the original version mutt_protect() looks okay to me.
I rebuilt mutt without the encrypt_self patch and valgrind did not complain
about a doubly freed pointer anymore. So the patch seems indeed to be the
culprit.
Thank you very much for looking into my bug report!
Regards
Christoph
--
FH Worms - University of Applied Sciences
Fachbereich Informatik / Telekommunikation
Erenburgerstr. 19, 67549 Worms, Germany