On 09/17/2011 06:25 PM, [email protected] wrote:
> Author: druggeri
> Date: Sat Sep 17 16:25:17 2011
> New Revision: 1172010
>
> URL: http://svn.apache.org/viewvc?rev=1172010&view=rev
> Log:
> Log better information and prevent leak of an X509 structure for
> SSLProxyMachineCertificateChainFile
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=1172010&r1=1172009&r2=1172010&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Sat Sep 17 16:25:17 2011
> @@ -1181,21 +1181,57 @@ static void ssl_init_proxy_certs(server_
> X509_STORE_load_locations(store, pkp->ca_cert_file, NULL);
>
> for (n = 0; n < ncerts; n++) {
> - int i;
> + int i, res;
> + char cert_cn[256];
> +
> X509_INFO *inf = sk_X509_INFO_value(pkp->certs, n);
> + X509_NAME *name = X509_get_subject_name(inf->x509);
> + X509_NAME_oneline(name, cert_cn, sizeof(cert_cn));
> X509_STORE_CTX_init(sctx, store, inf->x509, NULL);
> - X509_verify_cert(sctx);
> - ERR_clear_error();
>
> + res=X509_verify_cert(sctx);
Style violation.
> chain = X509_STORE_CTX_get1_chain(sctx);
> - sk_X509_shift(chain);
> +
> + if (res == 1) {
> + /* Removing the client cert if verification is OK
> + * could save a loop when choosing which cert to send
> + * when more than one is available */
> + /* XXX: This is not needed if we collapse the two
> + * checks in ssl_engine_kernel in the future */
> + X509_free(sk_X509_shift(chain));
> + }
> + else {
> + int n=X509_STORE_CTX_get_error(sctx);
Overwriting a symbol from the loop is IMHO bad and makes code hard to read.
Please use
another name instead of n. Besides we have a style violation here again.
> + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
> + "SSL proxy client cert chain verification failed
> for %s: %s",
> + cert_cn, X509_verify_cert_error_string(n));
> + }
> + ERR_clear_error();
> i=sk_X509_num(chain);
> pkp->ca_certs[n] = chain;
> +
> + if (i == 0 || (res != 1 && i == 1) ) {
> + /* zero or only the client cert won't be very useful
> + * due to verification failure */
> + sk_X509_pop_free(chain, X509_free);
> + i = 0;
> + pkp->ca_certs[n] = NULL;
> + }
> +
> X509_STORE_CTX_cleanup(sctx);
>
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
> - "client certificate %i has loaded %i "
> - "intermediate CA%s", n, i, i == 1 ? "" : "s");
> + "loaded %i intermediate CA%s for cert %i (%s)",
> + i, i == 1 ? "" : "s", n, cert_cn);
> + if (i > 0) {
> + int j;
> + for (j=0; j<i; j++) {
> + char ca_cn[256];
> + X509_NAME *ca_name =
> X509_get_subject_name(sk_X509_value(chain, j));
> + X509_NAME_oneline(ca_name, ca_cn, sizeof(ca_cn));
> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "%i: %s", j,
> ca_cn);
> + }
> + }
> }
>
> X509_STORE_CTX_free(sctx);
>
>
>
Regards
Rüdiger
