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 <b...@openssl.org>
AuthorDate: Sun Feb 20 23:04:06 2000 +0000
Commit: Bodo Möller <b...@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
From 0260f9d34d81f5080bbbd602577f201922fd4062 Mon Sep 17 00:00:00 2001
From: Matt Caswell <m...@openssl.org>
Date: Fri, 25 Sep 2015 10:34:27 +0100
Subject: [PATCH] Allow interleaved app data in a renegotiation
Prior to TLS1.1 SSL3/TLS1.0 was ambigous about what to do with application
data received during a renegotiation handshake. The existing code (which
predates TLS1.1) disallows this. TLS1.1 clarifies the situation to
explicitly allow this. This patch enables interleaved app data up until the
point that the CCS is received. Whilst the RFC does not explicitly say so
it seems incorrect to allow it after this point.
---
ssl/s3_pkt.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 3798902..22a27cf 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -1608,18 +1608,14 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
* 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.
+ * started), we will indulge it. RFCs 4346 & 5246 state that app
+ * data should be allowed during the handshake without any limits
+ * specified. This seems dangerous so we only allow it up until the CCS
+ * has been received.
*/
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->renegotiate &&
+ s->s3->change_cipher_spec == 0) {
s->s3->in_read_app_data = 2;
return (-1);
} else {
--
2.1.4
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
