Am 12.02.2017 um 17:49 schrieb Kevin J. McCarthy:
>
> Actually, since this loop, has no break inside, it doesn't make sense to
> have the X509_free() after the loop. Let me see if I can make sure the
> PEM_read_X509() frees the cert even on EOF, and if so we can just pull
> the X509_free() completely out.
>
I semantically only understand half of the paragraph, but the patch I
provided at 18:31 in my time zone with node ID
445386dcae5a017dfbc2a915977a5968c8d44c0a achieves just that, i. e. you
can move the X509_free() completely out of the while(){} with one change
(already made in that patch), you must no longer assign with cert =
PEM_read_X509().
I've traced this through the loop, with expired certificates stored, the
'cert' never changes after the first non-NULL allocation, and is left
untouched by PEM_read_X509() returning NULL, so even in that case we can
still X509_free(cert), we only mustn't lose the pointer.
Just to remove any potential confusion about the state of the code that
I am talking about, I'm pushing it to https://bitbucket.org/m-a/mutt as
we speak, where it should arrive about 20 min after this message has
been sent. I'll turn my focus to something else while the upload is
going and check back later.
There is another issue with the interactive prompting, and that is certificate
duplication in mutt's certificate store, and happens at least on hostname
mismatches (there may be other triggers, I haven't investigated).
This is with the current code as per my previous patch (mentioned above) in
this thread.
Scenario:
I am currently using a certificate that does not match the hostname because I'm
using "-f imaps://localhost/...", but it is for a FQDN, and now I have a dozen
VERBATIM IDENTICAL copies of that mismatched certificate (per subject hash) in
my ~/.mutt_certificates file...
# split ~/.mutt_certificates into F1 F2 F3 ... files
awk '/^-----BEGIN/{x="F" ++i;}{if (x) {print > x;}}' ~/.mutt_certificates
# obtain the contained certificates' hashes, sort, and count unique ones.
for i in F[0-9]* ; do printf "%s: " $i ; openssl x509 -subject_hash <$i -noout
; done \
| sort -n | cut -f2 -d: | uniq -c | sort -n
I suggest that mutt should NEVER write a certificate that's already present in
~/.mutt_certificates.
Whether it should query about it (especially with a host mismatch) is a
different matter because my answer may only be valid for the
hostname-certificate combination, and not the certificate on its own.
No time to fully debug that part now though, it happens here:
> /* check hostname only for the leaf certificate */ buf[0] = 0; if (pos
> == 0 && option (OPTSSLVERIFYHOST) != MUTT_NO) { if (!check_host (cert,
> host, buf, sizeof (buf))) { mutt_error (_("Certificate host check
> failed: %s"), buf); mutt_sleep (2); return interactive_check_cert
> (cert, pos, len); // < === ERROR TRIGGER ========== } dprint (2,
> (debugfile, "ssl_verify_callback: hostname check passed\n")); }
where we call interactive_check_cert, which in itself does not check if the
certificate it is about to store is a duplicate.
I can't currently propose where it's best to avoid the duplication of
certificates, whether you defer this to interactive_check_cert() or place it
elsewhere.
If deemed worthwhile for contrib/ or thereabouts, I can write a safe shell
script based on the two-liner above that safely purges duplicates from
~/.mutt_certificates.
Cheers,
Matthias
