On Mon, Jun 18, 2008 at 05:27:17PM +0200, I wrote:
> So, to support non-SNI clients "as far as possible", let me propose the
> attached (additional) patch. It corrects the shortcomings of my earlier
> attempt (no longer changing dc->nVerify{Client,Depth} in-place), and
> includes the changes to support SSLCipherSuite, SSLHonorCipherOrder,
> SSLCARevocation{File,Path} and
> SSLOCSP{Enable,DefaultResponder,OverrideResponder} for non-SNI clients.
It turns out that I introduced a regression for SNI clients with this
patch, unfortunately... sorry about that, mea culpa. While the changes
to ssl_callback_SSLVerify and ssl_callback_SSLVerify_CRL in that version
of the patch do the right thing for non-SNI clients, the code will
segfault when trying to verify the peer cert of an SNI client.
The reason is simple - for SNI clients, no request_rec has been assigned
at that stage (it doesn't exist yet), so accessing r->server attempts to
dereference the NULL pointer.
The attached version (v5) fixes this issue, and an interdiff to v4 is
also included. Would it be feasible/acceptable to commit this to trunk?
Thanks,
Kaspar
Index: httpd-trunk/modules/ssl/ssl_engine_kernel.c
===================================================================
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (revision 671932)
+++ httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
@@ -327,32 +327,35 @@ int ssl_hook_Access(request_rec *r)
* because it's the servers choice to select a cipher from the ones the
* client supports. So as long as the current cipher is still in the new
* cipher suite we're happy. Because we can assume we would have
* selected it again even when other (better) ciphers exists now in the
* new cipher suite. This approach is fine because the user explicitly
* 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) {
cipher = SSL_get_current_cipher(ssl);
}
else {
cipher_list_old = (STACK_OF(SSL_CIPHER) *)SSL_get_ciphers(ssl);
if (cipher_list_old) {
cipher_list_old = sk_SSL_CIPHER_dup(cipher_list_old);
}
}
/* configure new state */
- if (!modssl_set_cipher_list(ssl, dc->szCipherSuite)) {
+ 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))) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
"Unable to reconfigure (per-directory) "
"permitted SSL ciphers");
ssl_log_ssl_error(APLOG_MARK, APLOG_ERR, r->server);
if (cipher_list_old) {
sk_SSL_CIPHER_free(cipher_list_old);
}
@@ -408,18 +411,23 @@ int ssl_hook_Access(request_rec *r)
}
}
/* cleanup */
if (cipher_list_old) {
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");
}
}
/*
* override of SSLVerifyDepth
*
@@ -427,29 +435,26 @@ 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
* 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;
- }
- if (dc->nVerifyDepth != UNSET) {
+ if ((dc->nVerifyDepth != UNSET) ||
+ (sc->server->auth.verify_depth != 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) {
+ if ((dc->nVerifyDepth < n) ||
+ (sc->server->auth.verify_depth < n)) {
renegotiate = TRUE;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Reduced client verification depth will force "
"renegotiation");
}
}
/*
@@ -461,33 +466,32 @@ int ssl_hook_Access(request_rec *r)
* The order is: none << optional_no_ca << optional << require
*
* Additionally the following optimization is possible here: When the
* currently active verify type is "none" but a client certificate is
* already known/present, it's enough to manually force a client
* 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;
}
modssl_set_verify(ssl, verify, ssl_callback_SSLVerify);
SSL_set_verify_result(ssl, X509_V_OK);
/* determine whether we've to force a renegotiation */
@@ -574,16 +578,50 @@ int ssl_hook_Access(request_rec *r)
SSL_set_client_CA_list(ssl, ca_list);
renegotiate = TRUE;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Changed client verification locations will force "
"renegotiation");
}
+#else
+#ifndef OPENSSL_NO_TLSEXT
+#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 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
+ * has been set to r->connection->base_server). 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).
+ */
+ 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 */
#endif /* HAVE_SSL_SET_CERT_STORE */
/* 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
* streaming the request body over the wire already. In that
* case, it is not possible to stop and perform a new SSL
* handshake immediately; once the SSL library moves to the
@@ -749,18 +787,20 @@ int ssl_hook_Access(request_rec *r)
}
sslconn->client_cert = cert;
sslconn->client_dn = NULL;
}
/*
* 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,
"Re-negotiation handshake failed: "
"Client verification failed");
return HTTP_FORBIDDEN;
}
@@ -1266,18 +1306,18 @@ DH *ssl_callback_TmpDH(SSL *ssl, int exp
* does client authentication and verifies the certificate chain.
*/
int ssl_callback_SSLVerify(int ok, X509_STORE_CTX *ctx)
{
/* Get Apache context back through OpenSSL context */
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 = conn->base_server;
request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+ server_rec *s = r ? r->server : conn->base_server;
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLDirConfigRec *dc = r ? myDirConfig(r) : NULL;
SSLConnRec *sslconn = myConnConfig(conn);
modssl_ctx_t *mctx = myCtxConfig(sslconn, sc);
/* Get verify ingredients */
int errnum = X509_STORE_CTX_get_error(ctx);
@@ -1397,17 +1437,20 @@ int ssl_callback_SSLVerify(int ok, X509_
/*
* And finally signal OpenSSL the (perhaps changed) state
*/
return ok;
}
int ssl_callback_SSLVerify_CRL(int ok, X509_STORE_CTX *ctx, conn_rec *c)
{
- server_rec *s = c->base_server;
+ 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 : c->base_server;
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLConnRec *sslconn = myConnConfig(c);
modssl_ctx_t *mctx = myCtxConfig(sslconn, sc);
X509_OBJECT obj;
X509_NAME *subject, *issuer;
X509 *cert;
X509_CRL *crl;
EVP_PKEY *pubkey;
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)
@@ -1312,7 +1312,7 @@
SSL_get_ex_data_X509_STORE_CTX_idx());
conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
- server_rec *s = r->server;
+ server_rec *s = r ? r->server : conn->base_server;
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLDirConfigRec *dc = r ? myDirConfig(r) : NULL;
@@ -1445,7 +1445,7 @@
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->server;
+ server_rec *s = r ? r->server : c->base_server;
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLConnRec *sslconn = myConnConfig(c);
modssl_ctx_t *mctx = myCtxConfig(sslconn, sc);
