Fix DTLS memory leak.
A memory leak can occur in dtls1_buffer_record if either of the calls to
ssl3_setup_buffers or pqueue_insert fail. The former will fail if there
is a malloc failure, whilst the latter will fail if attempting to add a
duplicate record to the queue. This should never happen because
duplicate records should be detected and dropped before any attempt to
add them to the queue. Unfortunately records that arrive that are for
the next epoch are not being recorded correctly, and therefore replays
are not being detected. Additionally, these "should not happen" failures
that can occur in dtls1_buffer_record are not being treated as fatal and
therefore an attacker could exploit this by sending repeated replay
records for the next epoch, eventually causing a DoS through memory
exhaustion.
Thanks to Chris Mueller for reporting this issue and providing initial
analysis and a patch. Further analysis and the final patch was performed
by Matt Caswell from the OpenSSL development team.
CVE-2015-0206
(Imported from upstream's 7c6a3cf2375f5881ef3f3a58ac0fbd0b4663abd1).
Change-Id: I765fe61c75bc295bcc4ab356b8a5ce88c8964764
Reviewed-on: https://boringssl-review.googlesource.com/2782
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 59fc6f4..67b06a5 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -235,7 +235,7 @@
}
OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
- return 0;
+ return -1;
}
rdata->packet = s->packet;
@@ -251,21 +251,24 @@
memset(&(s->s3->rrec), 0, sizeof(SSL3_RECORD));
if (!ssl3_setup_buffers(s)) {
- OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
- OPENSSL_free(rdata);
- pitem_free(item);
- return 0;
+ goto internal_error;
}
/* insert should not fail, since duplicates are dropped */
if (pqueue_insert(queue->q, item) == NULL) {
- OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
- OPENSSL_free(rdata);
- pitem_free(item);
- return 0;
+ goto internal_error;
}
return 1;
+
+internal_error:
+ OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
+ if (rdata->rbuf.buf != NULL) {
+ OPENSSL_free(rdata->rbuf.buf);
+ }
+ OPENSSL_free(rdata);
+ pitem_free(item);
+ return -1;
}
static int dtls1_retrieve_buffered_record(SSL *s, record_pqueue *queue) {
@@ -310,7 +313,10 @@
if (!dtls1_process_record(s)) {
return 0;
}
- dtls1_buffer_record(s, &(s->d1->processed_rcds), s->s3->rrec.seq_num);
+ if (dtls1_buffer_record(s, &(s->d1->processed_rcds),
+ s->s3->rrec.seq_num) < 0) {
+ return -1;
+ }
}
}
@@ -438,7 +444,6 @@
/* we have pulled in a full packet so zero things */
s->packet_length = 0;
- dtls1_record_bitmap_update(s, &(s->d1->bitmap)); /* Mark receipt of record. */
return 1;
f_err:
@@ -470,7 +475,9 @@
/* The epoch may have changed. If so, process all the pending records. This
* is a non-blocking operation. */
- dtls1_process_buffered_records(s);
+ if (dtls1_process_buffered_records(s) < 0) {
+ return -1;
+ }
/* If we're renegotiating, then there may be buffered records. */
if (dtls1_get_processed_record(s)) {
@@ -597,7 +604,10 @@
*/
if (is_next_epoch) {
if (SSL_in_init(s) || s->in_handshake) {
- dtls1_buffer_record(s, &(s->d1->unprocessed_rcds), rr->seq_num);
+ if (dtls1_buffer_record(s, &(s->d1->unprocessed_rcds), rr->seq_num) < 0) {
+ return -1;
+ }
+ dtls1_record_bitmap_update(s, bitmap); /* Mark receipt of record. */
}
rr->length = 0;
s->packet_length = 0;
@@ -609,6 +619,7 @@
s->packet_length = 0; /* dump this record */
goto again; /* get another record */
}
+ dtls1_record_bitmap_update(s, bitmap); /* Mark receipt of record. */
return 1;
}
@@ -729,7 +740,10 @@
/* We now have application data between CCS and Finished. Most likely the
* packets were reordered on their way, so buffer the application data for
* later processing rather than dropping the connection. */
- dtls1_buffer_record(s, &(s->d1->buffered_app_data), rr->seq_num);
+ if (dtls1_buffer_record(s, &(s->d1->buffered_app_data), rr->seq_num) < 0) {
+ OPENSSL_PUT_ERROR(SSL, dtls1_read_bytes, ERR_R_INTERNAL_ERROR);
+ return -1;
+ }
rr->length = 0;
goto start;
}
