On Tuesday, 18 November 2008 at 22:30, TAKAHASHI Tamotsu wrote:
> Here it is. (I haven't tested this version, but it
> compiles without warning.)
>
> I don't declare the function "inline", because ANSI C
> doesn't have "inline" keyword (some compilers don't),
> and the routine is called only a few times (the number
> of the certs in your cert-chain). Feel free to modify.
autoconf handles this, defining out inline if the compiler doesn't
support it.
> Thanks for reviewing my patch!
I've pushed it with a few modifications.
> diff -r 8199185fa595 mutt_ssl.c
> +++ b/mutt_ssl.c Tue Nov 18 22:13:18 2008 +0900
...
> @@ -345,7 +351,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)"),
I've backed out this change. Only ssl_check_certificate itself wants
a different parameter for the interactive flag, so there's no reason
to burden outside callers with this complexity. Instead I've pulled
the non-interactive check up into its own function. In general I think
it's good to keep the argument list to the minimum.
> @@ -717,16 +728,20 @@
> return rc;
> }
>
> -static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data)
> +static int ssl_cache_trusted_cert (X509 *c)
> {
> - char *part[] =
> - {"/CN=", "/Email=", "/O=", "/OU=", "/L=", "/ST=", "/C="};
> - char helpstr[LONG_STRING];
> + dprint (1, (debugfile, "trusted: %s\n", c->name));
> + if (!SslSessionCerts)
> + SslSessionCerts = sk_new_null();
> + return (sk_X509_push (SslSessionCerts, X509_dup(c)));
> +}
> +
> +static int ssl_check_certificate (CONNECTION *conn, sslsockdata * data, int
> interactive)
> +{
> 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 +775,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));
> + ssl_cache_trusted_cert (cert);
> + }
> + else if (SslCertFile && check_certificate_by_digest (cert))
> + {
> + dprint (2, (debugfile, "ssl chain: trusted with file: %s\n",
> cert->name));
> + ssl_cache_trusted_cert (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);
this recursion is needless. The loop should return success as soon as
any of the checks succeed -- I've changed this.
Thanks for the patch -- now we just need a gnutls version ;)
Attachment:
pgpEeoA85lX_F.pgp
Description: PGP signature