Commit 44efb88a authored by Matt Caswell's avatar Matt Caswell
Browse files

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: default avatarTim Hudson <tjh@openssl.org>
parent c35d339d

ssl/record/rec_layer_s3.c

+1 −0
Original line number Diff line number Diff line
@@ -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);

}

ssl/record/record.h

+3 −0
Original line number Diff line number Diff line
@@ -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;

ssl/record/record_locl.h

+2 −0
Original line number Diff line number Diff line
@@ -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);

ssl/record/ssl3_record.c

+7 −7
Original line number Diff line number Diff line
@@ -163,14 +163,13 @@ int ssl3_get_record(SSL *s) 
             * 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.

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