Re: mutt/2195: double free in rfc822_free_address() when using S/MIME encryption

[Rocco Rutte <pdmef@xxxxxxx>]

The following reply was made to PR mutt/2195; it has been noted by GNATS.

From: Rocco Rutte <pdmef@xxxxxxx>
To: bug-any@xxxxxxxxxxxxx
Cc: 
Subject: Re: mutt/2195: double free in rfc822_free_address() when using S/MIME 
encryption
Date: Thu, 6 Jul 2006 16:18:40 +0000

 Hi,
 
 * 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>
 
 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.
 
 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 yes and have a default key, you later do:
 
      int size = ...
      new_keylist = safe_malloc(size);
      snprintf (new_keylist, ...)
 
    *** Now new_keylist points to newly malloc()'d memory. ***
 
 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.
 
 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.
 
 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.
 
    bye, Rocco
 -- 
 :wq!