You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2011/09/19 07:55:30 UTC
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
On 09/17/2011 06:25 PM, druggeri@apache.org 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
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 9/19/2011 12:55 AM, Ruediger Pluem wrote:
> On 09/17/2011 06:25 PM, druggeri@apache.org 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