From: Matt Caswell Date: Wed, 1 Jun 2016 15:31:11 +0000 (+0100) Subject: Reject out of context empty records X-Git-Tag: OpenSSL_1_1_0-pre6~518 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=255cfeacd88bcba13688da17fab72b344a78d24f Reject out of context empty records Previously if we received an empty record we just threw it away and ignored it. Really though if we get an empty record of a different content type to what we are expecting then that should be an error, i.e. we should reject out of context empty records. This commit makes the necessary changes to achieve that. RT#4395 Reviewed-by: Andy Polyakov --- diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index a656b56cf2..73260767d1 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1057,9 +1057,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, goto f_err; } } - /* Skip over any records we have already used or are zero in length */ + /* Skip over any records we have already read */ for (curr_rec = 0; - curr_rec < num_recs && SSL3_RECORD_get_length(&rr[curr_rec]) == 0; + curr_rec < num_recs && SSL3_RECORD_is_read(&rr[curr_rec]); curr_rec++); if (curr_rec == num_recs) { RECORD_LAYER_set_numrpipes(&s->rlayer, 0); @@ -1137,6 +1137,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (SSL3_RECORD_get_length(rr) == 0) { s->rlayer.rstate = SSL_ST_READ_HEADER; SSL3_RECORD_set_off(rr, 0); + SSL3_RECORD_set_read(rr); } } if (SSL3_RECORD_get_length(rr) == 0 @@ -1147,6 +1148,10 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, read_bytes += n; } while (type == SSL3_RT_APPLICATION_DATA && curr_rec < num_recs && read_bytes < (unsigned int)len); + if (read_bytes == 0) { + /* We must have read empty records. Get more data */ + goto start; + } if (!peek && curr_rec == num_recs && (s->mode & SSL_MODE_RELEASE_BUFFERS) && SSL3_BUFFER_get_left(rbuf) == 0) diff --git a/ssl/record/record.h b/ssl/record/record.h index 9e198224fc..cda4eff0d3 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -65,6 +65,10 @@ typedef struct ssl3_record_st { /* r */ unsigned char *comp; + /* Whether the data from this record has already been read or not */ + /* r */ + unsigned int read; + /* epoch number, needed by DTLS1 */ /* r */ unsigned long epoch; @@ -181,6 +185,9 @@ typedef struct record_layer_st { unsigned char handshake_fragment[4]; unsigned int handshake_fragment_len; + /* The number of consecutive empty records we have received */ + unsigned int empty_record_count; + /* partial write - check the numbers match */ /* number bytes written */ int wpend_tot; diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h index 67ae1f4290..ff1eb32191 100644 --- a/ssl/record/record_locl.h +++ b/ssl/record/record_locl.h @@ -27,6 +27,10 @@ #define RECORD_LAYER_get_write_sequence(rl) ((rl)->write_sequence) #define RECORD_LAYER_get_numrpipes(rl) ((rl)->numrpipes) #define RECORD_LAYER_set_numrpipes(rl, n) ((rl)->numrpipes = (n)) +#define RECORD_LAYER_inc_empty_record_count(rl) ((rl)->empty_record_count++) +#define RECORD_LAYER_reset_empty_record_count(rl) \ + ((rl)->empty_record_count = 0) +#define RECORD_LAYER_get_empty_record_count(rl) ((rl)->empty_record_count) #define DTLS_RECORD_LAYER_get_r_epoch(rl) ((rl)->d->r_epoch) __owur int ssl3_read_n(SSL *s, int n, int max, int extend, int clearold); @@ -89,6 +93,8 @@ int ssl3_release_write_buffer(SSL *s); #define SSL3_RECORD_get_epoch(r) ((r)->epoch) #define SSL3_RECORD_is_sslv2_record(r) \ ((r)->rec_version == SSL2_VERSION) +#define SSL3_RECORD_is_read(r) ((r)->read) +#define SSL3_RECORD_set_read(r) ((r)->read = 1) void SSL3_RECORD_clear(SSL3_RECORD *r, unsigned int num_recs); void SSL3_RECORD_release(SSL3_RECORD *r, unsigned int num_recs); diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 872a3812aa..d3b2bea40a 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -134,7 +134,6 @@ int ssl3_get_record(SSL *s) unsigned char md[EVP_MAX_MD_SIZE]; short version; unsigned mac_size; - unsigned empty_record_count = 0, curr_empty = 0; unsigned int num_recs = 0; unsigned int max_recs; unsigned int j; @@ -146,7 +145,6 @@ int ssl3_get_record(SSL *s) max_recs = 1; sess = s->session; - again: do { /* check if we have the header */ if ((RECORD_LAYER_get_rstate(&s->rlayer) != SSL_ST_READ_BODY) || @@ -323,6 +321,10 @@ int ssl3_get_record(SSL *s) /* decrypt in place in 'rr->input' */ rr[num_recs].data = rr[num_recs].input; rr[num_recs].orig_len = rr[num_recs].length; + + /* Mark this record as not read by upper layers yet */ + rr[num_recs].read = 0; + num_recs++; /* we have pulled in a full packet so zero things */ @@ -486,21 +488,17 @@ int ssl3_get_record(SSL *s) /* just read a 0 length packet */ if (rr[j].length == 0) { - curr_empty++; - empty_record_count++; - if (empty_record_count > MAX_EMPTY_RECORDS) { + RECORD_LAYER_inc_empty_record_count(&s->rlayer); + if (RECORD_LAYER_get_empty_record_count(&s->rlayer) + > MAX_EMPTY_RECORDS) { al = SSL_AD_UNEXPECTED_MESSAGE; SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_RECORD_TOO_SMALL); goto f_err; } + } else { + RECORD_LAYER_reset_empty_record_count(&s->rlayer); } } - if (curr_empty == num_recs) { - /* We have no data - do it all again */ - num_recs = 0; - curr_empty = 0; - goto again; - } RECORD_LAYER_set_numrpipes(&s->rlayer, num_recs); return 1;