From 44efb88a21d464dba3ac5084c8d4553d696fab33 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 1 Aug 2016 17:15:13 +0100 Subject: [PATCH] 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 --- ssl/record/rec_layer_s3.c | 1 + ssl/record/record.h | 3 +++ ssl/record/record_locl.h | 2 ++ ssl/record/ssl3_record.c | 14 +++++++------- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 2d0fca2672..048f756577 100644 --- 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 9177fb4be5..ce60a1508f 100644 --- 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 435e92a11e..8f40430745 100644 --- 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 49c6756376..8481815cf6 100644 --- 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)