[openssl-users] Authentication over ECDHE

[C.Wehrmeyer]

I don't have access to the actual testing environments until Wednesday 
next year, so I've had to create a private account.

 > Which version of OpenSSL is this? (I don't remember if you said this
 > already).

I'm not entirely sure, but I *think* it's 1.1.0.

=====================================================================

OK, so I've been reading the mails before going to sleep and spent some 
time thinking and researching about this, and I've come to a conclusion: 
OpenSSL is a goddamn mess, SSL_clear() is pretty much superfluous, and 
as such shouldn't exist.

Why? Well, to quote Viktor here:

 > DO NOT reuse the same SSL handle for multiple connections,

And that is fricking bullshit. Not the quote itself or the suggestion - 
it's unlikely you had anything to do with the actual code - but the way 
things have been thought through (or rather, have not been thought 
through) by the library devs. I've written highly scalable libraries in 
the past before, and one thing you always want to do there is to trim 
fat. And "object allocation and initialisation" is something that you 
very much want to trim fat of, not only for obvious reasons such as 
malloc() and free() (or whatever OpenSSL uses as wrappers) being 
complexity monsters, but also for cache reasons (loading different cache 
line hurts performance). That's why you usually have functions like 
XXX_clear() or XXX_reset(), which do exactly that - prepare an object 
for another usage. memset() (or the OpenSSL equivalent of a secure 
memset) your allocated resources. I don't really see the problem here.

Now add to that the fact that OpenSSL has been moving towards making its 
structures opaque, thus falling into the same trap that Microsoft has 
with COM and DirectX, and you can kind of see why, if you can't really 
determine anymore WHERE your object is going to be stored, you at least 
want to keep reusing it. This is not PHP, where people allocate memory 
all willy-nilly, or C++, where people don't even have shame anymore to 
use std::vector<std::strings> str_array instead of good old static const 
char*const str_array[] while expecting things to be made faster by 
invisible memory pools (and horribly failing at it), but C, where you 
want to think about each step quite carefully.

Then OpenSSL even provides an SSL_clear function which is advertised 
like this:

 > SSL_clear - reset SSL object to allow another connection

, and then, only later, in a big warning block, decides to tell the 
reader that this function only works when the stars align quite 
correctly and you've sacrificed at least two virgins, because:

 > The reset operation however keeps several settings of the last
 > sessions

Then, as the documentation suggests, I read the entry for SSL_get_session:

 > The ssl session contains all information required to re-establish the
 > connection without a full handshake for SSL versions up to and
 > including TLSv1.2. In TLSv1.3 the same is true, but sessions are
 > established after the main handshake has occurred.

And at this point it all falls apart. From my understanding OpenSSL 
keeps a session cache for servers so that key exchanges and protocol 
handshakes can be avoided. Problem is, *we're using ECDHE, where the 
last E stands for "ephemeral"*. In simple English: throw away the keys 
after you're done, we want to have forward secrecy. And then OpenSSL 
keeps a fresh copy of those for everyone who happened to be logged on at 
this point. Heartbleed apparently wasn't enough of a warning. Oh, but 
lets move everything to the heap so that it's more secure there now.

I don't want to reuse a session with ephemeral keys; I want to reuse an 
object that is supposed to already have resources allocated for doing 
its job, as is indicated by the documentation of this function except 
for a small note at the end that tells you that the devs didn't really 
think about what "ephemeral" means.

Creating a new SSL object (EVEN FROM AN EXISTING SSL_CTX object) entails:

- allocating the memory for the object itself on the heap (via 
OPENSSL_zalloc)
- creating and managing a new lock for the object, and who knows for 
much more subobjects
- creating a duplicate of the cipher suite stack (which isn't even a 
flat copy, but something that can cause the code to call OPENSSL_malloc 
*twice* in the worst case)
- creating a duplicate of the certificates (which I don't even use, but 
that doesn't stop the code of ssl_cert_dup() to call OPENSSL_zalloc *in 
its very first line!*)
- setting up a bunch of callbacks
- copying 32 bytes for a sid_ctx
- creating an X509_VERIFY_PARAM object (*which calls OPENSSL_zalloc 
again*) as well as creating a deep copy of the SSL_CTX's parameter via 
X509_VERIFY_PARAM_inherit(), with Thor knows how many copies hidden in 
all those *set* and *deep_copy* routines
- copying EC point formats from the context - deep again, of course, at 
least that's what OPENSSL_memdup() makes me think
- copying supported group informations, and of course deep again!
- deep-copying an ALPN object
- SSL_clear()-ing the object (no, really!)
- deep-copying a CRYPTO_EX_DATA object via CRYPTO_new_ex_data ... at 
this point, is anyone surprised here that timing attacks against crypto 
are *still* so successful? Because I'm not. Not at all.

I didn't bother looking up what freeing entails - it's obvious to anyone 
at this point that OpenSSL is a severe victim of feature creep, that its 
memory allocation scheme is a mess, and long story short: I will NOT 
free a perfectly fine object just because of incompetent devs' chutzpah 
expecting their users to allocate memory dynamically en mass for no 
goddamn reason whenever a new connection comes in. Fix your goddamn code.

And don't give me any "trust us, we're experienced programmers" 
bullshit. I've *seen* ssl/record/ssl3_record.c:

 > static const unsigned char ssl3_pad_1[48] = {
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36,
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36,
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36,
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36,
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36,
 >     0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36, 0x36
 > };
 > static const unsigned char ssl3_pad_2[48] = {
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
 >     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c
 > };

What's wrong with that, you ask? Let me show you how I'd have done that:

 > static const unsigned char ssl3_pad_1[] =
 > {
 >     "66666666"
 >     "66666666"
 >     "66666666"
 >     "66666666"
 >     "66666666"
 >     "66666666"
 > };
 >
 > static const unsigned char*ssl3_pad_2[] =
 > {
 >     "\\\\\\\\\\\\\\\\"
 >     "\\\\\\\\\\\\\\\\"
 >     "\\\\\\\\\\\\\\\\"
 >     "\\\\\\\\\\\\\\\\"
 >     "\\\\\\\\\\\\\\\\"
 >     "\\\\\\\\\\\\\\\\"
 > };

So, no. I don't trust anyone. Especially not this mess of a code.