All valid points. Thanks for writing the patch -- I'll try to find
some time to review it soon.
On Monday, 13 October 2008 at 15:13, TAKAHASHI Tamotsu wrote:
> Hi list,
>
> I think mutt's (Open)SSL support is kinda poor.
> It only checks a peer's direct signer/issuer.
> As SSL has a recursive structure
> (selfsigned root -> intermediate certs -> peer),
> I hope mutt will check certs a little deeper.
>
> Also, if a peer cert is not signed directly
> by any of $certificate_file or system certs,
> mutt asks me whether or not to trust it showing its
> MD5 fingerprint although OpenSSL defaults to SHA1.
> This is annoying, too.
>
> If anyone is interested,
> I wrote a patch (attached) to try to
> - check cert_chain and cache trusted certs
> - allow users to save intermediate certs
> - show SHA1 fingerprint instead of MD5
>
> Beware, I am no (Open)SSL expert either.
>
>
> Example situation:
>
> My POPS servers are secure.plala.or.jp (I don't
> know how many servers this domain name points to,
> but I'm given a different cert every time I
> connect to it).
>
> Their certs were all issued by
> "VeriSign International Server CA - Class 3",
> which was issued by VeriSign's Root CA,
> "Class 3 Public Primary Certification Authority".
>
> Most SSL clients need only the root cert.
> But mutt complains that I don't have
> "VeriSign International Server CA - Class 3"
> in my $certificate_file.
>
> Also note, the "missing" cert is actually sent
> to mutt from the peer POPS server.
> Mutt simply ignores it and complains to me.
>
> Even if I compromise, I don't want to append
> all the peer certs. I only want
> "VeriSign International Server CA - Class 3"
> in my $certificate_file. But how can I add it
> to the file? Mutt asks me only for the peer.
>
> Bye,
> diff -r 10a1f06bc8aa globals.h
> +++ b/globals.h Mon Oct 13 13:36:24 2008 +0900
> @@ -129,7 +129,8 @@
> WHERE char *SslCertFile INITVAL (NULL);
> WHERE char *SslClientCert INITVAL (NULL);
> #ifdef USE_SSL_OPENSSL
> -WHERE LIST *SslSessionCerts INITVAL (NULL);
> +#include <openssl/ssl.h>
> +WHERE STACK_OF(X509) *SslSessionCerts INITVAL (NULL);
> #endif
> WHERE char *SslEntropyFile INITVAL (NULL);
> #ifdef USE_SSL_GNUTLS
> diff -r 10a1f06bc8aa mutt_ssl.c
> +++ b/mutt_ssl.c Mon Oct 13 13:36:24 2008 +0900
> @@ -60,6 +60,14 @@
> #define HAVE_ENTROPY() (!access(DEVRANDOM, R_OK) || entropy_byte_count
> >= 16)
> #endif
>
> +/* keep a handle on accepted certificates in case we want to
> + * open up another connection to the same server in this session */
> +#define CACHE_TRUSTED(c) \
> + if (!SslSessionCerts) \
> + SslSessionCerts = sk_new_null(); \
> + sk_X509_push (SslSessionCerts, c); \
> + dprint (1, (debugfile, "trusted: %s\n", c->name))
> +
> typedef struct _sslsockdata
> {
> SSL_CTX *ctx;
> @@ -76,7 +84,8 @@
> static int ssl_socket_open (CONNECTION * conn);
> static int ssl_socket_close (CONNECTION * conn);
> static int tls_close (CONNECTION* conn);
> -static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data);
> +static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data, int);
> +static int interactive_check_cert (X509 *cert);
> static void ssl_get_client_cert(sslsockdata *ssldata, CONNECTION *conn);
> static int ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
> static int ssl_negotiate (CONNECTION *conn, sslsockdata*);
> @@ -345,7 +354,7 @@
> return -1;
> }
>
> - if (!ssl_check_certificate (conn, ssldata))
> + if (!ssl_check_certificate (conn, ssldata, 1))
> return -1;
>
> mutt_message (_("SSL connection using %s (%s)"),
> @@ -415,7 +424,7 @@
> unsigned int n;
> int j;
>
> - if (!X509_digest (cert, EVP_md5 (), md, &n))
> + if (!X509_digest (cert, EVP_sha1 (), md, &n))
> {
> snprintf (s, l, _("[unable to calculate]"));
> }
> @@ -424,7 +433,7 @@
> for (j = 0; j < (int) n; j++)
> {
> char ch[8];
> - snprintf (ch, 8, "%02X%s", md[j], (j % 2 ? " " : ""));
> + snprintf (ch, 8, "%02X%s", md[j], (j + 1 < (int) n ? ":" : ""));
> safe_strcat (s, l, ch);
> }
> }
> @@ -452,7 +461,7 @@
> {
> X509_STORE_CTX xsc;
> X509_STORE *ctx;
> - int pass = 0;
> + int pass = 0, i;
>
> ctx = X509_STORE_new ();
> if (ctx == NULL) return 0;
> @@ -470,6 +479,9 @@
> else
> dprint (2, (debugfile, "X509_STORE_load_locations_failed\n"));
>
> + for (i = 0; i < sk_X509_num (SslSessionCerts); i++)
> + pass += (X509_STORE_add_cert (ctx, sk_X509_value (SslSessionCerts, i))
> != 0);
> +
> if (pass == 0)
> {
> /* nothing to do */
> @@ -477,7 +489,7 @@
> return 0;
> }
>
> - X509_STORE_CTX_init (&xsc, ctx, peercert, NULL);
> + X509_STORE_CTX_init (&xsc, ctx, peercert, SslSessionCerts);
>
> pass = (X509_verify_cert (&xsc) > 0);
> #ifdef DEBUG
> @@ -490,6 +502,7 @@
> snprintf (buf, sizeof (buf), "%s (%d)",
> X509_verify_cert_error_string(err), err);
> dprint (2, (debugfile, "X509_verify_cert: %s\n", buf));
> + dprint (2, (debugfile, " [%s]\n", peercert->name));
> }
> #endif
> X509_STORE_CTX_cleanup (&xsc);
> @@ -525,16 +538,17 @@
> unsigned char peermd[EVP_MAX_MD_SIZE];
> unsigned int peermdlen;
> X509 *cert;
> - LIST *scert;
> + int i;
>
> - if (!X509_digest (peercert, EVP_sha1(), peermd, &peermdlen))
> + if (!X509_digest (peercert, EVP_sha1(), peermd, &peermdlen)
> + || !SslSessionCerts)
> {
> return 0;
> }
>
> - for (scert = SslSessionCerts; scert; scert = scert->next)
> + for (i = sk_X509_num (SslSessionCerts); i-- > 0;)
> {
> - cert = *(X509**)scert->data;
> + cert = sk_X509_value (SslSessionCerts, i);
> if (!compare_certificates (cert, peercert, peermd, peermdlen))
> {
> return 1;
> @@ -717,16 +731,12 @@
> return rc;
> }
>
> -static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data)
> +static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data, int
> interactive)
> {
> - char *part[] =
> - {"/CN=", "/Email=", "/O=", "/OU=", "/L=", "/ST=", "/C="};
> - char helpstr[LONG_STRING];
> char buf[SHORT_STRING];
> - MUTTMENU *menu;
> - int done, row, i, certerr_hostname = 0;
> - FILE *fp;
> - char *name = NULL, *c;
> + int i, certerr_hostname = 0, chain_len;
> + STACK_OF(X509) *chain;
> + X509 *cert;
>
> /* check session cache first */
> if (check_certificate_cache (data->cert))
> @@ -760,8 +770,60 @@
> return 1;
> }
>
> - /* interactive check from user */
> - menu = mutt_new_menu ();
> + if (!interactive)
> + return 0;
> +
> + chain = SSL_get_peer_cert_chain (data->ssl);
> + chain_len = sk_X509_num (chain);
> + if (!chain || (chain_len < 1))
> + return interactive_check_cert (data->cert);
> + else
> + {
> + /* check the chain from root to peer */
> + for (i = chain_len; i-- > 0;)
> + {
> + cert = sk_X509_value (chain, i);
> + if (check_certificate_cache (cert))
> + dprint (2, (debugfile, "ssl chain: already cached: %s\n", cert->name));
> + else if (i /* 0 is the peer */ || !certerr_hostname)
> + {
> + if (check_certificate_by_signer (cert))
> + {
> + dprint (2, (debugfile, "ssl chain: checked by signer: %s\n",
> cert->name));
> + CACHE_TRUSTED (cert);
> + }
> + else if (SslCertFile && check_certificate_by_digest (cert))
> + {
> + dprint (2, (debugfile, "ssl chain: trusted with file: %s\n",
> cert->name));
> + CACHE_TRUSTED (cert);
> + }
> + else /* allow users to shoot their foot */
> + {
> + dprint (2, (debugfile, "ssl chain: check failed: %s\n", cert->name));
> + interactive_check_cert (cert);
> + }
> + }
> + else /* highly suspicious because (i==0 && certerr_hostname) */
> + interactive_check_cert (cert);
> + }
> + /* retry with the trusted chain */
> + return ssl_check_certificate (conn, data, 0);
> + }
> +}
> +
> +static int interactive_check_cert (X509 *cert)
> +{
> + char *part[] =
> + {"/CN=", "/Email=", "/O=", "/OU=", "/L=", "/ST=", "/C="};
> + char helpstr[LONG_STRING];
> + char buf[SHORT_STRING];
> + MUTTMENU *menu = mutt_new_menu();
> + int done, row, i;
> + FILE *fp;
> + char *name = NULL, *c;
> +
> + dprint (2, (debugfile, "ssl interactive_check_cert: %s\n", cert->name));
> +
> menu->max = 19;
> menu->dialog = (char **) safe_calloc (1, menu->max * sizeof (char *));
> for (i = 0; i < menu->max; i++)
> @@ -770,7 +832,7 @@
> row = 0;
> strfcpy (menu->dialog[row], _("This certificate belongs to:"),
> SHORT_STRING);
> row++;
> - name = X509_NAME_oneline (X509_get_subject_name (data->cert),
> + name = X509_NAME_oneline (X509_get_subject_name (cert),
> buf, sizeof (buf));
> for (i = 0; i < 5; i++)
> {
> @@ -781,7 +843,7 @@
> row++;
> strfcpy (menu->dialog[row], _("This certificate was issued by:"),
> SHORT_STRING);
> row++;
> - name = X509_NAME_oneline (X509_get_issuer_name (data->cert),
> + name = X509_NAME_oneline (X509_get_issuer_name (cert),
> buf, sizeof (buf));
> for (i = 0; i < 5; i++)
> {
> @@ -792,18 +854,18 @@
> row++;
> snprintf (menu->dialog[row++], SHORT_STRING, _("This certificate is
> valid"));
> snprintf (menu->dialog[row++], SHORT_STRING, _(" from %s"),
> - asn1time_to_string (X509_get_notBefore (data->cert)));
> + asn1time_to_string (X509_get_notBefore (cert)));
> snprintf (menu->dialog[row++], SHORT_STRING, _(" to %s"),
> - asn1time_to_string (X509_get_notAfter (data->cert)));
> + asn1time_to_string (X509_get_notAfter (cert)));
>
> row++;
> buf[0] = '\0';
> - x509_fingerprint (buf, sizeof (buf), data->cert);
> + x509_fingerprint (buf, sizeof (buf), cert);
> snprintf (menu->dialog[row++], SHORT_STRING, _("Fingerprint: %s"), buf);
>
> menu->title = _("SSL Certificate check");
> - if (SslCertFile && X509_cmp_current_time (X509_get_notAfter (data->cert))
> >= 0
> - && X509_cmp_current_time (X509_get_notBefore (data->cert)) < 0)
> + if (SslCertFile && X509_cmp_current_time (X509_get_notAfter (cert)) >= 0
> + && X509_cmp_current_time (X509_get_notBefore (cert)) < 0)
> {
> menu->prompt = _("(r)eject, accept (o)nce, (a)ccept always");
> menu->keys = _("roa");
> @@ -836,7 +898,7 @@
> done = 0;
> if ((fp = fopen (SslCertFile, "a")))
> {
> - if (PEM_write_X509 (fp, data->cert))
> + if (PEM_write_X509 (fp, cert))
> done = 1;
> fclose (fp);
> }
> @@ -853,15 +915,13 @@
> /* fall through */
> case OP_MAX + 2: /* accept once */
> done = 2;
> - /* keep a handle on accepted certificates in case we want to
> - * open up another connection to the same server in this session */
> - SslSessionCerts = mutt_add_list_n (SslSessionCerts, &data->cert,
> - sizeof (X509 **));
> + CACHE_TRUSTED (cert);
> break;
> }
> }
> unset_option(OPTUNBUFFEREDINPUT);
> mutt_menuDestroy (&menu);
> + dprint (2, (debugfile, "ssl interactive_check_cert: done=%d\n", done));
> return (done == 2);
> }
>
Attachment:
pgp1mIWcr3lKF.pgp
Description: PGP signature