On 12/10/15 17:19, Matt Caswell via RT wrote:
>
>
> On 12/10/15 16:39, Matt Caswell via RT wrote:
>>
>>
>> On 12/10/15 16:03, Alessandro Ghedini via RT wrote:
>>> On Mon, Oct 12, 2015 at 01:45:20PM +0000, Hubert Kario via RT wrote:
>>>> On Friday 09 October 2015 18:05:19 Matt Caswell via RT wrote:
>>>>> On 09/10/15 19:02, Hubert Kario via RT wrote:
>>>>>> And for good measure, I also created a test script that
>>>>>> combines fragmentation with interleaving.
>>>>>
>>>>> Did you try my patch with it? And if so what happened?
>>>>
>>>> I'm using interleave-data-102.patch attached to this ticket.
>>>>
>>>> So, for state-machine-rewrite branch it doesn't apply, there's no
>>>> ssl/s3_pkt.c file.
>>>>
>>>> For current 1.0.1 branch, the patch applies, test case results are as
>>>> follows:
>>>> * test-openssl-3712.py - pass
>>>> * test-interleaved-application-data-in-renegotiation.py - pass
>>>> * test-interleaved-application-data-and-fragmented-handshakes-in-
>>>> renegotiation.py - pass
>>>>
>>>> For current 1.0.2 branch, the patch applies, tests case results are as
>>>> follows:
>>>> * test-openssl-3712.py - pass
>>>> * test-interleaved-application-data-in-renegotiation.py - pass
>>>> * test-interleaved-application-data-and-fragmented-handshakes-in-
>>>> renegotiation.py - pass
>>>>
>>>> for current master the patch doesn't apply, just like with state-
>>>> machine-rewrite there's no ssl/s3_pkt.c file
>>>>
>>>> Note: the two latter test cases need the s_server run in -www mode, the
>>>> first test case ignores server response so will work regardless, that
>>>> may be why Alessandro testing doesn't show the issue as fixed
>>>
>>> Ah, yep, with -www it works for me too. Note that on master the file to
>>> change
>>> should be ssl/record/ssl3_record.c. However, while the patch applies
>>> cleanly to
>>> this file, all the tests fail (even with -www). It seems that the field
>>> in_read_app_data is never true, so the UNEXPECTED_MESSAGE alert is sent.
>>
>> The value of "in_read_app_data" not being true when it is supposed to
>> appears to be running into a slightly different bug. It's also present
>> in 1.0.2 but you have to switch off version negotiation. So running
>> s_server like this in 1.0.2 and rerunning Hubert's test will hit it:
>>
>> openssl s_server -www -tls1_2
>>
>> The 1.0.2 version negotiation is hiding the issue. In master version neg
>> has been completely rewritten and simplified - but in doing so no longer
>> hides the problem. :-(
>
> Having done some more digging it seems the problem only occurs if you
> get the initial handshake, following by a second reneg handshake *and*
> interleaved app data all within the scope of a *single* SSL_read call.
> AFAICT if SSL_read returns between the first handshake and the second,
> you don't get the problem.
>
> That's starting to sound like quite an unlikely scenario and we're only
> hitting it now because of the slightly artificial nature of Hubert's
> test. I'm wondering whether "will not fix" is the right response to this
> second bug? Thoughts? Having said that it would be nice to have a
> reliable test for the interleaved-app data issue.
Ok, updated version of the patch attached. This is for 1.0.2 but should
pass Hubert's tests even when you run s_server with "-tls1_2".
Matt
>From af12b252a13444fa7cb072b6cba172716304b289 Mon Sep 17 00:00:00 2001
From: Matt Caswell <m...@openssl.org>
Date: Fri, 25 Sep 2015 10:34:27 +0100
Subject: [PATCH 1/2] 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.
RT#3712
---
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
>From 75caa967e318e61c60e5cbfbaa40aa373811ee18 Mon Sep 17 00:00:00 2001
From: Matt Caswell <m...@openssl.org>
Date: Tue, 13 Oct 2015 10:01:14 +0100
Subject: [PATCH 2/2] Force initial handshake in SSL_read
If we're called via SSL_read but we haven't done the initial handshake yet
then, previously, we would set |in_read_app_data| to 1 and then call
ssl_read_bytes. If ssl_read_bytes encounters handshake records then it will
process them. If it encounters interleaved app data then we call
ssl_read_bytes a second time to process them.
However during the initial handshake SSL_clear will get called. That also
clears the value of |in_read_app_data|. This isn't a problem in itself since
we should never have interleaved app data on the initial handshake anyway.
However it becomes a problem if we don't receive any app data and don't
return from SSL_read before initiating a renegotiation. In that instance
|in_read_app_data| will still be 0 and hence we will fail to correctly
process interleaved app data.
The solution is to force the initial handshake immediately if it has not
yet taken place before the first call to ssl_read_bytes.
---
ssl/s3_lib.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index ad9eeb6..1d64c7f 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4412,6 +4412,19 @@ static int ssl3_read_internal(SSL *s, void *buf, int len, int peek)
clear_sys_error();
if (s->s3->renegotiate)
ssl3_renegotiate_check(s);
+
+ /*
+ * If we're about to do the initial handshake, then force it now. Otherwise
+ * we leave it to ssl_read_bytes because we might get interleaved app data.
+ * The initial handshake cannot have interleaved app data and also calls
+ * SSL_clear() so we cannot rely on the |in_read_app_data| value anyway.
+ */
+ if (SSL_in_init(s) && (!s->in_handshake) && (s->renegotiate == 0)) {
+ ret = s->handshake_func(s);
+ if (ret <= 0)
+ return ret;
+ }
+
s->s3->in_read_app_data = 1;
ret =
s->method->ssl_read_bytes(s, SSL3_RT_APPLICATION_DATA, buf, len,
--
2.1.4
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
