From 12492580ffd561764111b5efbafde17125b91e92 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 29 Jan 2018 14:55:44 +0000 Subject: [PATCH] Make sure we check an incoming reneg ClientHello in DTLS In TLS we have a check to make sure an incoming reneg ClientHello is acceptable. The equivalent check is missing in the DTLS code. This means that if a client does not signal the ability to handle secure reneg in the initial handshake, then a subsequent reneg handshake should be rejected by the server. In the DTLS case the reneg was being allowed if the the 2nd ClientHello had a renegotiation_info extension. This is incorrect. While incorrect, this does not represent a security issue because if the renegotiation_info extension is present in the second ClientHello it also has to be *correct*. Therefore this will only work if both the client and server believe they are renegotiating, and both know the previous Finished result. This is not the case in an insecure rengotiation attack. I have also tidied up the check in the TLS code and given a better check for determining whether we are renegotiating or not. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/5191) --- ssl/record/rec_layer_d1.c | 17 +++++++++++++++++ ssl/record/rec_layer_s3.c | 19 +++++++++---------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index f674b79892..e7c481443b 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -686,6 +686,23 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, goto start; } + /* + * If we are a server and get a client hello when renegotiation isn't + * allowed send back a no renegotiation alert and carry on. + */ + if (s->server + && SSL_is_init_finished(s) + && !s->s3->send_connection_binding + && s->rlayer.d->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH + && s->rlayer.d->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO + && s->s3->previous_client_finished_len != 0 + && (s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) == 0) { + s->rlayer.d->handshake_fragment_len = 0; + SSL3_RECORD_set_length(rr, 0); + ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); + goto start; + } + if (s->rlayer.d->alert_fragment_len >= DTLS1_AL_HEADER_LENGTH) { int alert_level = s->rlayer.d->alert_fragment[0]; int alert_descr = s->rlayer.d->alert_fragment[1]; diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index ce3f93d2ae..0aad575121 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1330,17 +1330,16 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, } /* * If we are a server and get a client hello when renegotiation isn't - * allowed send back a no renegotiation alert and carry on. WARNING: - * experimental code, needs reviewing (steve) + * allowed send back a no renegotiation alert and carry on. */ - if (s->server && - SSL_is_init_finished(s) && - !s->s3->send_connection_binding && - (s->version > SSL3_VERSION) && - (s->rlayer.handshake_fragment_len >= 4) && - (s->rlayer.handshake_fragment[0] == SSL3_MT_CLIENT_HELLO) && - (s->session != NULL) && (s->session->cipher != NULL) && - !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) { + if (s->server + && SSL_is_init_finished(s) + && !s->s3->send_connection_binding + && s->version > SSL3_VERSION + && s->rlayer.handshake_fragment_len >= SSL3_HM_HEADER_LENGTH + && s->rlayer.handshake_fragment[0] == SSL3_MT_CLIENT_HELLO + && s->s3->previous_client_finished_len != 0 + && (s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) == 0) { SSL3_RECORD_set_length(rr, 0); SSL3_RECORD_set_read(rr); ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION); -- 2.25.1