httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Date Mon, 19 Sep 2011 05:55:30 GMT


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


Mime
View raw message