[openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken

[Matt Caswell]

On 24/09/15 21:40, Hubert Kario via RT wrote:
> I have made the reproducer cleaner and it should use relatively stable 
> API's of tlsfuzzer now.
> 
> openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt\
> -nodes -batch
> ~/dev/openssl/apps/openssl s_server -key localhost.key -cert\
> localhost.crt
> 
> pip install --pre tlslite-ng
> git clone https://github.com/tomato42/tlsfuzzer.git
> 
> cd tlsfuzzer
> PYTHONPATH=. python scripts/test-openssl-3712.py

This is really nice! Thanks Hubert.

I've been looking into this issue. The reason this fails is because at
some point in the past there has been an explicit design decision to
disallow it. The relevant code is here:

        /*
         * At this point, we were expecting handshake data, but have
         * application data.  If the library was running inside ssl3_read()
         * (i.e. in_read_app_data is set) and it makes sense to read
         * application data at this point (session renegotiation not yet
         * started), we will indulge it.
         */
        if (s->s3->in_read_app_data &&
            (s->s3->total_renegotiations != 0) &&
            (((s->state & SSL_ST_CONNECT) &&
              (s->state >= SSL3_ST_CW_CLNT_HELLO_A) &&
              (s->state <= SSL3_ST_CR_SRVR_HELLO_A)
             ) || ((s->state & SSL_ST_ACCEPT) &&
                   (s->state <= SSL3_ST_SW_HELLO_REQ_A) &&
                   (s->state >= SSL3_ST_SR_CLNT_HELLO_A)
             )
            )) {
            s->s3->in_read_app_data = 2;
            return (-1);
        } else {
            al = SSL_AD_UNEXPECTED_MESSAGE;
            SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
            goto f_err;
        }

So to pull this conditional apart a bit:

s->s3->in_read_app_data : we were originally called by SSL_read

AND

s->s3->total_renegotiations != 0 : we have started at least one
renegotiation at some point (*see below)

AND

(
  ((s->state & SSL_ST_CONNECT) &&
  (s->state >= SSL3_ST_CW_CLNT_HELLO_A) &&
  (s->state <= SSL3_ST_CR_SRVR_HELLO_A) : we are a client and are in a
  state where we have started to write/have finished writing a
  ClientHello out but have not yet received a ServerHello.

  OR

  (s->state & SSL_ST_ACCEPT) &&
  (s->state <= SSL3_ST_SW_HELLO_REQ_A) &&
  (s->state >= SSL3_ST_SR_CLNT_HELLO_A) : we are a server and are about
to write a HelloVerifyRequest, or we are in the process of reading a
ClientHello

)


Aside from whether this is the correct logic or not I think there is a
related bug here in the total_renegotiations value. I believe this is
intended to count up the number of renegotiations that have occurred. As
far as I can determine this is incremented when:
- There is an explicit call to SSL_renegotiate() or
SSL_renegotiate_abbreviated()
OR
- As a client we initiate a renegotiation following receipt of a
HelloRequest

So it does not seem to get incremented when, as a server and after the
initial handshake has completed, we receive an unsolicited ClientHello,
i.e. client initiated renegotiation as in your reproducer code. This
alone is sufficient for your test case to fail, but even fixing it won't
solve the problem due to the intent of the logic above.

The above logic can be traced back to this commit:

commit b35e9050f282c5ea2164bd5b08ed34d03accf45f
Author:     Bodo Möller <bodo at openssl.org>
AuthorDate: Sun Feb 20 23:04:06 2000 +0000
Commit:     Bodo Möller <bodo at openssl.org>
CommitDate: Sun Feb 20 23:04:06 2000 +0000

    Tolerate fragmentation and interleaving in the SSL 3/TLS record layer.


Note the date of February 2000! The OP correctly cites RFC 5246 (TLS1.2)
and RFC 4346 (TLS1.1) as follows:

   Note: Data of different TLS Record layer content types MAY be
   interleaved.  Application data is generally of lower precedence for
   transmission than other content types.  However, records MUST be
   delivered to the network in the same order as they are protected by
   the record layer.  Recipients MUST receive and process interleaved
   application layer traffic during handshakes subsequent to the first
   one on a connection.

As the OP also observes:
"The last sentence clearly puts the blame on OpenSSL.
This seems to be an addition in TLS 1.1 since it cannot be found in RFC
2246."

Clearly the code commit pre-dates the publication of TLS1.1 (April 2006)
and is a carry over from the ambiguous situation as it is in TLS1.0.
Therefore this does seem to be an OpenSSL bug.

However, I have some concerns with the wording of the RFC. It seems to
place no limits whatsoever on when it is valid to receive app data in
the handshake. By the wording in the RFC it would be valid for app data
to be received *after* the ChangeCipherSpec has been received but
*before* the Finished has been processed. This seems dangerous to me
because it is not until the Finished is processed that we verify the
handshake data MAC - and yet we could already have acted upon app data
received. I assume the intent was to allow the interleaved app data only
up until the point that the CCS is received. I have attached a patch for
1.0.2 that implements that logic.

Matt

-------------- next part --------------
A non-text attachment was scrubbed...
Name: interleave-data-102.patch
Type: text/x-patch
Size: 2086 bytes
Desc: not available
URL: <http://mta.openssl.org/pipermail/openssl-dev/attachments/20150925/481db123/attachment.bin>