Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> Le 18 juin 2018 à 15:54, Thierry Fournier a > écrit : > > I don’t known. In fact it works, so it is not a bug. But, when I use the > reservation for an ex_data slot, it returns the slot 0, and this slot is > used for the compatibility layer and can be crush some data. I conclude > that is a bug in Openssl. The reservation function must give a slot > starting to 1. > Indeed, it’s unexpected. > Maybe, the recommendation is to not mix the compatibility functions > like set_app_data() with *ex_data*() functions. But, I don’t see > anything about this. > Neither do I... In any case it's cleaner with *_get_ex_new_index. ++ Manu
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> On 18 Jun 2018, at 15:38, Emmanuel Hocdet wrote:
>
>
>> Le 18 juin 2018 à 15:30, Thierry Fournier a
>> écrit :
>>
>>
>>
>>> On 18 Jun 2018, at 14:37, Emmanuel Hocdet wrote:
>>>
Le 18 juin 2018 à 10:43, Thierry Fournier
a écrit :
> On 18 Jun 2018, at 10:33, Willy Tarreau wrote:
>
> On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org
> wrote:
>> Finally, I got it ! It works with luck because we have 1 bug in Haproxy
>> and 1 error (I suppose) in a OpenSSL compatibility layer.
> (...)
>> I join two patch. The first which fix the cipher capture must be
>> backported to 1.8, for the second patch wich fix the app data
>> compatibility, I dont known (at least 1.8).
>
> Good job! I imagine you didn't have a funny week-end playing with this
> one :-/
Yes, including the Friday :-) But I hope this path improve stability. If
someone
have time and is interested by the subject, it may be interesting to see
in the
OpenSSL code if the slot 0 used without reservation works fine, or works
because
we have luck.
>>>
>>> It work find because slot 0 is natively reserved for old *_{set,
>>> get}_app_data API compatibility.
>>
>>
>> Ok, thanks. So the classifcation BUG/MAJOR can be changed for BUG/MEDIUM
>> because it impacts only the usage of SSL join with the cipherlist hash.
>> Too late :-)
>>
>
> I think it should not be a bug at all (second patch), and set of ex_data
> without reservation
> (first patch and my patch) should be the only sources of bugs.
>
I don’t known. In fact it works, so it is not a bug. But, when I use the
reservation for an ex_data slot, it returns the slot 0, and this slot is
used for the compatibility layer and can be crush some data. I conclude
that is a bug in Openssl. The reservation function must give a slot
starting to 1.
Maybe, the recommendation is to not mix the compatibility functions
like set_app_data() with *ex_data*() functions. But, I don’t see
anything about this.
thierry
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> Le 18 juin 2018 à 15:30, Thierry Fournier a
> écrit :
>
>
>
>> On 18 Jun 2018, at 14:37, Emmanuel Hocdet wrote:
>>
>>>
>>> Le 18 juin 2018 à 10:43, Thierry Fournier a
>>> écrit :
>>>
>>>
On 18 Jun 2018, at 10:33, Willy Tarreau wrote:
On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org
wrote:
> Finally, I got it ! It works with luck because we have 1 bug in Haproxy
> and 1 error (I suppose) in a OpenSSL compatibility layer.
(...)
> I join two patch. The first which fix the cipher capture must be
> backported to 1.8, for the second patch wich fix the app data
> compatibility, I dont known (at least 1.8).
Good job! I imagine you didn't have a funny week-end playing with this one
:-/
>>>
>>>
>>> Yes, including the Friday :-) But I hope this path improve stability. If
>>> someone
>>> have time and is interested by the subject, it may be interesting to see in
>>> the
>>> OpenSSL code if the slot 0 used without reservation works fine, or works
>>> because
>>> we have luck.
>>>
>>
>> It work find because slot 0 is natively reserved for old *_{set,
>> get}_app_data API compatibility.
>
>
> Ok, thanks. So the classifcation BUG/MAJOR can be changed for BUG/MEDIUM
> because it impacts only the usage of SSL join with the cipherlist hash.
> Too late :-)
>
I think it should not be a bug at all (second patch), and set of ex_data
without reservation
(first patch and my patch) should be the only sources of bugs.
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> On 18 Jun 2018, at 14:37, Emmanuel Hocdet wrote:
>
>>
>> Le 18 juin 2018 à 10:43, Thierry Fournier a
>> écrit :
>>
>>
>>> On 18 Jun 2018, at 10:33, Willy Tarreau wrote:
>>>
>>> On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org
>>> wrote:
Finally, I got it ! It works with luck because we have 1 bug in Haproxy
and 1 error (I suppose) in a OpenSSL compatibility layer.
>>> (...)
I join two patch. The first which fix the cipher capture must be
backported to 1.8, for the second patch wich fix the app data
compatibility, I dont known (at least 1.8).
>>>
>>> Good job! I imagine you didn't have a funny week-end playing with this one
>>> :-/
>>
>>
>> Yes, including the Friday :-) But I hope this path improve stability. If
>> someone
>> have time and is interested by the subject, it may be interesting to see in
>> the
>> OpenSSL code if the slot 0 used without reservation works fine, or works
>> because
>> we have luck.
>>
>
> It work find because slot 0 is natively reserved for old *_{set,
> get}_app_data API compatibility.
Ok, thanks. So the classifcation BUG/MAJOR can be changed for BUG/MEDIUM
because it impacts only the usage of SSL join with the cipherlist hash.
Too late :-)
Thierry
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> Le 18 juin 2018 à 10:43, Thierry Fournier a
> écrit :
>
>
>> On 18 Jun 2018, at 10:33, Willy Tarreau wrote:
>>
>> On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org
>> wrote:
>>> Finally, I got it ! It works with luck because we have 1 bug in Haproxy
>>> and 1 error (I suppose) in a OpenSSL compatibility layer.
>> (...)
>>> I join two patch. The first which fix the cipher capture must be
>>> backported to 1.8, for the second patch wich fix the app data
>>> compatibility, I dont known (at least 1.8).
>>
>> Good job! I imagine you didn't have a funny week-end playing with this one
>> :-/
>
>
> Yes, including the Friday :-) But I hope this path improve stability. If
> someone
> have time and is interested by the subject, it may be interesting to see in
> the
> OpenSSL code if the slot 0 used without reservation works fine, or works
> because
> we have luck.
>
It work find because slot 0 is natively reserved for old *_{set, get}_app_data
API compatibility.
++
Manu
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
Hi Thierry, Willy > Le 18 juin 2018 à 10:43, Thierry Fournier a > écrit : > > >> On 18 Jun 2018, at 10:33, Willy Tarreau wrote: >> >> On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org >> wrote: >>> Finally, I got it ! It works with luck because we have 1 bug in Haproxy >>> and 1 error (I suppose) in a OpenSSL compatibility layer. >> (...) >>> I join two patch. The first which fix the cipher capture must be >>> backported to 1.8, for the second patch wich fix the app data >>> compatibility, I dont known (at least 1.8). >> >> Good job! I imagine you didn't have a funny week-end playing with this one >> :-/ > > > Yes, including the Friday :-) But I hope this path improve stability. If > someone > have time and is interested by the subject, it may be interesting to see in > the > OpenSSL code if the slot 0 used without reservation works fine, or works > because > we have luck. > good catch! I see some strange things on my code now. I check that. ++ Manu
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
> On 18 Jun 2018, at 10:33, Willy Tarreau wrote: > > On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org wrote: >> Finally, I got it ! It works with luck because we have 1 bug in Haproxy >> and 1 error (I suppose) in a OpenSSL compatibility layer. > (...) >> I join two patch. The first which fix the cipher capture must be >> backported to 1.8, for the second patch wich fix the app data >> compatibility, I dont known (at least 1.8). > > Good job! I imagine you didn't have a funny week-end playing with this one :-/ Yes, including the Friday :-) But I hope this path improve stability. If someone have time and is interested by the subject, it may be interesting to see in the OpenSSL code if the slot 0 used without reservation works fine, or works because we have luck. BR, Thierry
Re: [PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
On Sun, Jun 17, 2018 at 09:44:50PM +0200, thierry.fourn...@arpalert.org wrote: > Finally, I got it ! It works with luck because we have 1 bug in Haproxy > and 1 error (I suppose) in a OpenSSL compatibility layer. (...) > I join two patch. The first which fix the cipher capture must be > backported to 1.8, for the second patch wich fix the app data > compatibility, I dont known (at least 1.8). Good job! I imagine you didn't have a funny week-end playing with this one :-/ Now merged, thanks, Willy
[PATCH] Re: Random crash (segfault, double free, ...) with a mix of SSL + cipherlist hash
Finally, I got it ! It works with luck because we have 1 bug in Haproxy
and 1 error (I suppose) in a OpenSSL compatibility layer.
First point is some OpenSSL macro, in the ssl.h file:
#define SSL_set_app_data(s,arg) (SSL_set_ex_data(s,0,(char*)arg))
#define SSL_get_app_data(s) (SSL_get_ex_data(s,0))
These macro uses the function SSL_get_ex_data ans SSL_set_ex_data with
the index 0, but this index is not reserved with the function
SSL_get_ex_new_index()
When I reserve a slot for the cipherlist hash, I have the number 0, this
produce a collision an the set_app_data pointer is crushed by the
cipherlist data.
Luckily, the cipherlist hash is declared in the SSL_CTX space in place
of SSL space. The first consequence was a memory leak because the
allocated struct is never cleared. The second consequence is an index
number > 0.
- With this index number > 0, the data doesn't crush app_data, but
there stored in a non-reserved storage => various crash cause
- When I fix the reservation, The app_data are crushed, and we have
systematic segfault.
I join two patch. The first which fix the cipher capture must be
backported to 1.8, for the second patch wich fix the app data
compatibility, I dont known (at least 1.8).
I join also the backports for 1.8 (there are trivial backport, with ery
minor conflict)
Thierry
On Sun, 17 Jun 2018 03:01:54 +0200
Thierry FOURNIER wrote:
> Hi,
>
> When I use SSL requests and the cipherlist hash enabled, HAProxy
> randomly crash:
>
> - segfault
> - double free
> - munmap_chunk(): invalid pointer
>
> I think that is a memory crush.
>
> I read the "cipherlist hash" code, and I put some printf, I do not
> detect any memory override.
>
> When I comment the following line, the bug disappear
>
>SSL_set_ex_data(ssl, ssl_capture_ptr_index, capture);
>
> The crash happens with many versions of openssl:
>
> - 1.0.2j (home build)
> - 1.0.1t-1+deb7u4
> - 1.0.1t-1+deb8u8
> - 1.0.2g-1ubuntu4.12
>
> cipherlist hash is available from 1.8. The bug appears with current 1.8
> and current 1.9dev.
>
> I join some files:
>
> - bug36.build.sh : build script
> - bug36.run.sh : run haproxy command
> - bug36.request.sh : curl request
> - bug36.conf : minimal conf which reproduce the problem
> - bug36.pem: ramdom self signed certificate
>
> Just execute some requests, and the bug is reproduced.
>
> BR,
> Thierry
>From a99fcb5af1a079ab5403f4cf7d2cf9345e4daf03 Mon Sep 17 00:00:00 2001
From: Thierry FOURNIER
Date: Sun, 17 Jun 2018 21:33:01 +0200
Subject: [PATCH 1/2] BUG/MAJOR: Random crash with cipherlist capture
The cipher list capture struct is stored in the SSL memory space,
but the slot is reserved in the SSL_CTX memory space. This causes
ramdom crashes.
This patch should be backported to 1.8
---
src/ssl_sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 9fb2bb151..599c8c3ec 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8805,7 +8805,7 @@ static void __ssl_sock_init(void)
#if (OPENSSL_VERSION_NUMBER >= 0x1000200fL && !defined OPENSSL_NO_TLSEXT && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER)
sctl_ex_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_sctl_free_func);
#endif
- ssl_capture_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func);
+ ssl_capture_ptr_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func);
sample_register_fetches(_fetch_keywords);
acl_register_keywords(_kws);
bind_register_keywords(_kws);
--
2.16.3
>From 54ce55a1203e732d266e72332bfea19d30f45487 Mon Sep 17 00:00:00 2001
From: Thierry FOURNIER
Date: Sun, 17 Jun 2018 21:37:05 +0200
Subject: [PATCH 2/2] BUG/MAJOR: OpenSSL context is stored in non-reserved
memory slot
We never saw unexplicated crash with SSL, so I suppose that we are
luck, or the slot 0 is always reserved. Anyway the usage of the macro
SSL_get_app_data() and SSL_set_app_data() seem wrong. This patch change
the deprecated functions SSL_get_app_data() and SSL_set_app_data()
by the new functions SSL_get_ex_data() and SSL_set_ex_data(), and
it reserves the slot in the SSL memory space.
For information, this is the two declaration which seems wrong or
incomplete in the OpenSSL ssl.h file. We can see the usage of the
slot 0 whoch is hardcoded, but never reserved.
#define SSL_set_app_data(s,arg) (SSL_set_ex_data(s,0,(char *)arg))
#define SSL_get_app_data(s) (SSL_get_ex_data(s,0))
This patch must be backported at least in 1.8, maybe in other versions.
---
src/ssl_sock.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 599c8c3ec..2707a40a9 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -260,6 +260,7 @@ struct ssl_capture {
};
struct pool_head *pool_head_ssl_capture = NULL;
static int ssl_capture_ptr_index = -1;
+static int
