You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by dr...@apache.org on 2011/09/17 18:25:17 UTC
svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
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);
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);
+ 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);
RE: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Kaspar Brand
> Sent: Freitag, 23. September 2011 17:07
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1172010 -
> /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
>
> Maybe I'm somewhat confused by what "Apache Group" is
> actually referring
> to here - I read that to be the PMC... but I'll gladly stand
> corrected.
> Can someone clarify?
It is correct that only PMC members have binding votes. But this is only critical
for releases. For backports any committer can vote and I have never seen a vote
on a backport to be discarded just because the voter was not on the PMC.
Even on releases you can and should vote (even non comitters can do this),
but finally the vote needs 3 +1's from PMC members to pass.
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/26/2011 2:12 AM, Kaspar Brand wrote:
>
> Go ahead, I'll add my (nonbinding) vote afterwards :-)
>
> Just one (hopefully last) thing I overlooked before: the
> "ssl_log_ssl_error(SSLLOG_MARK, APLOG_EMERG, s);" line before the
> ssl_die() call apparently got lost somewhere on its way to the tree -
> could you re-add that? It makes sure we also capture the OpenSSL error
> in the log, before aborting.
>
> Kaspar
All set - the suggestions you made have been added and the results
committed to trunk. STATUS and the 2.2 patch have been updated as well.
Thanks again - cheers!
--
Daniel Ruggeri
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Kaspar Brand <ht...@velox.ch>.
On 25.9.11 18:54, Daniel Ruggeri wrote:
> On 9/23/2011 10:07 AM, Kaspar Brand wrote:
> Alternatively, we could adjust the callback and init functions to always
> build a chain (even if SSLProxyMachineCertificateChainFile is not set)
> and check "by chain" by doing the X509_NAME_cmp for each item in the
> STACK_OF(X509) in pkp->ca_certs rather than checking the issuer of each
> item in pkp->certs. If the new directive is not set, everything would
> *essentially* function the same way. To me, they are two ways to do the
> same thing, though with the current approach, the verification messages
> in startup will not show up unless using the new directive.
I'm fine with leaving it as is, i.e. having the EE certs in pkp->certs
and the issuer certs in pkp->ca_certs - storing the EE cert twice
would appear rather clumsy to me. I therefore suggest dropping the XXX
comment, and perhaps add a few words to the definition of the "certs"
and "ca_certs" members of the modssl_pk_proxy_t struct in ssl_private.h.
>>> pkp->ca_certs[n] = NULL;
>> Strictly speaking, the last assignment isn't necessary, since your
>> calloc'ing ca_certs before.
>
> Setting to NULL will be caught by the update Rüdiger put in for 1162103
> and will skip all of the new logic in the callback function. IMO, I feel
> this way is just a bit cleaner and easier to follow. I can be swayed if
> you feel strongly about it, though.
What I meant is that calloc() already sets "all bits zero", by
definition. However, as I learned in the meantime, there are indeed
machines where the memory representation of NULL is non-null
(http://c-faq.com/null/machexamp.html), so I have been convinced.
> I'll wait for feedback before committing and updating 2.2 STATUS.
Go ahead, I'll add my (nonbinding) vote afterwards :-)
Just one (hopefully last) thing I overlooked before: the
"ssl_log_ssl_error(SSLLOG_MARK, APLOG_EMERG, s);" line before the
ssl_die() call apparently got lost somewhere on its way to the tree -
could you re-add that? It makes sure we also capture the OpenSSL error
in the log, before aborting.
Kaspar
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 9/23/2011 10:07 AM, Kaspar Brand wrote:
> On 22.09.2011 22:25, Daniel Ruggeri wrote:
>> trunk suggestion - if this jives, I'll commit later when I have a bit
> Looks good, just some nits:
>
>> for (n = 0; n < ncerts; n++) {
>> int i, res;
> res is no longer used, AFAICT
Correct - removed
>> if (chain != NULL) {
>> /* Dicard end entity cert from the chain */
>> /* XXX: This is not needed if we collapse the two
>> * checks in ssl_engine_kernel in the future */
>> X509_free(sk_X509_shift(chain));
> s/Di/Dis/. As for the XXX, do you mean the idea of having a common
> routine for checking server certs and proxy client certs? That would
> probably go to ssl_engine_init.c as well, as sort of a companion to
> ssl_check_public_cert().
In the proxy client cert callback function in ssl_engine_kernel, each
cert is first checked if it is directly signed by each of the CA's in
the list. If that fails, then we start trying to match by chain. The
comment I added just points out that if we leave the end cert in the
STACK_OF(X509) we will perform the same check twice - once for the
direct issuer check and once again for the first item in the chain
without shifting it off.
Alternatively, we could adjust the callback and init functions to always
build a chain (even if SSLProxyMachineCertificateChainFile is not set)
and check "by chain" by doing the X509_NAME_cmp for each item in the
STACK_OF(X509) in pkp->ca_certs rather than checking the issuer of each
item in pkp->certs. If the new directive is not set, everything would
*essentially* function the same way. To me, they are two ways to do the
same thing, though with the current approach, the verification messages
in startup will not show up unless using the new directive.
... I'm not sure if I explained my thought process well, though, so let
me know if I should elaborate further.
>> else {
>> /* Discard empty chain */
>> sk_X509_pop_free(chain, X509_free);
>> pkp->ca_certs[n] = NULL;
> Strictly speaking, the last assignment isn't necessary, since your
> calloc'ing ca_certs before.
Setting to NULL will be caught by the update Rüdiger put in for 1162103
and will skip all of the new logic in the callback function. IMO, I feel
this way is just a bit cleaner and easier to follow. I can be swayed if
you feel strongly about it, though.
> Style - missing spaces. Kaspar
I'm so bad about this. Corrected also. Thank you very much for
reviewing. I'll wait for feedback before committing and updating 2.2 STATUS.
--
Daniel Ruggeri
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Kaspar Brand <ht...@velox.ch>.
On 22.09.2011 22:25, Daniel Ruggeri wrote:
> On 9/22/2011 5:39 AM, Kaspar Brand wrote:
>> Having it in one patch seems fine to me, but in the end, it's the
>> PMC members who will vote on backport proposals (IIUC), so it's
>> their opinion which really matters.
>
> IINM, I believe we as committers all have a vote... that said, I hope
> you would drop a +1 in the 2.2 STATUS file after the dust settles on
> this change :-)
Hmm, I thought I wasn't supposed to cast votes on that, buy maybe I'm
misinterpreting the guidelines
(http://httpd.apache.org/dev/guidelines.html):
> However, the only binding votes are those cast by active members of
> the Apache Group; if the vote is about a change to source code or
> documentation, the primary author of what is being changed may also
> cast a binding vote on that issue.
Maybe I'm somewhat confused by what "Apache Group" is actually referring
to here - I read that to be the PMC... but I'll gladly stand corrected.
Can someone clarify?
> trunk suggestion - if this jives, I'll commit later when I have a bit
Looks good, just some nits:
> for (n = 0; n < ncerts; n++) {
> int i, res;
res is no longer used, AFAICT
> if (chain != NULL) {
> /* Dicard end entity cert from the chain */
> /* XXX: This is not needed if we collapse the two
> * checks in ssl_engine_kernel in the future */
> X509_free(sk_X509_shift(chain));
s/Di/Dis/. As for the XXX, do you mean the idea of having a common
routine for checking server certs and proxy client certs? That would
probably go to ssl_engine_init.c as well, as sort of a companion to
ssl_check_public_cert().
> else {
> /* Discard empty chain */
> sk_X509_pop_free(chain, X509_free);
> pkp->ca_certs[n] = NULL;
Strictly speaking, the last assignment isn't necessary, since your
calloc'ing ca_certs before.
> if (i > 0) {
> int j;
> for (j=0; j<i; j++) {
Style - missing spaces.
Kaspar
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 9/22/2011 5:39 AM, Kaspar Brand wrote:
> Having it in one patch seems fine to me, but in the end, it's the
> PMC members who will vote on backport proposals (IIUC), so it's
> their opinion which really matters.
IINM, I believe we as committers all have a vote... that said, I hope
you would drop a +1 in the 2.2 STATUS file after the dust settles on
this change :-)
> For trunk, I would prefer ssl_log_xerror being used, like so:
>
> ssl_log_xerror(SSLLOG_MARK, APLOG_WARNING, 0, ptemp, s, inf->x509,
> "SSL proxy client cert chain verification failed: %s:",
>
X509_verify_cert_error_string(X509_STORE_CTX_get_error(sctx)));
>
> Only having the subject DN in the log message sometimes makes it
> difficult to unambiguously identify the cert (think e.g. of the
> situation when replacing a previously used cert with a freshly
> issued one, where both have exactly the same subject DN).
Absolutely. I see it is quite a bit more verbose, but definitely
provides "enough" information.
> For reasons of readability, I would reorder the OpenSSL calls
> after X509_verify_cert() somewhat:
> ...
>
> Note that X509_STORE_CTX_get1_chain() may also return NULL
> (not under normal circumstances, but still). For reasons of
> robustness, we should fail gracefully in this case.
>
> And for the logging statements, I would again prefer ssl_log_xerror
> being used (especially since it's APLOG_DEBUG level).
>
Sure... no problem - updates per feedback provided below. I can't think
what action we *could* take if the chain comes back NULL, so I wrapped
the remaining logic within a check that it is not.
Since this is where the trunk and 2.2 patches differ, feel free to have
a look at the 2.2. patch here:
http://people.apache.org/~druggeri/patches/httpd-2.2-SSLProxyMachineCertificateChainFile.patch
The only difference worth noting is the usage of the new logging routine
you provided in trunk, but not 2.2.
trunk suggestion - if this jives, I'll commit later when I have a bit
if (!pkp->ca_cert_file || !store) {
return;
}
/* Load all of the CA certs and construct a chain */
pkp->ca_certs = (STACK_OF(X509) **) apr_pcalloc(p, ncerts * sizeof(sk));
sctx = X509_STORE_CTX_new();
if (!sctx) {
ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s,
"SSL proxy client cert initialization failed");
ssl_die();
}
X509_STORE_load_locations(store, pkp->ca_cert_file, NULL);
for (n = 0; n < ncerts; n++) {
int i, res;
X509_INFO *inf = sk_X509_INFO_value(pkp->certs, n);
X509_STORE_CTX_init(sctx, store, inf->x509, NULL);
/* Attempt to verify the client cert */
if (X509_verify_cert(sctx) != 1) {
int err = X509_STORE_CTX_get_error(sctx);
ssl_log_xerror(SSLLOG_MARK, APLOG_WARNING, 0, ptemp, s,
inf->x509,
"SSL proxy client cert chain verification
failed: %s :",
X509_verify_cert_error_string(err));
}
/* Clear X509_verify_cert errors */
ERR_clear_error();
/* Obtain a copy of the verified chain */
chain = X509_STORE_CTX_get1_chain(sctx);
if (chain != NULL) {
/* Dicard end entity cert from the chain */
/* XXX: This is not needed if we collapse the two
* checks in ssl_engine_kernel in the future */
X509_free(sk_X509_shift(chain));
if ((i = sk_X509_num(chain)) > 0) {
/* Store the chain for later use */
pkp->ca_certs[n] = chain;
}
else {
/* Discard empty chain */
sk_X509_pop_free(chain, X509_free);
pkp->ca_certs[n] = NULL;
}
ssl_log_xerror(SSLLOG_MARK, APLOG_DEBUG, 0, ptemp, s, inf->x509,
"loaded %i intermediate CA%s for cert %i: ",
i, i == 1 ? "" : "s", n);
if (i > 0) {
int j;
for (j=0; j<i; j++) {
ssl_log_xerror(SSLLOG_MARK, APLOG_DEBUG, 0, ptemp, s,
sk_X509_value(chain, j), "%i:", j);
}
}
}
/* get ready for next X509_STORE_CTX_init */
X509_STORE_CTX_cleanup(sctx);
}
X509_STORE_CTX_free(sctx);
--
Daniel Ruggeri
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Kaspar Brand <ht...@velox.ch>.
On 21.09.2011 19:40, Daniel Ruggeri wrote:
> Also - any opposition to including SSL_X509_NAME_to_string as part of
> the backport proposal? I would like to keep the patches consistent. If
> not, would you prefer me to roll it into the
> SSLProxyMachineCertificateChainFile patch or propose it separately?
Having it in one patch seems fine to me, but in the end, it's the
PMC members who will vote on backport proposals (IIUC), so it's
their opinion which really matters.
> if (X509_verify_cert(sctx) != 1) {
> int err = X509_STORE_CTX_get_error(sctx);
> ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
> "SSL proxy client cert chain verification
> failed for %s: %s",
> cert_dn, X509_verify_cert_error_string(err));
> }
For trunk, I would prefer ssl_log_xerror being used, like so:
ssl_log_xerror(SSLLOG_MARK, APLOG_WARNING, 0, ptemp, s, inf->x509,
"SSL proxy client cert chain verification failed: %s:",
X509_verify_cert_error_string(X509_STORE_CTX_get_error(sctx)));
Only having the subject DN in the log message sometimes makes it
difficult to unambiguously identify the cert (think e.g. of the
situation when replacing a previously used cert with a freshly
issued one, where both have exactly the same subject DN).
> chain = X509_STORE_CTX_get1_chain(sctx);
>
> /* 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));
>
> i = sk_X509_num(chain);
> pkp->ca_certs[n] = chain;
>
> if (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,
> "loaded %i intermediate CA%s for cert %i (%s)",
> i, i == 1 ? "" : "s", n, cert_dn);
> if (i > 0) {
> int j;
> for (j=0; j<i; j++) {
> X509_NAME *ca_name =
> X509_get_subject_name(sk_X509_value(chain, j));
> char *ca_dn = SSL_X509_NAME_to_string(ptemp, ca_name, 0);
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "%i: %s", j,
> ca_dn);
> }
> }
> }
For reasons of readability, I would reorder the OpenSSL calls
after X509_verify_cert() somewhat:
/* clear X509_verify_cert errors */
ERR_clear_error();
/* grab a copy of the chain */
chain = X509_STORE_CTX_get1_chain(sctx);
/* remove the EE cert from the chain */
X509_free(sk_X509_shift(chain));
/* deal with what is left now */
if ((i = sk_X509_num(chain)) > 0) {
/* remember the certs in pkp->ca_certs */
}
else {
/* discard "chain" */
}
/* get ready for next X509_STORE_CTX_init */
X509_STORE_CTX_cleanup(sctx);
Note that X509_STORE_CTX_get1_chain() may also return NULL
(not under normal circumstances, but still). For reasons of
robustness, we should fail gracefully in this case.
And for the logging statements, I would again prefer ssl_log_xerror
being used (especially since it's APLOG_DEBUG level).
Kaspar
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 9/19/2011 3:28 PM, Kaspar Brand wrote:
> IMO, you can always drop the first element of the chain, since you only
> want to remember CA certs in pkp->ca_certs.
>
OK, cool - I was unsure if the chain would ALWAYS contain the cert in
cases of validation OK or error. I'll make this quick update.
>> + else {
>> + int n=X509_STORE_CTX_get_error(sctx);
>> + 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));
>> + }
> Here, cert_cn holds the X509_NAME_oneline() string of the subject DN.
> Either the variable name is a misnomer or a typo (did you mean cert_dn
> instead of cert_cn?)
Yes, indeed. s/_cn/_dn/g
>
> I have just added ssl_log_xerror() and SSL_X509_NAME_to_string() in
> r1172797, can you adapt the code in ssl_callback_proxy_cert() to make
> use of these where applicable/possible? Hopefully this makes logging
> cert details in mod_ssl more straightforward.
>
> Kaspar
Sure - no problem. Have a look at the update below. If this jives, I'll
commit to trunk when I have a minute to do so.
Also - any opposition to including SSL_X509_NAME_to_string as part of
the backport proposal? I would like to keep the patches consistent. If
not, would you prefer me to roll it into the
SSLProxyMachineCertificateChainFile patch or propose it separately?
Update...
X509_STORE_load_locations(store, pkp->ca_cert_file, NULL);
for (n = 0; n < ncerts; n++) {
int i, res;
X509_INFO *inf = sk_X509_INFO_value(pkp->certs, n);
X509_NAME *name = X509_get_subject_name(inf->x509);
char *cert_dn = SSL_X509_NAME_to_string(ptemp, name, 0);
X509_STORE_CTX_init(sctx, store, inf->x509, NULL);
if (X509_verify_cert(sctx) != 1) {
int err = X509_STORE_CTX_get_error(sctx);
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
"SSL proxy client cert chain verification
failed for %s: %s",
cert_dn, X509_verify_cert_error_string(err));
}
chain = X509_STORE_CTX_get1_chain(sctx);
/* 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));
ERR_clear_error();
i = sk_X509_num(chain);
pkp->ca_certs[n] = chain;
if (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,
"loaded %i intermediate CA%s for cert %i (%s)",
i, i == 1 ? "" : "s", n, cert_dn);
if (i > 0) {
int j;
for (j=0; j<i; j++) {
X509_NAME *ca_name =
X509_get_subject_name(sk_X509_value(chain, j));
char *ca_dn = SSL_X509_NAME_to_string(ptemp, ca_name, 0);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "%i: %s", j,
ca_dn);
}
}
}
X509_STORE_CTX_free(sctx);
--
Daniel Ruggeri
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Kaspar Brand <ht...@velox.ch>.
On 17.09.2011 18:25, druggeri@apache.org wrote:
> + 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));
IMO, you can always drop the first element of the chain, since you only
want to remember CA certs in pkp->ca_certs.
> + else {
> + int n=X509_STORE_CTX_get_error(sctx);
> + 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));
> + }
Here, cert_cn holds the X509_NAME_oneline() string of the subject DN.
Either the variable name is a misnomer or a typo (did you mean cert_dn
instead of cert_cn?), but more importantly, we should not add new code
which still calls X509_NAME_oneline(), at least for trunk... as its
OpenSSL man page states: its use "is strongly discouraged in new
applications".
I have just added ssl_log_xerror() and SSL_X509_NAME_to_string() in
r1172797, can you adapt the code in ssl_callback_proxy_cert() to make
use of these where applicable/possible? Hopefully this makes logging
cert details in mod_ssl more straightforward.
Kaspar
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
Re: svn commit: r1172010 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
Posted by Ruediger Pluem <rp...@apache.org>.
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