On 04/08/2009 10:17 AM, Kaspar Brand wrote:
> 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?
Correct.
@@ -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
*
I don't really understand this part of the patch. If the default host has
a verify depth of lets say 10 and the virtual host with the correct name has 1
then IMHO a renegotiation should happen but the code above does not seem to do
this.
·
+#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 */
+
I don't get why the above code is only needed in the case that Openssl is SNI
capable. IMHO this check is always needed or better regardless of SNI or not SNI
only needed
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
Otherwise a compiler warning about verify might be used uninitialized tells you
that something is wrong with the code anyway.
The only thing that needs an ifndef above is
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
which is always true in the non SNI case.
One other thing: r->connection->base_server is IMHO always wrong in the patch
and should be replaced with sslconn->server
Regards
Rüdiger
