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

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index a656b56cf296b983b04e0254a0252b993583f55d..73260767d16d84231be1591fafdef3e6ef35bfa6 100644 (file)
--- 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 9e198224fc66a5513ce736ce3b45ce1e01b6472c..cda4eff0d36251f9ca6aee86b6b26242f0820ce7 100644 (file)
--- 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 67ae1f42902a1470f390120e030df30a05e88581..ff1eb32191eabc26718b380acc47163996dcb7cb 100644 (file)
--- 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 872a3812aa902dda31f7588f4705a014afe01f74..d3b2bea40a9173308f7cbbdade60255436a5ee4b 100644 (file)
--- 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;