Plüm, Rüdiger, VF-Group wrote:
> I reviewed your patch and found some issues with it.
Thanks for your review and testing, Rüdiger. I assume you used and
adapted version of the sni_sslverifyclient-v5.diff patch, is that correct?
> (All cases below use Name based virtual hosting and a non SNI client):
>
> 1. Renegotiation due to more restrictive cipher requirements:
>
> Lets say the first virtual host allows cipher A and B.
> The handshake with the client decided to use A.
> The virtual host the client switches to later also allows A and B.
> But /restricted in this host only allows B.
> In this case a request to /restricted does not cause a renegotiation
> but it should.
Right. It also applies to SNI clients, actually, and the problem is that
the logic of this code (added in sni_sslverifyclient-v4.diff) is flawed:
if ((dc->szCipherSuite &&
!modssl_set_cipher_list(ssl, dc->szCipherSuite)) ||
(sc->server->auth.cipher_suite &&
!modssl_set_cipher_list(ssl, sc->server->auth.cipher_suite))) {
- it will override the per-dir setting for the cipher suite with that
from the vhost level, if the latter is also set. Changing these lines to
if ((dc->szCipherSuite || sc->server->auth.cipher_suite) &&
!modssl_set_cipher_list(ssl, dc->szCipherSuite ?
dc->szCipherSuite :
sc->server->auth.cipher_suite)) {
resolves this issue for me.
> 2. The verification depth check causes unneeded renegotiations which
> break the ssl v2 tests in the perl framework (No dicussion here please
> whether we should still support SSL v2 :-))
This is an issue I already addressed in the patch for 2.2.x
(http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch), but I guess you
were testing a trunk version without these changes, is that correct?
> There might be further issues but I currently have no time to check.
>
> I think we both agree that without this patch from you name based virtual
> hosting with SSL is definitely unsafe.
> I haven't analyzed any further if the above issues are fixable or not
> and I admit that I currently have no resources to do so.
I'm attaching a new patch (against r763127, i.e. current trunk), which
addresses both issues. Would very much appreciate if you could have a
look at it / give it a try, as it would definitely improve the situation
regarding SNI support in mod_ssl.
Kaspar
Index: httpd-trunk/modules/ssl/ssl_engine_kernel.c
===================================================================
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (revision 763127)
+++ 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);
@@ -362,7 +352,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 != r->connection->base_server)) {
/* remember old state */
if (dc->nOptions & SSL_OPT_OPTRENEGOTIATE) {
@@ -377,7 +367,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 +436,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");
}
@@ -462,26 +460,17 @@ int ssl_hook_Access(request_rec *r)
* 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 = (dc->nVerifyDepth != UNSET) ?
+ dc->nVerifyDepth : sc->server->auth.verify_depth;
+ if ((sslconn->verify_depth < n) ||
+ ((n == 0) && (sc->server->auth.verify_depth == 0))) {
+ 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 +485,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;
}
@@ -550,6 +538,39 @@ int ssl_hook_Access(request_rec *r)
}
}
+#ifndef OPENSSL_NO_TLSEXT
+ /* 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 (SSL_set_cert_store() would be required
+ * for this).
+ */
+#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 ((r->server != r->connection->base_server) &&
+ (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) &&
+ renegotiate &&
+ !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
+ SSLSrvConfigRec *bssc = mySrvConfig(r->connection->base_server);
+
+ 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;
+ }
+ }
+#endif /* OPENSSL_NO_TLSEXT */
+
/* If a renegotiation is now required for this location, and the
* request includes a message body (and the client has not
* requested a "100 Continue" response), then the client will be
@@ -732,8 +753,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 +1218,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 +1349,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);
