From a75be9fd34b5d66f349186f21cd8d063d2fa87a4 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 25 Jul 2018 21:00:45 -0500 Subject: [PATCH] Improve backwards compat for SSL_get_servername() Commit 1c4aa31d79821dee9be98e915159d52cc30d8403 changed how we process and store SNI information during the handshake, so that a hostname is only saved in the SSL_SESSION structure if that SNI value has actually been negotiated. SSL_get_servername() was adjusted to match, with a new conditional being added to handle the case when the handshake processing is ongoing, and a different location should be consulted for the offered SNI value. This was done in an attempt to preserve the historical behavior of SSL_get_servername(), a function whose behavior only mostly matches its documentation, and whose documentation is both lacking and does not necessarily reflect the actual desired behavior for such an API. Unfortunately, sweeping changes that would bring more sanity to this space are not possible until OpenSSL 1.2.0, for ABI compatibility reasons, so we must attempt to maintain the existing behavior to the extent possible. The above-mentioned commit did not take into account the behavior of SSL_get_servername() during resumption handshakes for TLS 1.2 and prior, where no SNI negotiation is performed. In that case we would not properly parse the incoming SNI and erroneously return NULL as the servername, when instead the logical session is associated with the SNI value cached in the SSL_SESSION. (Note that in some cases an SNI callback may not need to do anything in a TLS 1.2 or prior resumption flow, but we are calling the callbacks and did not provide any guidance that they should no-op if the connection is being resumed, so we must handle this case in a usable fashion.) Update our behavior accordingly to return the session's cached value during the handshake, when resuming. This fixes the boringssl tests. [extended tests] Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/6792) --- ssl/ssl_lib.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 10a76940dd..15380e1b72 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2618,8 +2618,11 @@ const char *SSL_get_servername(const SSL *s, const int type) * peer send" and "what was actually negotiated"; we should have * a clear distinction amongst those three. */ - if (SSL_in_init(s)) + if (SSL_in_init(s)) { + if (s->hit) + return s->session->ext.hostname; return s->ext.hostname; + } return (s->session != NULL && s->ext.hostname == NULL) ? s->session->ext.hostname : s->ext.hostname; } -- 2.25.1