Fix DTLS memory leak (CVE-2015-0206).

There were four bugs fixed by this patch:

* dtls1_buffer_record() now frees rdata->rbuf.buf on error.  Since
  s->s3->rbuf was memset, rdata->rbuf is the only pointer left which
  points to the old rbuf.  On error, rdata is freed so there will not
  be any way of freeing this memory unless we do it here.

* Changed the return code of dtls1_buffer_record() to differentiate
  between queue full (0) and error (-1).  See below as this differs
  from upstream.

* Handle errors if calls to dtls1_buffer_record() fail with -1.
  Previously, it did not check the return value.

* Changed the way receipts are recorded.  Previously, it was recorded
  when processed successfully (whether buffered or not) in
  dtls1_process_record().  Now, it records when it is handled in
  dtls1_get_record(): either when it is entered into the queue to buffer
  for the next epoch or when it is processed directly.  Processing
  buffered records does not add a receipt because it needed one in
  order to get into the queue.

The above bugs combined contributed to an eventual DoS through memory
exhaustion.  The memory leak came from dtls1_buffer_record()'s error
handling.  The error handling can be triggered by a duplicate record
or malloc failure.  It was possible to add duplicate records because
they were not being dropped.  The faulty receipts logic did not detect
replays when dealing with records for the next epoch.  Additionally,
dtls1_buffer_record()'s return value was not checked so an attacker
could send repeated replay records for the next epoch.

Reported to OpenSSL by Chris Mueller.

Patch based on OpenSSL commit 103b171d8fc282ef435f8de9afbf7782e312961f
and BoringSSL commit 44e2709cd65fbd2172b9516c79e56f1875f60300.

Our patch matches BoringSSL's commit.  OpenSSL returns 0 when the queue
is full or when malloc() or pitem_new() fails.  They return -1 on error
including !ssl3_setup_buffers() which is another failure to allocate
memory.

BoringSSL and LibreSSL changed the return code for dtls1_buffer_record()
to be 1 on success, 0 when the queue is full and -1 on error.

input + ok bcook@, jsing@

diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c
index 7bdf245..91e9c14 100644 (file)
--- a/lib/libssl/d1_pkt.c
+++ b/lib/libssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.38 2014/12/14 15:30:50 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.39 2015/01/21 00:15:50 doug Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -222,7 +222,7 @@ dtls1_buffer_record(SSL *s, record_pqueue *queue, unsigned char *priority)
        rdata = malloc(sizeof(DTLS1_RECORD_DATA));
        item = pitem_new(priority, rdata);
        if (rdata == NULL || item == NULL)
-               goto err;
+               goto init_err;
 
        rdata->packet = s->packet;
        rdata->packet_length = s->packet_length;
@@ -254,10 +254,13 @@ dtls1_buffer_record(SSL *s, record_pqueue *queue, unsigned char *priority)
        return (1);
 
 err:
+       free(rdata->rbuf.buf);
+
+init_err:
        SSLerr(SSL_F_DTLS1_BUFFER_RECORD, ERR_R_INTERNAL_ERROR);
        free(rdata);
        pitem_free(item);
-       return (0);
+       return (-1);
 }
 
 
@@ -308,8 +311,9 @@ dtls1_process_buffered_records(SSL *s)
                        dtls1_get_unprocessed_record(s);
                        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);
                }
        }
 
@@ -446,7 +450,6 @@ dtls1_process_record(SSL *s)
 
        /* 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:
@@ -480,7 +483,8 @@ dtls1_get_record(SSL *s)
 
        /* 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))
@@ -611,7 +615,11 @@ again:
         */
        if (is_next_epoch) {
                if ((SSL_in_init(s) || s->in_handshake) && !s->d1->listen) {
-                       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);
+                       /* Mark receipt of record. */
+                       dtls1_record_bitmap_update(s, bitmap);
                }
                rr->length = 0;
                s->packet_length = 0;
@@ -625,6 +633,8 @@ again:
                goto again;
                /* get another record */
        }
+       /* Mark receipt of record. */
+       dtls1_record_bitmap_update(s, bitmap);
 
        return (1);
 
@@ -769,7 +779,11 @@ start:
                 * 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) {
+                       SSLerr(SSL_F_DTLS1_READ_BYTES, ERR_R_INTERNAL_ERROR);
+                       return (-1);
+               }
                rr->length = 0;
                goto start;
        }

diff --git a/lib/libssl/src/ssl/d1_pkt.c b/lib/libssl/src/ssl/d1_pkt.c
index 7bdf245..91e9c14 100644 (file)
--- a/lib/libssl/src/ssl/d1_pkt.c
+++ b/lib/libssl/src/ssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.38 2014/12/14 15:30:50 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.39 2015/01/21 00:15:50 doug Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -222,7 +222,7 @@ dtls1_buffer_record(SSL *s, record_pqueue *queue, unsigned char *priority)
        rdata = malloc(sizeof(DTLS1_RECORD_DATA));
        item = pitem_new(priority, rdata);
        if (rdata == NULL || item == NULL)
-               goto err;
+               goto init_err;
 
        rdata->packet = s->packet;
        rdata->packet_length = s->packet_length;
@@ -254,10 +254,13 @@ dtls1_buffer_record(SSL *s, record_pqueue *queue, unsigned char *priority)
        return (1);
 
 err:
+       free(rdata->rbuf.buf);
+
+init_err:
        SSLerr(SSL_F_DTLS1_BUFFER_RECORD, ERR_R_INTERNAL_ERROR);
        free(rdata);
        pitem_free(item);
-       return (0);
+       return (-1);
 }
 
 
@@ -308,8 +311,9 @@ dtls1_process_buffered_records(SSL *s)
                        dtls1_get_unprocessed_record(s);
                        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);
                }
        }
 
@@ -446,7 +450,6 @@ dtls1_process_record(SSL *s)
 
        /* 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:
@@ -480,7 +483,8 @@ dtls1_get_record(SSL *s)
 
        /* 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))
@@ -611,7 +615,11 @@ again:
         */
        if (is_next_epoch) {
                if ((SSL_in_init(s) || s->in_handshake) && !s->d1->listen) {
-                       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);
+                       /* Mark receipt of record. */
+                       dtls1_record_bitmap_update(s, bitmap);
                }
                rr->length = 0;
                s->packet_length = 0;
@@ -625,6 +633,8 @@ again:
                goto again;
                /* get another record */
        }
+       /* Mark receipt of record. */
+       dtls1_record_bitmap_update(s, bitmap);
 
        return (1);
 
@@ -769,7 +779,11 @@ start:
                 * 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) {
+                       SSLerr(SSL_F_DTLS1_READ_BYTES, ERR_R_INTERNAL_ERROR);
+                       return (-1);
+               }
                rr->length = 0;
                goto start;
        }