error_handler loops forever after handshake fails

[sderouchie]

I have a case where using via-httplib as an HTTPS server, the handshake will fail due to unsupported TLS version/cipher specs:

Client Hello:

Secure Sockets Layer
    SSLv2 Record Layer: Client Hello
        [Version: SSL 2.0 (0x0002)]
        Length: 53
        Handshake Message Type: Client Hello (1)
        Version: TLS 1.0 (0x0301)
        Cipher Spec Length: 12
        Session ID Length: 0
        Challenge Length: 32
        Cipher Specs (4 specs)
            Cipher Spec: TLS_RSA_WITH_AES_128_CBC_SHA (0x00002f)
            Cipher Spec: TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x000033)
            Cipher Spec: TLS_DHE_DSS_WITH_AES_128_CBC_SHA (0x000032)
            Cipher Spec: TLS_DH_anon_WITH_AES_128_CBC_SHA (0x000034)
        Challenge

Handshake Failure:

TLSv1 Record Layer: Alert (Level: Fatal, Description: Handshake Failure)
    Content Type: Alert (21)
    Version: TLS 1.0 (0x0301)
    Length: 2
    Alert Message
        Level: Fatal (2)
        Description: Handshake Failure (40)

I think this failure is expected since the client is attempting to use a deprecated version of SSL/ciphers.

The issue is with how the library handles this. Even though the client does not try to reconnect, the error_handler loops indefinitely:

error_handler
asio.ssl:336130315
error_handler
asio.ssl:336462231
error_handler
asio.ssl:336462231
error_handler
asio.ssl:336462231
error_handler
asio.ssl:336462231
error_handler
asio.ssl:336462231

Is there some call that I am missing?

[kenba]

Thanks you for bringing this to my attention. It looks like a bug in the server accept_handler: it was resetting the nextconnection on success but not on failure...

I've changed the accept handler to reset the nextconnection regardless and pushed the change to the develop branch. Please try it out and let me know if it's fixed the issue.

[sderouchie]

That was a very quick response, thanks! I tested the change, but it does not appear to fix the issue.

Not sure if this helps, but I set a breakpoint in error_handler.

The first time it hits that handler, it is getting signalled from connection.hpp in this function:

static void handshake_callback(weak_pointer ptr,
                                     ASIO_ERROR_CODE const& error)
      {
        shared_pointer pointer(ptr.lock());
        if (pointer && (ASIO::error::operation_aborted != error))
        {
          if (!error)
          {
            pointer->connected_ = true;
            pointer->event_callback_(CONNECTED, ptr);
            pointer->set_socket_options();
            if (!pointer->tx_queue_->empty())
              pointer->write_data
          (ConstBuffers(1, ASIO::buffer(pointer->tx_queue_->front())));
            pointer->receiving_ = false;
            pointer->enable_reception();
          }
          else
          {
            pointer->close();
            pointer->signal_error(error);        <-- this calls the error_handler
          }
        }
      }

If I then continue, the subsequent calls to error_handler get signalled from here:

      static void write_callback(weak_pointer ptr,
                                 ASIO_ERROR_CODE const& error,
                                 size_t bytes_transferred,
                                 std::shared_ptr<std::deque<Container> >) // tx_queue)
      {
        shared_pointer pointer(ptr.lock());
        if (pointer && (ASIO::error::operation_aborted != error))
        {
          if (error)
          {
            pointer->tx_queue_->clear();
            pointer->signal_error(error); <-- this calls the error handler
          }
          else
          {
            if (pointer->disconnect_pending_)
              pointer->shutdown();
            else
              pointer->write_handler(bytes_transferred);
          }
        }
      }

[kenba]

Clearly my response was too quick!

You are correct that the signal_error function is called by both the handshake_callback and write_callback. However, the signal_error function calls is_error_a_disconnect to determine whether the socket should be disconnected in response to the error, because not all errors should cause a disconnect...

For SSL sockets, the is_error_a_disconnect function calls is_disconnect in the ssl_tcp_adaptor.hpp file. It is this function that determines whether to disconnect or continue. I believe that in your case, it's not recognising the SSL code(s) : asio.ssl:336130315 and asio.ssl:336462231 as errors in the line:

bool ssl_error(ASIO::error::get_ssl_category() == error.category());

Please can you set a breakpoint in is_disconnect and determine whether the ssl_error flag is being set?

[sderouchie]

Ok, so I set a breakpoint in there and I can see that ssl_error is set to 'true'.

Then it sets the ssl_shutdown flag:

 ssl_shutdown = ssl_error &&
// SSL_R_SHORT_READ is no longer defined in openssl 1.1.x
#ifdef SSL_R_SHORT_READ
               (SSL_R_SHORT_READ != ERR_GET_REASON(error.value())) &&
#endif
               (SSL_R_PROTOCOL_IS_SHUTDOWN != ERR_GET_REASON(error.value()));

          return ssl_error && !ssl_shutdown;

The ssl_shutdown flag is also true. Therefore, this function returns false.

This causes is_error_a_disconnect to call shutdown(), and return false.

[kenba]

Thank you for that information. I understand the issue now.

The connection is attempting to shutdown gracefully in the is_error_a_disconnect function called by signal_error. However, the connection isn't fully established at this point but the shutdown function tries to send an SSL close_notify message on the connection...

So I've removed the call to signal_error in handshake_callback and connect_callback and replaced it with calls to error_callback_ and event_callback_ to simply close the socket and remove the connection.

I've pushed the changes to the develop branch. Please try it out and let me know whether they've fixed the issue.

[sderouchie]

Thank you! This has resolved the issue.

[kenba]

Thank you. That's great news. I've copied over the changes from the develop branch into master and tagged it 1.5.1.