On 9/19/2011 12:55 AM, Ruediger Pluem wrote: > 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 >
Thank you. Fixed in r1172562. -- Daniel Ruggeri
