On Mon, 25 Nov 2013 15:55:41 -0600
"William A. Rowe Jr." <wr...@rowe-clan.net> wrote:
> It appears that our SNI hostname comparison is invalid for forward
> proxy applications, specifically proxy CONNECT. RFC 2616 states;
>
> 14.23 Host
>
> The Host request-header field specifies the Internet host and port
> number of the resource being requested, as obtained from the
> original URI given by the user or referring resource (generally an
> HTTP URL, as described in section 3.2.2). The Host field value MUST
> represent the naming authority of the origin server or gateway given
> by the original URL. This allows the origin server or gateway to
> differentiate between internally-ambiguous URLs, such as the root
> "/" URL of a server for multiple host names on a single IP address.
>
> I had read this as proxy host's name in forward proxy applications,
> but reading on, it's clear this is ambiguous; from RFC 2817
>
> 5.2 Requesting a Tunnel with CONNECT
>
> A CONNECT method requests that a proxy establish a tunnel
> connection on its behalf. The Request-URI portion of the Request-Line
> is always an 'authority' as defined by URI Generic Syntax [2], which
> is to say the host name and port number destination of the requested
> connection separated by a colon:
>
> CONNECT server.example.com:80 HTTP/1.1
> Host: server.example.com:80
>
> So we have a clear, RFC-approved example of using the final endpoint
> as the Host: header value, not the intermediate proxy.example.com.
>
> In this case, we fail the request because the SNI of the proxy machine
> does not match the Host: of the origin server name.
>
> Ideas for the appropriate patch to httpd? Scope this fix to CONNECT
> requests alone, or all forward proxy requests?
After reading a rather confused and confusing thread on this question,
I've come to the conclusion that the entire block of code below is
wrong;
167 rpluem 757373 if ((servername = SSL_get_servername(ssl,
TLSEXT_NAMETYPE_host_name))) {
168 char *host, *scope_id;
169 apr_port_t port;
170 apr_status_t rv;
171
172 /*
173 * The SNI extension supplied a hostname. So
don't accept requests
174 * with either no hostname or a different
hostname.
175 */
176 if (!r->hostname) {
177 sf 1209766 ap_log_error(APLOG_MARK, APLOG_ERR, 0,
r->server, APLOGNO(02031)
178 rpluem 757373 "Hostname %s provided via SNI,
but no hostname"
179 " provided in HTTP request",
servername);
180 return HTTP_BAD_REQUEST;
181 }
This cannot occur for HTTP/1.1 requests as missing Host: is rejected elsewhere.
We have no particular reason to test for or prohibit HTTP/1.0 or 0.9 requests,
AFAICT.
182 rv = apr_parse_addr_port(&host, &scope_id,
&port, r->hostname, r->pool);
183 if (rv != APR_SUCCESS || scope_id) {
184 return HTTP_BAD_REQUEST;
185 }
186 jorton 1082189 if (strcasecmp(host, servername)) {
187 sf 1209766 ap_log_error(APLOG_MARK, APLOG_ERR, 0,
r->server, APLOGNO(02032)
188 rpluem 757373 "Hostname %s provided via SNI
and hostname %s provided"
189 " via HTTP are different",
servername, host);
190 return HTTP_BAD_REQUEST;
191 }
This occurs in all cases for HTTP CONNECT requests. There seems to be no
justification to test for this condition, since VirtualHost only establishes
a relationship of the HTTP Host: header. Any relationship to TLS/SNI hostnames
is a new (unwise) invention, which has broken a number of facilities in httpd.
Validation of the SNI hostname against ServerName/ServerAlias already occurs
elsewhere. As Yann is pointing out, validation of SSL session requirements
also already occurs elsewhere.
The definition of SNI hostname and HTTP Host: header values were never equal,
only roughly equivalent, and this attempt to make them synonymous has really
been disruptive to httpd stability.
I'm proposing the following patch to drop this foolishness into the very same
SSLStrictSNIVHostCheck ruleset, for those interesting in adding these new,
non-HTTP compliance requirements as HTTP compliance requirements.
Comments?
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c (revision 1550833)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -164,48 +164,52 @@
return DECLINED;
}
#ifdef HAVE_TLSEXT
- if ((servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
- char *host, *scope_id;
- apr_port_t port;
- apr_status_t rv;
+ /* Check whether strict SNI checking was setup either in the
+ * server config used for handshaking or the current server config.
+ * This should avoid insecure configuration by accident.
+ */
+ if ((sc->strict_sni_vhost_check == SSL_ENABLED_TRUE)
+ || (mySrvConfig(sslconn->server))->strict_sni_vhost_check
+ == SSL_ENABLED_TRUE)) {
+ if ((servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
+ char *host, *scope_id;
+ apr_port_t port;
+ apr_status_t rv;
- /*
- * The SNI extension supplied a hostname. So don't accept requests
- * with either no hostname or a different hostname.
- */
- if (!r->hostname) {
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02031)
- "Hostname %s provided via SNI, but no hostname"
- " provided in HTTP request", servername);
- return HTTP_BAD_REQUEST;
+ /*
+ * The SNI extension supplied a hostname. So don't accept requests
+ * with either no hostname or a different hostname.
+ */
+ if (!r->hostname) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02031)
+ "Hostname %s provided via SNI, but no hostname"
+ " provided in HTTP request", servername);
+ return HTTP_BAD_REQUEST;
+ }
+ rv = apr_parse_addr_port(&host, &scope_id, &port,
+ r->hostname, r->pool);
+ if (rv != APR_SUCCESS || scope_id) {
+ return HTTP_BAD_REQUEST;
+ }
+ if (strcasecmp(host, servername)) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02032)
+ "Hostname %s provided via SNI and hostname %s"
+ " provided via HTTP are different",
+ servername, host);
+ return HTTP_BAD_REQUEST;
+ }
}
- rv = apr_parse_addr_port(&host, &scope_id, &port, r->hostname, r->pool);
- if (rv != APR_SUCCESS || scope_id) {
- 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.
+ */
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02033)
+ "No hostname was provided via SNI for a name based"
+ " virtual host");
+ return HTTP_FORBIDDEN;
}
- if (strcasecmp(host, servername)) {
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02032)
- "Hostname %s provided via SNI and hostname %s provided"
- " via HTTP are different", servername, host);
- return HTTP_BAD_REQUEST;
- }
}
- else if (((sc->strict_sni_vhost_check == SSL_ENABLED_TRUE)
- || (mySrvConfig(sslconn->server))->strict_sni_vhost_check
- == SSL_ENABLED_TRUE)
- && r->connection->vhost_lookup_data) {
- /*
- * We are using a name based configuration here, but no hostname was
- * provided via SNI. Don't allow that if are requested to do strict
- * checking. Check wether this strict checking was setup either in the
- * server config we used for handshaking or in our current server.
- * This should avoid insecure configuration by accident.
- */
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, APLOGNO(02033)
- "No hostname was provided via SNI for a name based"
- " virtual host");
- return HTTP_FORBIDDEN;
- }
#endif
SSL_set_app_data2(ssl, r);
