Rework SNI client reverse proxy

Add a note to the table to indicate that the handhake is complete
so we don't set the extension every time data is read or written.

Drop NSSHandshakeCallback() as it didn't do anything and is replaced
by the proxy callback.

Extend the checks around calling SetURL to match those in mod_ssl:
  - a hostname is available
  - not SSLv3
  - not an IP address

3 files changed, 43 insertions, 17 deletions

diff --git a/mod_nss.c b/mod_nss.c
index 381e9e7..c58ca47 100644
--- a/mod_nss.c
+++ b/mod_nss.c
@@ -283,7 +283,7 @@ SECStatus NSSBadCertHandler(void *arg, PRFileDesc * socket)
                     if (rv != SECSuccess) {
                         char *remote = CERT_GetCommonName(&peerCert->subject);
                         ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
-                            "SSL Proxy: Possible man-in-the-middle attack. The remove server is %s, we expected %s", remote, hostname_note);
+                            "SSL Proxy: Possible man-in-the-middle attack. The remote server is %s, we expected %s", remote, hostname_note);
                         PORT_Free(remote);
                     }
                 } else {
diff --git a/nss_engine_init.c b/nss_engine_init.c
index 75a0b8b..00d8d8b 100644
--- a/nss_engine_init.c
+++ b/nss_engine_init.c
@@ -1319,7 +1319,6 @@ static void nss_init_server_certs(server_rec *s,
             "Error setting PKCS11 pin argument: '%s'", mctx->nickname);
         nss_die();
     }
-    
     secstatus = (SECStatus)SSL_HandshakeCallback(mctx->model, (SSLHandshakeCallback)NSSHandshakeCallback, NULL);
     if (secstatus != SECSuccess)
     {
@@ -1328,7 +1327,7 @@ static void nss_init_server_certs(server_rec *s,
         nss_log_nss_error(APLOG_MARK, APLOG_ERR, s);
         nss_die();
     }
-
+    
 }
 
 static void nss_init_proxy_ctx(server_rec *s,
diff --git a/nss_engine_io.c b/nss_engine_io.c
index b851b31..5593246 100644
--- a/nss_engine_io.c
+++ b/nss_engine_io.c
@@ -673,31 +673,58 @@ static apr_status_t nss_io_filter_cleanup(void *data)
     return APR_SUCCESS;
 }
 
+/*
+ * This can't be done in a callback because of the way that mod_proxy
+ * handles reuqests. It creates the connection, which for NSS
+ * just generates a socket from the model, then it sets the proxy
+ * hostname, then it writes the request. For NSS the writing of the
+ * request is what generates the handshake but we don't have the
+ * proxy hostname to set the SNI value for yet, so do it here.
+ */
 static apr_status_t nss_io_filter_handshake(ap_filter_t *f)
 {
     conn_rec *c         = f->c;
     SSLConnRec *sslconn = myConnConfig(c);
+    SECStatus rv;
 
     /*
-     * Enable SNI for backend requests. Make sure we don't do it for
-     * pure SSLv3 connections
+     * Enable SNI for proxy backend requests. Make sure we don't do it for
+     * pure SSLv3 connections, and also prevent IP addresses
+     * from being included in the SNI extension.
      */
     if (sslconn->is_proxy) {
-        const char *hostname_note = apr_table_get(c->notes, "proxy-request-hostname");
-        if (hostname_note) {
-            if (SSL_SetURL(sslconn->ssl, hostname_note) == -1) {
+        char *name = SSL_RevealURL(sslconn->ssl);
+        const char *hostname_note;
+        SSLChannelInfo channel;
+        apr_ipsubnet_t *ip;
+
+        if (name) {
+            /* handshake is completed, SNI hostname already set */
+            PORT_Free(name);
+            return APR_SUCCESS;
+        }
+
+        hostname_note = apr_table_get(c->notes, "proxy-request-hostname");
+
+        if ((hostname_note) &&
+            (SSL_GetChannelInfo(sslconn->ssl, &channel, sizeof channel)
+                == SECSuccess) &&
+            (channel.protocolVersion != SSL_LIBRARY_VERSION_3_0) &&
+             apr_ipsubnet_create(&ip, hostname_note, NULL, c->pool)
+                != APR_SUCCESS)
+        {
+            if ((rv = SSL_SetURL(sslconn->ssl, hostname_note)) != SECSuccess) {
                 ap_log_error(APLOG_MARK, APLOG_INFO, 0, c->base_server,
-                              "Error setting SNI extension for SSL Proxy request: %d",
-                              PR_GetError());
+                    "Error setting SNI extension for SSL Proxy request: %d",
+                     PR_GetError());
             } else {
-                ap_log_error(APLOG_MARK, APLOG_INFO, 0, c,
-                              "SNI extension for SSL Proxy request set to '%s'",
-                              hostname_note);
+                ap_log_error(APLOG_MARK, APLOG_INFO, 0, c->base_server,
+                    "SNI extension for SSL Proxy request set to '%s'",
+                     hostname_note);
             }
-        }
-        else {
-            ap_log_error(APLOG_MARK, APLOG_INFO, 0, c,
-                              "Can't set SNI extension: no hostname available";
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_INFO, 0, c->base_server,
+                "Can't set SNI extension: no hostname available");
         }
     }