[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Wed May 17 09:44:36 UTC 2017
The branch master has been updated
via bd990e2535ca387def9a01218a813dc3fa547e3c (commit)
from e1cfd184dafb3e0759c567d7ca13a92b5491ff89 (commit)
- Log -----------------------------------------------------------------
commit bd990e2535ca387def9a01218a813dc3fa547e3c
Author: Matt Caswell <matt at openssl.org>
Date: Mon May 15 11:24:24 2017 +0100
Don't allow fragmented alerts
An alert message is 2 bytes long. In theory it is permissible in SSLv3 -
TLSv1.2 to fragment such alerts across multiple records (some of which
could be empty). In practice it make no sense to send an empty alert
record, or to fragment one. TLSv1.3 prohibts this altogether and other
libraries (BoringSSL, NSS) do not support this at all. Supporting it adds
significant complexity to the record layer, and its removal is unlikely
to cause inter-operability issues.
The DTLS code for this never worked anyway and it is not supported at a
protocol level for DTLS. Similarly fragmented DTLS handshake records only
work at a protocol level where at least the handshake message header
exists within the record. DTLS code existed for trying to handle fragmented
handshake records smaller than this size. This code didn't work either so
has also been removed.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3476)
-----------------------------------------------------------------------
Summary of changes:
CHANGES | 10 +++
ssl/record/rec_layer_d1.c | 155 ++++++--------------------------------
ssl/record/rec_layer_s3.c | 44 ++++-------
ssl/record/record.h | 14 ----
test/recipes/70-test_sslrecords.t | 8 +-
5 files changed, 53 insertions(+), 178 deletions(-)
diff --git a/CHANGES b/CHANGES
index 7bd0f92..9e4271d 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,16 @@
Changes between 1.1.0e and 1.1.1 [xx XXX xxxx]
+ *) Fragmented SSL/TLS alerts are no longer accepted. An alert message is 2
+ bytes long. In theory it is permissible in SSLv3 - TLSv1.2 to fragment such
+ alerts across multiple records (some of which could be empty). In practice
+ it make no sense to send an empty alert record, or to fragment one. TLSv1.3
+ prohibts this altogether and other libraries (BoringSSL, NSS) do not
+ support this at all. Supporting it adds significant complexity to the
+ record layer, and its removal is unlikely to cause inter-operability
+ issues.
+ [Matt Caswell]
+
*) Add the ASN.1 types INT32, UINT32, INT64, UINT64 and variants prefixed
with Z. These are meant to replace LONG and ZLONG and to be size safe.
The use of LONG and ZLONG is discouraged and scheduled for deprecation
diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 243eff7..487b096 100644
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -15,6 +15,7 @@
#include <openssl/buffer.h>
#include "record_locl.h"
#include <assert.h>
+#include "../packet_locl.h"
int DTLS_RECORD_LAYER_new(RECORD_LAYER *rl)
{
@@ -114,9 +115,6 @@ void DTLS_RECORD_LAYER_set_write_sequence(RECORD_LAYER *rl, unsigned char *seq)
memcpy(rl->write_sequence, seq, SEQ_NUM_SIZE);
}
-static size_t have_handshake_fragment(SSL *s, int type, unsigned char *buf,
- size_t len);
-
/* copy buffered record into SSL structure */
static int dtls1_copy_record(SSL *s, pitem *item)
{
@@ -335,7 +333,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
size_t len, int peek, size_t *readbytes)
{
int al, i, j, iret;
- size_t ret, n;
+ size_t n;
SSL3_RECORD *rr;
void (*cb) (const SSL *ssl, int type2, int val) = NULL;
@@ -352,21 +350,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
return -1;
}
- /*
- * check whether there's a handshake message (client hello?) waiting
- */
- ret = have_handshake_fragment(s, type, buf, len);
- if (ret > 0) {
- *recvd_type = SSL3_RT_HANDSHAKE;
- *readbytes = ret;
- return 1;
- }
-
- /*
- * Now s->rlayer.d->handshake_fragment_len == 0 if
- * type == SSL3_RT_HANDSHAKE.
- */
-
if (!ossl_statem_get_in_handshake(s) && SSL_in_init(s))
{
/* type == SSL3_RT_APPLICATION_DATA */
@@ -530,82 +513,23 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
* then it was unexpected (Hello Request or Client Hello).
*/
- /*
- * In case of record types for which we have 'fragment' storage, fill
- * that so that we can process the data at a fixed place.
- */
- {
- size_t k, dest_maxlen = 0;
- unsigned char *dest = NULL;
- size_t *dest_len = NULL;
-
- if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
- dest_maxlen = sizeof s->rlayer.d->handshake_fragment;
- dest = s->rlayer.d->handshake_fragment;
- dest_len = &s->rlayer.d->handshake_fragment_len;
- } else if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
- dest_maxlen = sizeof(s->rlayer.d->alert_fragment);
- dest = s->rlayer.d->alert_fragment;
- dest_len = &s->rlayer.d->alert_fragment_len;
- }
- /* else it's a CCS message, or application data or wrong */
- else if (SSL3_RECORD_get_type(rr) != SSL3_RT_CHANGE_CIPHER_SPEC) {
- /*
- * Application data while renegotiating is allowed. Try again
- * reading.
- */
- if (SSL3_RECORD_get_type(rr) == SSL3_RT_APPLICATION_DATA) {
- BIO *bio;
- s->s3->in_read_app_data = 2;
- bio = SSL_get_rbio(s);
- s->rwstate = SSL_READING;
- BIO_clear_retry_flags(bio);
- BIO_set_retry_read(bio);
- return -1;
- }
+ if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
+ unsigned int alert_level, alert_descr;
+ unsigned char *alert_bytes = SSL3_RECORD_get_data(rr)
+ + SSL3_RECORD_get_off(rr);
+ PACKET alert;
- /* Not certain if this is the right error handling */
+ if (!PACKET_buf_init(&alert, alert_bytes, SSL3_RECORD_get_length(rr))
+ || !PACKET_get_1(&alert, &alert_level)
+ || !PACKET_get_1(&alert, &alert_descr)
+ || PACKET_remaining(&alert) != 0) {
al = SSL_AD_UNEXPECTED_MESSAGE;
- SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
+ SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_INVALID_ALERT);
goto f_err;
}
- if (dest_maxlen > 0) {
- /*
- * XDTLS: In a pathological case, the Client Hello may be
- * fragmented--don't always expect dest_maxlen bytes
- */
- if (SSL3_RECORD_get_length(rr) < dest_maxlen) {
- s->rlayer.rstate = SSL_ST_READ_HEADER;
- SSL3_RECORD_set_length(rr, 0);
- goto start;
- }
-
- /* now move 'n' bytes: */
- for (k = 0; k < dest_maxlen; k++) {
- dest[k] = SSL3_RECORD_get_data(rr)[SSL3_RECORD_get_off(rr)];
- SSL3_RECORD_add_off(rr, 1);
- SSL3_RECORD_add_length(rr, -1);
- }
- *dest_len = dest_maxlen;
- }
- }
-
- /*-
- * s->rlayer.d->handshake_fragment_len == 12 iff rr->type == SSL3_RT_HANDSHAKE;
- * s->rlayer.d->alert_fragment_len == 7 iff rr->type == SSL3_RT_ALERT.
- * (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
- */
-
- 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];
-
- s->rlayer.d->alert_fragment_len = 0;
-
if (s->msg_callback)
- s->msg_callback(0, s->version, SSL3_RT_ALERT,
- s->rlayer.d->alert_fragment, 2, s,
+ s->msg_callback(0, s->version, SSL3_RT_ALERT, alert_bytes, 2, s,
s->msg_callback_arg);
if (s->info_callback != NULL)
@@ -686,17 +610,22 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
/*
* Unexpected handshake message (Client Hello, or protocol violation)
*/
- if ((s->rlayer.d->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH) &&
- !ossl_statem_get_in_handshake(s)) {
+ if ((SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) &&
+ !ossl_statem_get_in_handshake(s)) {
struct hm_header_st msg_hdr;
- /* this may just be a stale retransmit */
- dtls1_get_message_header(rr->data, &msg_hdr);
- if (SSL3_RECORD_get_epoch(rr) != s->rlayer.d->r_epoch) {
+ /*
+ * This may just be a stale retransmit. Also sanity check that we have
+ * at least enough record bytes for a message header
+ */
+ if (SSL3_RECORD_get_epoch(rr) != s->rlayer.d->r_epoch
+ || SSL3_RECORD_get_length(rr) < DTLS1_HM_HEADER_LENGTH) {
SSL3_RECORD_set_length(rr, 0);
goto start;
}
+ dtls1_get_message_header(rr->data, &msg_hdr);
+
/*
* If we are server, we may have a repeated FINISHED of the client
* here, then retransmit our CCS and FINISHED.
@@ -756,11 +685,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
switch (SSL3_RECORD_get_type(rr)) {
default:
- /* TLS just ignores unknown message types */
- if (s->version == TLS1_VERSION) {
- SSL3_RECORD_set_length(rr, 0);
- goto start;
- }
al = SSL_AD_UNEXPECTED_MESSAGE;
SSLerr(SSL_F_DTLS1_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
goto f_err;
@@ -802,39 +726,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
}
/*
- * this only happens when a client hello is received and a handshake
- * is started.
- */
-static size_t have_handshake_fragment(SSL *s, int type, unsigned char *buf,
- size_t len)
-{
-
- if ((type == SSL3_RT_HANDSHAKE)
- && (s->rlayer.d->handshake_fragment_len > 0))
- /* (partially) satisfy request from storage */
- {
- unsigned char *src = s->rlayer.d->handshake_fragment;
- unsigned char *dst = buf;
- size_t k, n;
-
- /* peek == 0 */
- n = 0;
- while ((len > 0) && (s->rlayer.d->handshake_fragment_len > 0)) {
- *dst++ = *src++;
- len--;
- s->rlayer.d->handshake_fragment_len--;
- n++;
- }
- /* move any remaining fragment bytes: */
- for (k = 0; k < s->rlayer.d->handshake_fragment_len; k++)
- s->rlayer.d->handshake_fragment[k] = *src++;
- return n;
- }
-
- return 0;
-}
-
-/*
* Call this to write data in records of type 'type' It will return <= 0 if
* not all data has been sent or non-blocking IO.
*/
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index de112cc..dabb02c 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -17,6 +17,7 @@
#include <openssl/buffer.h>
#include <openssl/rand.h>
#include "record_locl.h"
+#include "../packet_locl.h"
#if defined(OPENSSL_SMALL_FOOTPRINT) || \
!( defined(AES_ASM) && ( \
@@ -47,8 +48,6 @@ void RECORD_LAYER_clear(RECORD_LAYER *rl)
rl->packet = NULL;
rl->packet_length = 0;
rl->wnum = 0;
- memset(rl->alert_fragment, 0, sizeof(rl->alert_fragment));
- rl->alert_fragment_len = 0;
memset(rl->handshake_fragment, 0, sizeof(rl->handshake_fragment));
rl->handshake_fragment_len = 0;
rl->wpend_tot = 0;
@@ -1402,10 +1401,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
dest_maxlen = sizeof s->rlayer.handshake_fragment;
dest = s->rlayer.handshake_fragment;
dest_len = &s->rlayer.handshake_fragment_len;
- } else if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
- dest_maxlen = sizeof s->rlayer.alert_fragment;
- dest = s->rlayer.alert_fragment;
- dest_len = &s->rlayer.alert_fragment_len;
}
if (dest_maxlen > 0) {
@@ -1422,20 +1417,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
if (SSL3_RECORD_get_length(rr) == 0)
SSL3_RECORD_set_read(rr);
- if (SSL_IS_TLS13(s)
- && SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
- if (*dest_len < dest_maxlen
- || SSL3_RECORD_get_length(rr) != 0) {
- /*
- * TLSv1.3 forbids fragmented alerts, and only one alert
- * may be present in a record
- */
- al = SSL_AD_UNEXPECTED_MESSAGE;
- SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INVALID_ALERT);
- goto f_err;
- }
- }
-
if (*dest_len < dest_maxlen)
goto start; /* fragment was too small */
}
@@ -1443,7 +1424,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
/*-
* s->rlayer.handshake_fragment_len == 4 iff rr->type == SSL3_RT_HANDSHAKE;
- * s->rlayer.alert_fragment_len == 2 iff rr->type == SSL3_RT_ALERT.
* (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
*/
@@ -1466,15 +1446,23 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
goto start;
}
- if (s->rlayer.alert_fragment_len >= 2) {
- int alert_level = s->rlayer.alert_fragment[0];
- int alert_descr = s->rlayer.alert_fragment[1];
-
- s->rlayer.alert_fragment_len = 0;
+ if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
+ unsigned int alert_level, alert_descr;
+ unsigned char *alert_bytes = SSL3_RECORD_get_data(rr)
+ + SSL3_RECORD_get_off(rr);
+ PACKET alert;
+
+ if (!PACKET_buf_init(&alert, alert_bytes, SSL3_RECORD_get_length(rr))
+ || !PACKET_get_1(&alert, &alert_level)
+ || !PACKET_get_1(&alert, &alert_descr)
+ || PACKET_remaining(&alert) != 0) {
+ al = SSL_AD_UNEXPECTED_MESSAGE;
+ SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INVALID_ALERT);
+ goto f_err;
+ }
if (s->msg_callback)
- s->msg_callback(0, s->version, SSL3_RT_ALERT,
- s->rlayer.alert_fragment, 2, s,
+ s->msg_callback(0, s->version, SSL3_RT_ALERT, alert_bytes, 2, s,
s->msg_callback_arg);
if (s->info_callback != NULL)
diff --git a/ssl/record/record.h b/ssl/record/record.h
index 6880f77..32db821 100644
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -111,14 +111,6 @@ typedef struct dtls_record_layer_st {
* loss.
*/
record_pqueue buffered_app_data;
- /*
- * storage for Alert/Handshake protocol data received but not yet
- * processed by ssl3_read_bytes:
- */
- unsigned char alert_fragment[DTLS1_AL_HEADER_LENGTH];
- size_t alert_fragment_len;
- unsigned char handshake_fragment[DTLS1_HM_HEADER_LENGTH];
- size_t handshake_fragment_len;
/* save last and current sequence numbers for retransmissions */
unsigned char last_write_sequence[8];
unsigned char curr_write_sequence[8];
@@ -157,12 +149,6 @@ typedef struct record_layer_st {
size_t packet_length;
/* number of bytes sent so far */
size_t wnum;
- /*
- * storage for Alert/Handshake protocol data received but not yet
- * processed by ssl3_read_bytes:
- */
- unsigned char alert_fragment[2];
- size_t alert_fragment_len;
unsigned char handshake_fragment[4];
size_t handshake_fragment_len;
/* The number of consecutive empty records we have received */
diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t
index 99b0181..bac738c 100644
--- a/test/recipes/70-test_sslrecords.t
+++ b/test/recipes/70-test_sslrecords.t
@@ -59,14 +59,14 @@ $proxy->serverflags("-tls1_2");
$proxy->start();
ok(TLSProxy::Message->fail(), "Too many in context empty records test");
-#Test 4: Injecting a fragmented fatal alert should fail. We actually expect no
-# alerts to be sent from either side because *we* injected the fatal
-# alert, i.e. this will look like a disorderly close
+#Test 4: Injecting a fragmented fatal alert should fail. We expect the server to
+# send back an alert of its own because it cannot handle fragmented
+# alerts
$proxy->clear();
$proxy->filter(\&add_frag_alert_filter);
$proxy->serverflags("-tls1_2");
$proxy->start();
-ok(!TLSProxy::Message->end(), "Fragmented alert records test");
+ok(TLSProxy::Message->fail(), "Fragmented alert records test");
#Run some SSLv2 ClientHello tests
More information about the openssl-commits
mailing list