[RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Lukas Tribus]

Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
from accessing packet_length directly (not available in LibreSSL) to
calling SSL_state() instead.

However, SSL_state() appears to be fully broken in both OpenSSL and
LibreSSL.

Since there is no possibility in LibreSSL to detect an empty handshake,
let's not try (like BoringSSL) and restore this functionality for
OpenSSL 1.0.2 and older, by reverting to the previous behavior.

Should be backported to 2.0.
---

Requesting feedback from Ilya.

---
 src/ssl_sock.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index c9fffbe..d4b6852 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5352,7 +5352,7 @@ static int ssl_sock_handshake(struct connection *conn, 
unsigned int flag)
if (!errno && conn->flags & CO_FL_WAIT_L4_CONN)
conn->flags &= ~CO_FL_WAIT_L4_CONN;
if (!conn->err_code) {
-#ifdef OPENSSL_IS_BORINGSSL /* BoringSSL */
+#if defined(OPENSSL_IS_BORINGSSL) || defined(LIBRESSL_VERSION_NUMBER) /* 
BoringSSL or LibreSSL */
conn->err_code = CO_ER_SSL_HANDSHAKE;
 #else
int empty_handshake;
@@ -5360,7 +5360,7 @@ static int ssl_sock_handshake(struct connection *conn, 
unsigned int flag)
OSSL_HANDSHAKE_STATE state = 
SSL_get_state((SSL *)ctx->ssl);
empty_handshake = state == 
TLS_ST_BEFORE;
 #else
-   empty_handshake = SSL_state((SSL 
*)ctx->ssl) == SSL_ST_BEFORE;
+   empty_handshake = 
!ctx->ssl->packet_length;
 #endif
if (empty_handshake) {
if (!errno) {
@@ -5433,7 +5433,7 @@ check_error:
if (!errno && conn->flags & CO_FL_WAIT_L4_CONN)
conn->flags &= ~CO_FL_WAIT_L4_CONN;
if (!conn->err_code) {
-#ifdef OPENSSL_IS_BORINGSSL  /* BoringSSL */
+#if defined(OPENSSL_IS_BORINGSSL) || defined(LIBRESSL_VERSION_NUMBER) /* 
BoringSSL or LibreSSL */
conn->err_code = CO_ER_SSL_HANDSHAKE;
 #else
int empty_handshake;
@@ -5441,7 +5441,7 @@ check_error:
OSSL_HANDSHAKE_STATE state = 
SSL_get_state(ctx->ssl);
empty_handshake = state == TLS_ST_BEFORE;
 #else
-   empty_handshake = SSL_state((SSL *)ctx->ssl) == 
SSL_ST_BEFORE;
+   empty_handshake = !ctx->ssl->packet_length;
 #endif
if (empty_handshake) {
if (!errno) {
-- 
2.7.4

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Willy Tarreau]

On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
> Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> from accessing packet_length directly (not available in LibreSSL) to
> calling SSL_state() instead.
(...)

Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
concrete user-visible issue this bug can cause ? It would help bisecting
issues later. I don't know in what case an empty handshake may happen.

Cheers,
Willy

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Lukas Tribus]

Hello Willy,

On Mon, 1 Jul 2019 at 22:34, Willy Tarreau  wrote:
>
> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
> > Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> > changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> > from accessing packet_length directly (not available in LibreSSL) to
> > calling SSL_state() instead.
> (...)
>
> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
> concrete user-visible issue this bug can cause ? It would help bisecting
> issues later. I don't know in what case an empty handshake may happen.

The investigation was initiated by the following discourse thread:
https://discourse.haproxy.org/t/haproxy-2-0-ssl-handshake-failure/3954

It's about an Amazon ELB load-balancer and it's TCP-only health check,
which does not initiate a SSL handshake after connecting (and in this
case, sending the proxy protocol). Haproxy 1.9 recognized the missing
handshake and provides an appropriate log, 2.0 does not and logs a
handshake failure/error. There is still a disparity between what the
discourse report is saying (no logs at all for the ELB health checks
in 1.9), and what I can actually reproduce (empty handshake detection
causing a "Connection closed during SSL handshake" log message) - but
I'm gonna ignore that as I don't have any clue why this is.

So the difference is what we log in this situation; we should not log
an error or a failure when the SSL handshake didn't even begin.


Lukas

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Илья Шипицин]

вт, 2 июл. 2019 г. в 01:34, Willy Tarreau :

> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
> > Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> > changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> > from accessing packet_length directly (not available in LibreSSL) to
> > calling SSL_state() instead.
> (...)
>
> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
> concrete user-visible issue this bug can cause ? It would help bisecting
> issues later. I don't know in what case an empty handshake may happen.
>

nmap scan ?


>
> Cheers,
> Willy
>

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Willy Tarreau]

On Mon, Jul 01, 2019 at 10:55:41PM +0200, Lukas Tribus wrote:
> Hello Willy,
> 
> On Mon, 1 Jul 2019 at 22:34, Willy Tarreau  wrote:
> >
> > On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
> > > Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> > > changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> > > from accessing packet_length directly (not available in LibreSSL) to
> > > calling SSL_state() instead.
> > (...)
> >
> > Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
> > concrete user-visible issue this bug can cause ? It would help bisecting
> > issues later. I don't know in what case an empty handshake may happen.
> 
> The investigation was initiated by the following discourse thread:
> https://discourse.haproxy.org/t/haproxy-2-0-ssl-handshake-failure/3954
> 
> It's about an Amazon ELB load-balancer and it's TCP-only health check,
> which does not initiate a SSL handshake after connecting (and in this
> case, sending the proxy protocol). Haproxy 1.9 recognized the missing
> handshake and provides an appropriate log, 2.0 does not and logs a
> handshake failure/error. There is still a disparity between what the
> discourse report is saying (no logs at all for the ELB health checks
> in 1.9), and what I can actually reproduce (empty handshake detection
> causing a "Connection closed during SSL handshake" log message) - but
> I'm gonna ignore that as I don't have any clue why this is.
> 
> So the difference is what we log in this situation; we should not log
> an error or a failure when the SSL handshake didn't even begin.

Ah OK, this makes sense, thanks for the explanation!

Willy

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Lukas Tribus]

Hello Ilya,


On Mon, 1 Jul 2019 at 23:08, Илья Шипицин  wrote:
>
>
>
> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau :
>>
>> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
>> > Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
>> > changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
>> > from accessing packet_length directly (not available in LibreSSL) to
>> > calling SSL_state() instead.
>> (...)
>>
>> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
>> concrete user-visible issue this bug can cause ? It would help bisecting
>> issues later. I don't know in what case an empty handshake may happen.
>
>
> nmap scan ?

Ilya, just to avoid misunderstandings, I would like to have your
feedback on this patch so we can decide whether to commit it or work
on counter-proposals.


Thanks,
Lukas

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Emmanuel Hocdet]

Hi,

This thread reminds me that with BoringSSL empty (and abort) handshake is not 
set.
After tests BoringSSL seems to have simpler case.
I sent a patch to fix that.

For OpenSSL <= 1.0.2, revert is the thing to do.
For LibreSSL, include it with BoringSSL case could be ok (with my patch).
With time (no HB and better error report in libSSL), it seems code could simply 
look like:
  if (!errno)
  conn->err_code = CO_ER_SSL_EMPTY;
  else
  conn->err_code = CO_ER_SSL_ABORT;

++
Manu

> Le 4 juil. 2019 à 12:14, Lukas Tribus  a écrit :
> 
> Hello Ilya,
> 
> 
> On Mon, 1 Jul 2019 at 23:08, Илья Шипицин  > wrote:
>> 
>> 
>> 
>> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau :
>>> 
>>> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
 Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
 changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
 from accessing packet_length directly (not available in LibreSSL) to
 calling SSL_state() instead.
>>> (...)
>>> 
>>> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
>>> concrete user-visible issue this bug can cause ? It would help bisecting
>>> issues later. I don't know in what case an empty handshake may happen.
>> 
>> 
>> nmap scan ?
> 
> Ilya, just to avoid misunderstandings, I would like to have your
> feedback on this patch so we can decide whether to commit it or work
> on counter-proposals.
> 
> 
> Thanks,
> Lukas

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Илья Шипицин]

can you provide some comment around code ?

I think almost nobody can read such code

чт, 4 июл. 2019 г. в 21:17, Emmanuel Hocdet :

> Hi,
>
> This thread reminds me that with BoringSSL empty (and abort) handshake is
> not set.
> After tests BoringSSL seems to have simpler case.
> I sent a patch to fix that.
>
> For OpenSSL <= 1.0.2, revert is the thing to do.
> For LibreSSL, include it with BoringSSL case could be ok (with my patch).
> With time (no HB and better error report in libSSL), it seems code could
> simply look like:
>   *if* (!errno)
>   conn->err_code = CO_ER_SSL_EMPTY;
>   *else*
>   conn->err_code = CO_ER_SSL_ABORT;
>
> ++
> Manu
>
> Le 4 juil. 2019 à 12:14, Lukas Tribus  a écrit :
>
> Hello Ilya,
>
>
> On Mon, 1 Jul 2019 at 23:08, Илья Шипицин  wrote:
>
>
>
>
> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau :
>
>
> On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
>
> Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> from accessing packet_length directly (not available in LibreSSL) to
> calling SSL_state() instead.
>
> (...)
>
> Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
> concrete user-visible issue this bug can cause ? It would help bisecting
> issues later. I don't know in what case an empty handshake may happen.
>
>
>
> nmap scan ?
>
>
> Ilya, just to avoid misunderstandings, I would like to have your
> feedback on this patch so we can decide whether to commit it or work
> on counter-proposals.
>
>
> Thanks,
> Lukas
>
>
>

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Emmanuel Hocdet]

> Le 4 juil. 2019 à 18:55, Илья Шипицин  a écrit :
> 
> can you provide some comment around code ?
> 
> I think almost nobody can read such code
> 
> чт, 4 июл. 2019 г. в 21:17, Emmanuel Hocdet  >:
> Hi,
> 
> This thread reminds me that with BoringSSL empty (and abort) handshake is not 
> set.
> After tests BoringSSL seems to have simpler case.
> I sent a patch to fix that.
> 
> For OpenSSL <= 1.0.2, revert is the thing to do.
> For LibreSSL, include it with BoringSSL case could be ok (with my patch).
> With time (no HB and better error report in libSSL), it seems code could 
> simply look like:
>   if (!errno)
>   conn->err_code = CO_ER_SSL_EMPTY;
>   else
>   conn->err_code = CO_ER_SSL_ABORT;
> 

Only CO_ER_SSL_EMPTY and CO_ER_SSL_ABORT  can be set for conn->err_code
(it’s the case for BoringSSL)


> ++
> Manu
> 
>> Le 4 juil. 2019 à 12:14, Lukas Tribus mailto:lu...@ltri.eu>> 
>> a écrit :
>> 
>> Hello Ilya,
>> 
>> 
>> On Mon, 1 Jul 2019 at 23:08, Илья Шипицин > > wrote:
>>> 
>>> 
>>> 
>>> вт, 2 июл. 2019 г. в 01:34, Willy Tarreau >> >:
 
 On Mon, Jul 01, 2019 at 10:32:29PM +0200, Lukas Tribus wrote:
> Commit 54832b97 ("BUILD: enable several LibreSSL hacks, including")
> changed empty handshake detection in OpenSSL <= 1.0.2 and LibreSSL,
> from accessing packet_length directly (not available in LibreSSL) to
> calling SSL_state() instead.
 (...)
 
 Thanks a lot Lukas. Just out of curiosity, do you have any idea of a
 concrete user-visible issue this bug can cause ? It would help bisecting
 issues later. I don't know in what case an empty handshake may happen.
>>> 
>>> 
>>> nmap scan ?
>> 
>> Ilya, just to avoid misunderstandings, I would like to have your
>> feedback on this patch so we can decide whether to commit it or work
>> on counter-proposals.
>> 
>> 
>> Thanks,
>> Lukas
> 

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Willy Tarreau]

hi Guys,

On Fri, Jul 05, 2019 at 05:07:27PM +0200, Emmanuel Hocdet wrote:
> 
> > Le 4 juil. 2019 à 18:55,  ???  a écrit :
> > 
> > can you provide some comment around code ?
> > 
> > I think almost nobody can read such code
> > 
> > ??, 4 ???. 2019 ?. ? 21:17, Emmanuel Hocdet  > >:
> > Hi,
> > 
> > This thread reminds me that with BoringSSL empty (and abort) handshake is 
> > not set.
> > After tests BoringSSL seems to have simpler case.
> > I sent a patch to fix that.
> > 
> > For OpenSSL <= 1.0.2, revert is the thing to do.
> > For LibreSSL, include it with BoringSSL case could be ok (with my patch).
> > With time (no HB and better error report in libSSL), it seems code could 
> > simply look like:
> >   if (!errno)
> >   conn->err_code = CO_ER_SSL_EMPTY;
> >   else
> >   conn->err_code = CO_ER_SSL_ABORT;
> > 
> 
> Only CO_ER_SSL_EMPTY and CO_ER_SSL_ABORT  can be set for conn->err_code
> (it's the case for BoringSSL)

Thanks Manu. Ilya and Lukas, just let me know if you still have any objection
against this patch being merged, or if I should wait for Lukas' one first to
be tested. Anything's fine for me, I'm just waiting for instructions.

Willy

Re: [RFC PATCH] BUG/MINOR: ssl: revert empty handshake detection in OpenSSL <= 1.0.2

[Илья Шипицин]

сб, 6 июл. 2019 г. в 16:09, Willy Tarreau :

> hi Guys,
>
> On Fri, Jul 05, 2019 at 05:07:27PM +0200, Emmanuel Hocdet wrote:
> >
> > > Le 4 juil. 2019 à 18:55,  ???  a écrit :
> > >
> > > can you provide some comment around code ?
> > >
> > > I think almost nobody can read such code
> > >
> > > ??, 4 ???. 2019 ?. ? 21:17, Emmanuel Hocdet  m...@gandi.net>>:
> > > Hi,
> > >
> > > This thread reminds me that with BoringSSL empty (and abort) handshake
> is not set.
> > > After tests BoringSSL seems to have simpler case.
> > > I sent a patch to fix that.
> > >
> > > For OpenSSL <= 1.0.2, revert is the thing to do.
> > > For LibreSSL, include it with BoringSSL case could be ok (with my
> patch).
> > > With time (no HB and better error report in libSSL), it seems code
> could simply look like:
> > >   if (!errno)
> > >   conn->err_code = CO_ER_SSL_EMPTY;
> > >   else
> > >   conn->err_code = CO_ER_SSL_ABORT;
> > >
> >
> > Only CO_ER_SSL_EMPTY and CO_ER_SSL_ABORT  can be set for conn->err_code
> > (it's the case for BoringSSL)
>
> Thanks Manu. Ilya and Lukas, just let me know if you still have any
> objection
> against this patch being merged, or if I should wait for Lukas' one first
> to
> be tested. Anything's fine for me, I'm just waiting for instructions.
>

Lukas gave very detailed description in mail.
however, code is not documented.

I'd like to have more comments around. Code itself is fine


>
> Willy
>