On Mon, Dec 23, 2013 at 9:49 PM, Ruediger Pluem <rpl...@apache.org> wrote: > > > Kaspar Brand wrote: >> On 21.12.2013 14:21, Ruediger Pluem wrote: >>>> I guess a more general fix for this would be: >>> >>> No further comments / feedback? If not then I would commit the patch. >> >> The change looks fine to me (for easier comparison/review, >> a whitespace-change-ignoring version is attached). >> >> What would probably make sense is to amend the following comment >> on this occasion: >> >> /* >> * The SNI extension supplied a hostname. So don't accept requests >> * with either no hostname or a different hostname. >> */ >> >> It doesn't say anything about the rationale right now, and as >> recent discussions have shown, it would be helpful to explain >> why this is done. > > Done. I have tried to improve the comment and added a TODO if my > reasoning for the checks is really true.
Sorry to be late, I'm overwhelmed these days... + /* + * The SNI extension supplied a hostname. So don't accept requests + * with either no hostname or a different hostname as this could + * cause us to end up in a different virtual host as the one that + * was used for the handshake causing different SSL parameters to + * be applied. + * XXX: TODO check if this is really true and that there are + * SSL parameters that are not fixed by a renegotiation in + * ssl_hook_Access. + */ According to http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox/%3c48592955.2090...@velox.ch%3E, the (great) analyse Kaspar made in 2008, the only parameters which won't be renegotiated are SSLCACertificateFile/Path and SSLCADNRequestFile/Path. This is because of the lacking OpenSSL's SSL_set_cert_store() function, which always seem to be the case with the latest versions (AFAICT). There may also be issues when SSLVerifyClient is "optional*", but optional issues IMHO. + 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; + } One thing this code is possibly missing is HTTP < 1.1 requests (as William noted in his proposed patch), where no Host is provided, couldn't SNI be used by the client in this case? Personnaly I like the idea of being able to disable the check, when SSLStrictSNIVHostCheck is off for example, but probably it should be set "on" by default in this case. Regards, Yann.
