[openssl-commits] [openssl] master update

[kaduk at mit.edu]

The branch master has been updated
       via  a75be9fd34b5d66f349186f21cd8d063d2fa87a4 (commit)
       via  45a2353056da3f357a924131578ad0a4a2e5fbb7 (commit)
      from  9d91530d2d7da1447b7be8631b269599023430e7 (commit)


- Log -----------------------------------------------------------------
commit a75be9fd34b5d66f349186f21cd8d063d2fa87a4
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date:   Wed Jul 25 21:00:45 2018 -0500

    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 <levitte at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/6792)

commit 45a2353056da3f357a924131578ad0a4a2e5fbb7
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date:   Wed Jul 25 14:48:30 2018 -0500

    Fix ossl_shim SNI handling
    
    To start with, actually set an SNI callback (copied from bssl_shim); we
    weren't actually testing much otherwise (and just happened to have been
    passing due to buggy libssl behavior prior to
    commit 1c4aa31d79821dee9be98e915159d52cc30d8403).
    
    Also use proper C++ code for handling C strings -- when a C API
    (SSL_get_servername()) returns NULL instead of a string, special-case
    that instead of blindly trying to compare NULL against a std::string,
    and perform the comparsion using the std::string operators instead of
    falling back to pointer comparison.
    
    Reviewed-by: Richard Levitte <levitte at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/6792)

-----------------------------------------------------------------------

Summary of changes:
 ssl/ssl_lib.c               |  5 ++++-
 test/ossl_shim/ossl_shim.cc | 21 ++++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 10a7694..15380e1 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;
 }
diff --git a/test/ossl_shim/ossl_shim.cc b/test/ossl_shim/ossl_shim.cc
index b1067e8..90d1f1e 100644
--- a/test/ossl_shim/ossl_shim.cc
+++ b/test/ossl_shim/ossl_shim.cc
@@ -459,6 +459,20 @@ static int CustomExtensionParseCallback(SSL *ssl, unsigned extension_value,
   return 1;
 }
 
+static int ServerNameCallback(SSL *ssl, int *out_alert, void *arg) {
+  // SNI must be accessible from the SNI callback.
+  const TestConfig *config = GetTestConfig(ssl);
+  const char *server_name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
+  if (server_name == nullptr ||
+      std::string(server_name) != config->expected_server_name) {
+    fprintf(stderr, "servername mismatch (got %s; want %s)\n", server_name,
+            config->expected_server_name.c_str());
+    return SSL_TLSEXT_ERR_ALERT_FATAL;
+  }
+
+  return SSL_TLSEXT_ERR_OK;
+}
+
 // Connect returns a new socket connected to localhost on |port| or -1 on
 // error.
 static int Connect(uint16_t port) {
@@ -645,6 +659,10 @@ static bssl::UniquePtr<SSL_CTX> SetupCtx(const TestConfig *config) {
                                       sizeof(sess_id_ctx) - 1))
     return nullptr;
 
+  if (!config->expected_server_name.empty()) {
+    SSL_CTX_set_tlsext_servername_callback(ssl_ctx.get(), ServerNameCallback);
+  }
+
   return ssl_ctx;
 }
 
@@ -809,7 +827,8 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume) {
   if (!config->expected_server_name.empty()) {
     const char *server_name =
         SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
-    if (server_name != config->expected_server_name) {
+    if (server_name == nullptr ||
+            std::string(server_name) != config->expected_server_name) {
       fprintf(stderr, "servername mismatch (got %s; want %s)\n",
               server_name, config->expected_server_name.c_str());
       return false;