From 6b41b3f5eacc6b1bb851c9dce22d6e893f32ea7d Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 21 May 2015 14:06:52 +0100 Subject: [PATCH] Fix a memory leak in compression The function RECORD_LAYER_clear() is supposed to clear the contents of the RECORD_LAYER structure, but retain certain data such as buffers that are allocated. Unfortunately one buffer (for compression) got missed and was inadvertently being wiped, thus causing a memory leak. In part this is due to the fact that RECORD_LAYER_clear() was reaching inside SSL3_BUFFERs and SSL3_RECORDs, which it really shouldn't. So, I've rewritten it to only clear the data it knows about, and to defer clearing of SSL3_RECORD and SSL3_BUFFER structures to SSL_RECORD_clear() and the new function SSL3_BUFFER_clear(). Reviewed-by: Tim Hudson Reviewed-by: Rich Salz --- ssl/record/rec_layer_s3.c | 49 +++++++++++++++++++-------------------- ssl/record/record_locl.h | 1 + ssl/record/ssl3_buffer.c | 13 +++++++++++ ssl/record/ssl3_record.c | 8 ++++++- 4 files changed, 45 insertions(+), 26 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 456fac46d0..47a021d687 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -142,35 +142,34 @@ void RECORD_LAYER_init(RECORD_LAYER *rl, SSL *s) void RECORD_LAYER_clear(RECORD_LAYER *rl) { - unsigned char *rp, *wp; - size_t rlen, wlen; - int read_ahead; - SSL *s; - DTLS_RECORD_LAYER *d; - - s = rl->s; - d = rl->d; - read_ahead = rl->read_ahead; - rp = SSL3_BUFFER_get_buf(&rl->rbuf); - rlen = SSL3_BUFFER_get_len(&rl->rbuf); - wp = SSL3_BUFFER_get_buf(&rl->wbuf); - wlen = SSL3_BUFFER_get_len(&rl->wbuf); - memset(rl, 0, sizeof(*rl)); - SSL3_BUFFER_set_buf(&rl->rbuf, rp); - SSL3_BUFFER_set_len(&rl->rbuf, rlen); - SSL3_BUFFER_set_buf(&rl->wbuf, wp); - SSL3_BUFFER_set_len(&rl->wbuf, wlen); - - /* Do I need to do this? As far as I can tell read_ahead did not + rl->rstate = SSL_ST_READ_HEADER; + + /* Do I need to clear read_ahead? As far as I can tell read_ahead did not * previously get reset by SSL_clear...so I'll keep it that way..but is * that right? */ - rl->read_ahead = read_ahead; - rl->rstate = SSL_ST_READ_HEADER; - rl->s = s; - rl->d = d; + + rl->packet = NULL; + rl->packet_length = 0; + rl->wnum = 0; + memset(rl->alert_fragment, 0, sizeof(rl->alert_fragment)); + rl->alert_fragment_len = 0; + memset(rl->handshake_fragment, 0, sizeof(rl->handshake_fragment)); + rl->handshake_fragment_len = 0; + rl->wpend_tot = 0; + rl->wpend_type = 0; + rl->wpend_ret = 0; + rl->wpend_buf = NULL; + + SSL3_BUFFER_clear(&rl->rbuf); + SSL3_BUFFER_clear(&rl->wbuf); + SSL3_RECORD_clear(&rl->rrec); + SSL3_RECORD_clear(&rl->wrec); + + memset(rl->read_sequence, 0, sizeof(rl->read_sequence)); + memset(rl->write_sequence, 0, sizeof(rl->write_sequence)); - if (d) + if (rl->d) DTLS_RECORD_LAYER_clear(rl); } diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h index b2222d7c22..f92e89d86f 100644 --- a/ssl/record/record_locl.h +++ b/ssl/record/record_locl.h @@ -162,6 +162,7 @@ void dtls1_record_bitmap_update(SSL *s, DTLS1_BITMAP *bitmap); #define SSL3_BUFFER_add_offset(b, o) ((b)->offset += (o)) #define SSL3_BUFFER_is_initialised(b) ((b)->buf != NULL) +void SSL3_BUFFER_clear(SSL3_BUFFER *b); void SSL3_BUFFER_set_data(SSL3_BUFFER *b, const unsigned char *d, int n); void SSL3_BUFFER_release(SSL3_BUFFER *b); __owur int ssl3_setup_read_buffer(SSL *s); diff --git a/ssl/record/ssl3_buffer.c b/ssl/record/ssl3_buffer.c index 5a8d34c6fb..66fb721b1d 100644 --- a/ssl/record/ssl3_buffer.c +++ b/ssl/record/ssl3_buffer.c @@ -120,6 +120,19 @@ void SSL3_BUFFER_set_data(SSL3_BUFFER *b, const unsigned char *d, int n) b->offset = 0; } +/* + * Clear the contents of an SSL3_BUFFER but retain any memory allocated + */ +void SSL3_BUFFER_clear(SSL3_BUFFER *b) +{ + unsigned char *buf = b->buf; + size_t len = b->len; + + memset(b, 0, sizeof(*b)); + b->buf = buf; + b->len = len; +} + void SSL3_BUFFER_release(SSL3_BUFFER *b) { OPENSSL_free(b->buf); diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index b0eb7cce92..5070bc35c0 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -132,9 +132,15 @@ static const unsigned char ssl3_pad_2[48] = { 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c }; +/* + * Clear the contents of an SSL3_RECORD but retain any memory allocated + */ void SSL3_RECORD_clear(SSL3_RECORD *r) { - memset(r->seq_num, 0, sizeof(r->seq_num)); + unsigned char *comp = r->comp; + + memset(r, 0, sizeof(*r)); + r->comp = comp; } void SSL3_RECORD_release(SSL3_RECORD *r) -- 2.34.1