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: NAndy Polyakov <appro@openssl.org>

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)

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;

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);

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;