Address feedback on SSLv2 ClientHello processing
authorMatt Caswell <matt@openssl.org>
Mon, 1 Aug 2016 16:15:13 +0000 (17:15 +0100)
committerMatt Caswell <matt@openssl.org>
Mon, 15 Aug 2016 22:14:30 +0000 (23:14 +0100)
Feedback on the previous SSLv2 ClientHello processing fix was that it
breaks layering by reading init_num in the record layer. It also does not
detect if there was a previous non-fatal warning.

This is an alternative approach that directly tracks in the record layer
whether this is the first record.

GitHub Issue #1298

Reviewed-by: Tim Hudson <tjh@openssl.org>
ssl/record/rec_layer_s3.c
ssl/record/record.h
ssl/record/record_locl.h
ssl/record/ssl3_record.c

index 2d0fca2..048f756 100644 (file)
@@ -33,6 +33,7 @@
 void RECORD_LAYER_init(RECORD_LAYER *rl, SSL *s)
 {
     rl->s = s;
+    RECORD_LAYER_set_first_record(&s->rlayer, 1);
     SSL3_RECORD_clear(rl->rrec, SSL_MAX_PIPELINES);
 }
 
index 9177fb4..ce60a15 100644 (file)
@@ -199,6 +199,9 @@ typedef struct record_layer_st {
     unsigned char read_sequence[SEQ_NUM_SIZE];
     unsigned char write_sequence[SEQ_NUM_SIZE];
 
+    /* Set to true if this is the first record in a connection */
+    unsigned int is_first_record;
+
     DTLS_RECORD_LAYER *d;
 } RECORD_LAYER;
 
index 435e92a..8f40430 100644 (file)
@@ -31,6 +31,8 @@
 #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 RECORD_LAYER_is_first_record(rl)        ((rl)->is_first_record)
+#define RECORD_LAYER_set_first_record(rl, val)  ((rl)->is_first_record = (val))
 #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);
index 49c6756..8481815 100644 (file)
@@ -162,15 +162,14 @@ int ssl3_get_record(SSL *s)
              * Check whether this is a regular record or an SSLv2 style record.
              * The latter can only be used in the first record of an initial
              * ClientHello for old clients. Initial ClientHello means
-             * s->first_packet is set and s->server is true. The first record
-             * means we've not received any data so far (s->init_num == 0) and
-             * have had no empty records. We check s->read_hash and
-             * s->enc_read_ctx to ensure this does not apply during
-             * renegotiation.
+             * s->first_packet is set and s->server is true.  The first record
+             * means s->rlayer.is_first_record is true. Probably this is
+             * sufficient in itself instead of s->first_packet, but I am
+             * cautious. We check s->read_hash and s->enc_read_ctx to ensure
+             * this does not apply during renegotiation.
              */
             if (s->first_packet && s->server
-                    && s->init_num == 0
-                    && RECORD_LAYER_get_empty_record_count(&s->rlayer) == 0
+                    && RECORD_LAYER_is_first_record(&s->rlayer)
                     && s->read_hash == NULL && s->enc_read_ctx == NULL
                     && (p[0] & 0x80) && (p[2] == SSL2_MT_CLIENT_HELLO)) {
                 /*
@@ -335,6 +334,7 @@ int ssl3_get_record(SSL *s)
 
         /* we have pulled in a full packet so zero things */
         RECORD_LAYER_reset_packet_length(&s->rlayer);
+        RECORD_LAYER_set_first_record(&s->rlayer, 0);
     } while (num_recs < max_recs
              && rr[num_recs-1].type == SSL3_RT_APPLICATION_DATA
              && SSL_USE_EXPLICIT_IV(s)