Patch candidat to merge in 1.8dev.
I think this patch should be backported, at least in versions compat with openssl-1.1.0.


Attachment: 0001-BUG-MEDIUM-ssl-switchctx-should-not-return-SSL_TLSEX.patch
Description: Binary data


Le 3 mars 2017 à 10:50, Emmanuel Hocdet <[email protected]> a écrit :

Hi Roberto,

I agree with that. And so the RFC says.

Manu

Le 2 mars 2017 à 23:26, Roberto Guimaraes <[email protected]> a écrit :

hi thanks for the reply.

You are right about SSL_TLSEXT_ERR_NOACK as it is a pretty innocuous return in openssl-1.1 (merely sets a flag which may even seem redundant considering the other checks).

However, I’ve confirmed that the issue I see is indeed caused by a SSL_TLSEXT_ERR_ALERT_WARNING return from the SNI callback.

Reproducing this is fairly simple: build haproxy with openssl-1.1.x and cause an SNI mismatch.
--
GnuTLS base clients will fail consistently:

apt-get gnutls_handshake() failed: A TLS warning alert has been received.

wget
GnuTLS: A TLS warning alert has been received.
GnuTLS: received alert [112]: The server name sent was not recognized

Secure Sockets Layer
    TLSv1.2 Record Layer: Alert (Level: Warning, Description: Unrecognized Name)
        Content Type: Alert (21)
        Version: TLS 1.2 (0x0303)
        Length: 2
        Alert Message
            Level: Warning (1)
            Description: Unrecognized Name (112)
    TLSv1.2 Record Layer: Handshake Protocol: Server Hello

Sorry, I did not mean that s_server has changed, but openssl library itself has changed in 1.1 concerning these warnings. I don’t see them getting sent with openssl-1.0.2x upon SNI mismatch at all, but I do see them on the wire (tcpdump, tshark) with openssl-1.1.x.

Also, reading the RFC, it seems sending the alert is not recommended:


It is NOT RECOMMENDED to send a warning-level unrecognized_name(112) alert,
   because the client's behavior in response to warning-level alerts is
   unpredictable.

therefore I think that openssl-1.1 support demands the following patch (works just as well with prior openssl releases):

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 1a9c185..95b5dc7 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1707,7 +1707,7 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *priv)
 #endif
                return (s->strict_sni ?
                        SSL_TLSEXT_ERR_ALERT_FATAL :
-                       SSL_TLSEXT_ERR_ALERT_WARNING);
+                       SSL_TLSEXT_ERR_OK);
        }

        /* switch ctx */

thanks,
roberto


On Feb 17, 2017, at 10:22 AM, Emmanuel Hocdet <[email protected]> wrote:

Hi Roberto

Le 17 févr. 2017 à 01:27, Roberto Guimaraes <[email protected]> a écrit :

greetings,

just a heads up that we’ve seen client breakage when using haproxy with openssl-1.1 — dunno how far along you are concerning ossl1.1 usage, but it has become very clear that openssl-1.1 behaves differently in a number of ways esp SNI callback.

so, openssl-1.0.x does not actually send the warnings/alerts when the callback returns SSL_TLSEXT_ERR_NOACK or SSL_TLSEXT_ERR_ALERT_WARNING, but openssl-1.1 will send them.

Some clients, such as java1.8 and gnutls, will fail upon receiving these warnings, even if the RFC isn’t clear on the correct behavior.

It’s strange. I suppose it’s another problem. Need logs and more infos to help.

I don’t know if this 100% applicable to you, but perhaps consider removing the warnings unless explicitly requested with strict_sni? Looking at the “reference” s_server.c in openssl-1.1 and it does return ERR_OK for both mismatch and missing servername.

I don’t seen any changes about that at s_server.c in openssl-1.1.

SSL_TLSEXT_ERR_NOACK is used when client does not sent an SNI. Is not a warning and is useful.

SSL_TLSEXT_ERR_ALERT_WARNING is used when certificate does not match the SNI sent by the client.
it could be removed because clients check it and generate the alert.
Replacing it with SSL_TLSEXT_ERR_OK will not change anything to correct clients. BoringSSL ignores it and
sends SSL_TLSEXT_ERR_OK instead and it does not change anything for all known clients (via ssllabs).

Manu

roberto

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index e7eb5df..2b227fc 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1470,7 +1470,7 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *priv)
#endif
              return (s->strict_sni ?
                      SSL_TLSEXT_ERR_ALERT_FATAL :
-                       SSL_TLSEXT_ERR_NOACK);
+                       SSL_TLSEXT_ERR_OK);
      }

      for (i = 0; i < trash.size; i++) {
@@ -1507,7 +1507,7 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *priv)
#endif
              return (s->strict_sni ?
                      SSL_TLSEXT_ERR_ALERT_FATAL :
-                       SSL_TLSEXT_ERR_ALERT_WARNING);
+                       SSL_TLSEXT_ERR_OK);
      }

      /* switch ctx */




Reply via email to