Address feedback on SSLv2 ClientHello processing

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>

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 2d0fca2672aafc39d660c92cc66b9fbba272c044..048f756577d5c54eb37a6acc8c7cc15c76cee978 100644 (file)
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -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);
 }
 

diff --git a/ssl/record/record.h b/ssl/record/record.h
index 9177fb4be5210d47510ed831b11a0899a416a113..ce60a1508f36c576254be6ff78083d3b5366a559 100644 (file)
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -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;
 

diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h
index 435e92a11e439bc695587b429535a5d384b02c2f..8f404307456f224d085fbd3e7eca15a39dd2a5f6 100644 (file)
--- a/ssl/record/record_locl.h
+++ b/ssl/record/record_locl.h
@@ -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);

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index 49c6756376a5d42f983e93f33653ad5db8887f92..8481815cf61280ad64f5a03ec59f2d0257c288e3 100644 (file)
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -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)