Ruediger Pluem wrote:
> Looks good. Some minor nitpicks:
>
> Reviewing the code again I don't think we need to have
>
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
>
> this condition at all.
Agreed. I have removed this part from the if expression.
> 2. The whole
>
> + if ((r->server != sslconn->server)
> + && renegotiate
> + && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
> + )
>
> can move inside the if block above (
>
> + if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> + (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
>
> )
> because only if get inside this block it can happen that
> verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT
> gets true. So we safe some checks in the case that
> ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET))
>
> is not true that will always fail and thus waste cycles even more
> so as there are many SSL configurations outside that do not use
> client certs at all.
> This also means we can move the initialization of verify back
> into the if block.
Correct, changed accordingly.
> Furthermore I think that we need to check for CA list change in any case
> that we need to renegotiate as even if we do not fail on SSL level due
> to SSL_VERIFY_FAIL_IF_NO_PEER_CERT there could be other conditions later
> on in the configuration (sslrequire / rewriterules) that check if we had been
> successful with our check on SSL level to e.g. provide a nice error page
> if we were not. So if we have changed the CA list we might signal success
> to these downstream configuration options although there wasn't one because
> we used the wrong CA list.
Here I'm not sure I'm really following you / understanding your point.
The case I was primarily thinking of is something like
<VirtualHost foo.example.com:443>
SSLVerifyClient none # (the default, anyway)
</VirtualHost>
<VirtualHost bar.example.com:443>
SSLVerifyClient optional
SSLCACertificateFile bar-clientauth-bundle.pem
</VirtualHost>
In this situation, if a non-SNI client requests content from
bar.example.com, the "renegotiate" variable will get set, but since
"verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT" is not true, we will
currently let it proceed. Are you proposing to return HTTP_FORBIDDEN
immediately after the MODSSL_CFG_CA_NE checks fail, instead (i.e., even
if SSLVerifyClient is optional)? My idea when writing that code was that
unless SSLVerifyClient is set to "require", we should not stop non-SNI
clients here - the evaluation of a possible SSLRequire configuration
directive at the end of ssl_hook_Access can still return HTTP_FORBIDDEN,
if really needed (OTOH, why exactly would the admin choose "optional",
then?). But maybe I'm simply missing your real point.
What I had to do anyway, however, is setting r->connection->aborted
before returning HTTP_FORBIDDEN... otherwise we run into a problem with
keep-alive requests, as additional testing has shown: if we don't drop
the connection at the same time (by setting r->connection->aborted),
then verify_old keeps its value from the previous request, and
"renegotiate" may therefore not be set again when processing the next
request (if verify == verify_old). Using r->connection->aborted for
closing the connection immediately is also used in code further down in
ssl_hook_Access (when a renegotiation doesn't have the expected outcome)
- i.e., we're not introducing new behavior by using this "technique".
> 3. I created a var handshakeserver to avoid the dereferencing of
> sslconn->server
> over and over again but this might be only a minor issue.
Fine with me - let me know if v8 includes the changes you had in mind.
Kaspar
Index: httpd-trunk/modules/ssl/ssl_engine_kernel.c
===================================================================
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (revision 765079)
+++ httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
@@ -186,16 +186,6 @@ int ssl_hook_ReadReq(request_rec *r)
return HTTP_BAD_REQUEST;
}
}
- else if (r->connection->vhost_lookup_data) {
- /*
- * We are using a name based configuration here, but no hostname was
- * provided via SNI. Don't allow that.
- */
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
- "No hostname was provided via SNI for a name based"
- " virtual host");
- return HTTP_FORBIDDEN;
- }
#endif
SSL_set_app_data2(ssl, r);
@@ -265,10 +255,11 @@ static void ssl_configure_env(request_rec *r, SSLC
*/
int ssl_hook_Access(request_rec *r)
{
- SSLDirConfigRec *dc = myDirConfig(r);
- SSLSrvConfigRec *sc = mySrvConfig(r->server);
- SSLConnRec *sslconn = myConnConfig(r->connection);
- SSL *ssl = sslconn ? sslconn->ssl : NULL;
+ SSLDirConfigRec *dc = myDirConfig(r);
+ SSLSrvConfigRec *sc = mySrvConfig(r->server);
+ SSLConnRec *sslconn = myConnConfig(r->connection);
+ SSL *ssl = sslconn ? sslconn->ssl : NULL;
+ server_rec *handshakeserver = sslconn ? sslconn->server : NULL;
SSL_CTX *ctx = NULL;
apr_array_header_t *requires;
ssl_require_t *ssl_requires;
@@ -362,7 +353,7 @@ int ssl_hook_Access(request_rec *r)
* has to enable this via ``SSLOptions +OptRenegotiate''. So we do no
* implicit optimizations.
*/
- if (dc->szCipherSuite) {
+ if (dc->szCipherSuite || (r->server != handshakeserver)) {
/* remember old state */
if (dc->nOptions & SSL_OPT_OPTRENEGOTIATE) {
@@ -377,7 +368,10 @@ int ssl_hook_Access(request_rec *r)
}
/* configure new state */
- if (!modssl_set_cipher_list(ssl, dc->szCipherSuite)) {
+ if ((dc->szCipherSuite || sc->server->auth.cipher_suite) &&
+ !modssl_set_cipher_list(ssl, dc->szCipherSuite ?
+ dc->szCipherSuite :
+ sc->server->auth.cipher_suite)) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
"Unable to reconfigure (per-directory) "
"permitted SSL ciphers");
@@ -443,8 +437,13 @@ int ssl_hook_Access(request_rec *r)
sk_SSL_CIPHER_free(cipher_list_old);
}
- /* tracing */
if (renegotiate) {
+#ifdef SSL_OP_CIPHER_SERVER_PREFERENCE
+ if (sc->cipher_server_pref == TRUE) {
+ SSL_set_options(ssl, SSL_OP_CIPHER_SERVER_PREFERENCE);
+ }
+#endif
+ /* tracing */
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Reconfigured cipher suite will force renegotiation");
}
@@ -457,31 +456,24 @@ int ssl_hook_Access(request_rec *r)
* function and not by OpenSSL internally (and our function is aware of
* both the per-server and per-directory contexts). So we cannot ask
* OpenSSL about the currently verify depth. Instead we remember it in our
- * ap_ctx attached to the SSL* of OpenSSL. We've to force the
+ * SSLConnRec attached to the SSL* of OpenSSL. We've to force the
* renegotiation if the reconfigured/new verify depth is less than the
* currently active/remembered verify depth (because this means more
* restriction on the certificate chain).
*/
- if ((sc->server->auth.verify_depth != UNSET) &&
- (dc->nVerifyDepth == UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyDepth = sc->server->auth.verify_depth;
+ n = sslconn->verify_depth ?
+ sslconn->verify_depth :
+ (mySrvConfig(handshakeserver))->server->auth.verify_depth;
+ /* determine the new depth */
+ sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
+ dc->nVerifyDepth : sc->server->auth.verify_depth;
+ if (sslconn->verify_depth < n) {
+ renegotiate = TRUE;
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Reduced client verification depth will force "
+ "renegotiation");
}
- if (dc->nVerifyDepth != UNSET) {
- /* XXX: doesnt look like sslconn->verify_depth is actually used */
- if (!(n = sslconn->verify_depth)) {
- sslconn->verify_depth = n = sc->server->auth.verify_depth;
- }
- /* determine whether a renegotiation has to be forced */
- if (dc->nVerifyDepth < n) {
- renegotiate = TRUE;
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
- "Reduced client verification depth will force "
- "renegotiation");
- }
- }
-
/*
* override of SSLVerifyClient
*
@@ -496,23 +488,22 @@ int ssl_hook_Access(request_rec *r)
* verification but at least skip the I/O-intensive renegotation
* handshake.
*/
- if ((sc->server->auth.verify_mode != SSL_CVERIFY_UNSET) &&
- (dc->nVerifyClient == SSL_CVERIFY_UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyClient = sc->server->auth.verify_mode;
- }
- if (dc->nVerifyClient != SSL_CVERIFY_UNSET) {
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
/* remember old state */
verify_old = SSL_get_verify_mode(ssl);
/* configure new state */
verify = SSL_VERIFY_NONE;
- if (dc->nVerifyClient == SSL_CVERIFY_REQUIRE) {
+ if ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_REQUIRE)) {
verify |= SSL_VERIFY_PEER_STRICT;
}
if ((dc->nVerifyClient == SSL_CVERIFY_OPTIONAL) ||
- (dc->nVerifyClient == SSL_CVERIFY_OPTIONAL_NO_CA))
+ (dc->nVerifyClient == SSL_CVERIFY_OPTIONAL_NO_CA) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_OPTIONAL) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA))
{
verify |= SSL_VERIFY_PEER;
}
@@ -548,6 +539,38 @@ int ssl_hook_Access(request_rec *r)
renegotiate_quick ? "quick " : "");
}
}
+ /* If we're handling a request for a vhost other than the default one,
+ * then we need to make sure that client authentication is properly
+ * enforced. For clients supplying an SNI extension, the peer
+ * certificate verification has happened in the handshake already
+ * (and r->server == handshakeserver). For non-SNI requests,
+ * an additional check is needed here. If client authentication
+ * is configured as mandatory, then we can only proceed if the
+ * CA list doesn't have to be changed (OpenSSL doesn't provide
+ * an option to change the list for an existing session).
+ */
+ if ((r->server != handshakeserver)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
+ SSLSrvConfigRec *hssc = mySrvConfig(handshakeserver);
+
+#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && \
+ strNE(sc1->server->auth.f, sc2->server->auth.f)))
+
+ if (MODSSL_CFG_CA_NE(ca_cert_file, sc, hssc) ||
+ MODSSL_CFG_CA_NE(ca_cert_path, sc, hssc)) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "Non-default virtual host with SSLVerify set to 'require'
"
+ "and VirtualHost-specific CA certificate list is only "
+ "available to clients with TLS server name indication "
+ "(SNI) support");
+ r->connection->aborted = 1;
+ return HTTP_FORBIDDEN;
+ }
+ }
}
/* If a renegotiation is now required for this location, and the
@@ -732,8 +755,10 @@ int ssl_hook_Access(request_rec *r)
/*
* Finally check for acceptable renegotiation results
*/
- if (dc->nVerifyClient != SSL_CVERIFY_NONE) {
- BOOL do_verify = (dc->nVerifyClient == SSL_CVERIFY_REQUIRE);
+ if ((dc->nVerifyClient != SSL_CVERIFY_NONE) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_NONE)) {
+ BOOL do_verify = ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
+ (sc->server->auth.verify_mode ==
SSL_CVERIFY_REQUIRE));
if (do_verify && (SSL_get_verify_result(ssl) != X509_V_OK)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -1195,8 +1220,8 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
- server_rec *s = mySrvFromConn(conn);
request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+ server_rec *s = r ? r->server : mySrvFromConn(conn);
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLDirConfigRec *dc = r ? myDirConfig(r) : NULL;
@@ -1326,7 +1351,10 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
int ssl_callback_SSLVerify_CRL(int ok, X509_STORE_CTX *ctx, conn_rec *c)
{
- server_rec *s = mySrvFromConn(c);
+ SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
+
SSL_get_ex_data_X509_STORE_CTX_idx());
+ request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+ server_rec *s = r ? r->server : mySrvFromConn(c);
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLConnRec *sslconn = myConnConfig(c);
modssl_ctx_t *mctx = myCtxConfig(sslconn, sc);
diff -u httpd-trunk/modules/ssl/ssl_engine_kernel.c
httpd-trunk/modules/ssl/ssl_engine_kernel.c
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
+++ httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
@@ -255,10 +255,11 @@
*/
int ssl_hook_Access(request_rec *r)
{
- SSLDirConfigRec *dc = myDirConfig(r);
- SSLSrvConfigRec *sc = mySrvConfig(r->server);
- SSLConnRec *sslconn = myConnConfig(r->connection);
- SSL *ssl = sslconn ? sslconn->ssl : NULL;
+ SSLDirConfigRec *dc = myDirConfig(r);
+ SSLSrvConfigRec *sc = mySrvConfig(r->server);
+ SSLConnRec *sslconn = myConnConfig(r->connection);
+ SSL *ssl = sslconn ? sslconn->ssl : NULL;
+ server_rec *handshakeserver = sslconn ? sslconn->server : NULL;
SSL_CTX *ctx = NULL;
apr_array_header_t *requires;
ssl_require_t *ssl_requires;
@@ -271,7 +272,7 @@
X509_STORE_CTX cert_store_ctx;
STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL;
const SSL_CIPHER *cipher = NULL;
- int depth, verify_old, verify = SSL_VERIFY_NONE, n;
+ int depth, verify_old, verify, n;
if (ssl) {
ctx = SSL_get_SSL_CTX(ssl);
@@ -352,7 +353,7 @@
* has to enable this via ``SSLOptions +OptRenegotiate''. So we do no
* implicit optimizations.
*/
- if (dc->szCipherSuite || (r->server != sslconn->server)) {
+ if (dc->szCipherSuite || (r->server != handshakeserver)) {
/* remember old state */
if (dc->nOptions & SSL_OPT_OPTRENEGOTIATE) {
@@ -462,7 +463,7 @@
*/
n = sslconn->verify_depth ?
sslconn->verify_depth :
- (mySrvConfig(sslconn->server))->server->auth.verify_depth;
+ (mySrvConfig(handshakeserver))->server->auth.verify_depth;
/* determine the new depth */
sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
dc->nVerifyDepth : sc->server->auth.verify_depth;
@@ -491,8 +492,9 @@
(sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
/* remember old state */
verify_old = SSL_get_verify_mode(ssl);
-
/* configure new state */
+ verify = SSL_VERIFY_NONE;
+
if ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
(sc->server->auth.verify_mode == SSL_CVERIFY_REQUIRE)) {
verify |= SSL_VERIFY_PEER_STRICT;
@@ -537,40 +539,37 @@
renegotiate_quick ? "quick " : "");
}
}
- }
-
- /* If we're handling a request for a vhost other than the default one,
- * then we need to make sure that client authentication is properly
- * enforced. For clients supplying an SNI extension, the peer certificate
- * verification has happened in the handshake already. For non-SNI
requests,
- * an additional check is needed here. If client authentication is
- * configured as mandatory, then we can only proceed if the CA list
- * doesn't have to be changed (OpenSSL doesn't provide an option
- * to change the list for an existing session).
- */
- if ((r->server != sslconn->server)
- && renegotiate
- && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
-#ifndef OPENSSL_NO_TLSEXT
- && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
-#endif
- ) {
- SSLSrvConfigRec *bssc = mySrvConfig(sslconn->server);
+ /* If we're handling a request for a vhost other than the default one,
+ * then we need to make sure that client authentication is properly
+ * enforced. For clients supplying an SNI extension, the peer
+ * certificate verification has happened in the handshake already
+ * (and r->server == handshakeserver). For non-SNI requests,
+ * an additional check is needed here. If client authentication
+ * is configured as mandatory, then we can only proceed if the
+ * CA list doesn't have to be changed (OpenSSL doesn't provide
+ * an option to change the list for an existing session).
+ */
+ if ((r->server != handshakeserver)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
+ SSLSrvConfigRec *hssc = mySrvConfig(handshakeserver);
#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
- (sc1->server->auth.f && \
- (!sc2->server->auth.f || \
- sc2->server->auth.f && \
- strNE(sc1->server->auth.f, sc2->server->auth.f)))
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && \
+ strNE(sc1->server->auth.f, sc2->server->auth.f)))
- if (MODSSL_CFG_CA_NE(ca_cert_file, sc, bssc) ||
- MODSSL_CFG_CA_NE(ca_cert_path, sc, bssc)) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
- "Non-default virtual host with SSLVerify set to 'require' "
- "and VirtualHost-specific CA certificate list is only "
- "supported for clients with TLS server name indication "
- "(SNI) support");
- return HTTP_FORBIDDEN;
+ if (MODSSL_CFG_CA_NE(ca_cert_file, sc, hssc) ||
+ MODSSL_CFG_CA_NE(ca_cert_path, sc, hssc)) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "Non-default virtual host with SSLVerify set to 'require'
"
+ "and VirtualHost-specific CA certificate list is only "
+ "available to clients with TLS server name indication "
+ "(SNI) support");
+ r->connection->aborted = 1;
+ return HTTP_FORBIDDEN;
+ }
}
}
