[openssl-dev] TLS session ticket extension problem when using the ssl23_client_hello method

[Jouni Malinen]

On Thu, Jul 30, 2015 at 11:00:45AM +0100, Matt Caswell wrote:
> On 28/07/15 15:09, Jouni Malinen wrote:
> > The remaining issue for EAP-FAST server is in the
> > SSL_set_session_secret_cb() callback not having access to the correct
> > server_random through SSL_get_server_random().

> Is this still a problem? From looking at the code it seems to me that
> the server random is set prior to calling the callback:

It is a problem with the current master branch snapshot, but not with
older versions, i.e., a regression of some sort..

>     /*
>      * Check if we want to use external pre-shared secret for this handshake
>      * for not reused session only. We need to generate server_random before
>      * calling tls_session_secret_cb in order to allow SessionTicket
>      * processing to use it in key derivation.
>      */
>     {
>         unsigned char *pos;
>         pos = s->s3->server_random;
>         if (ssl_fill_hello_random(s, 1, pos, SSL3_RANDOM_SIZE) <= 0) {

This is indeed the server_random I see in the tls_session_secret_cb, but
that is not the server_random that gets used in the negotiation..

> Checking the commit logs this seems to have been put in by this commit
> responding to one of your tickets!
> 
> commit 12bf56c017a34bd0d5fc6d817564ae49d0a9e861

It was indeed and that commit worked.. Please note that it uses #ifdef
OPENSSL_NO_TLSEXT to avoid having ssl3_send_server_hello() override the
previously selected server_random.

> You seem to imply that you can get the server_random through
> ssl->s3->server_random but not through SSL_get_server_random(). Looking
> at the code I can't see an obvious reason why that would be the case.
> Here is SSL_get_server_random():
> 
> size_t SSL_get_server_random(const SSL *ssl, unsigned char *out, size_t
> outlen)
> {
>     if (outlen == 0)
>         return sizeof(ssl->s3->server_random);
>     if (outlen > sizeof(ssl->s3->server_random))
>         outlen = sizeof(ssl->s3->server_random);
>     memcpy(out, ssl->s3->server_random, outlen);
>     return outlen;
> }

I had not checked what this does, but yes, that's indeed identical to
the mechanism I use with older OpenSSL version. In other words, the
issue is in ssl3_send_server_hello().

It looks like commit e481f9b90b164fd1053015d1c4e0a0d92076d7a8
("Remove support for OPENSSL_NO_TLSEXT") broke this. It is deleting
number of "#ifndef OPENSSL_NO_TLSEXT" lines correctly, but it is also
deleting one "#ifdef OPENSSL_NO_TLSEXT" without removing the block of
code that should have also been removed from ssl3_send_server_hello().
Because of that, server_random gets replaced after the call to
tls_session_secret_cb which breaks the EAP-FAST use case.

This is the relevant part of that commit:

@@ -1602,13 +1585,13 @@ int ssl3_send_server_hello(SSL *s)
 
     if (s->state == SSL3_ST_SW_SRVR_HELLO_A) {
         buf = (unsigned char *)s->init_buf->data;
-#ifdef OPENSSL_NO_TLSEXT
+
         p = s->s3->server_random;
         if (ssl_fill_hello_random(s, 1, p, SSL3_RANDOM_SIZE) <= 0) {
             s->state = SSL_ST_ERR;
             return -1;
         }
-#endif
+
         /* Do the message type and length last */
         d = p = ssl_handshake_start(s);
 

That ssl_fill_hello_random() call needs to be deleted to fix this issue.
Based on a quick test, that does indeed fix the EAP-FAST server issue I
saw.

-- 
Jouni Malinen                                            PGP id EFC895FA