[Openvpn-devel,4/5,v2] buffer_list_aggregate_separator(): prevent 0-byte malloc

Message ID 1514541240-19536-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers show
Series None | expand

Commit Message

Steffan Karger Dec. 28, 2017, 10:54 p.m. UTC
As pointed out in finding OVPN-05 of the cryptograpy engineering audit
(funded by Private Internet Access), buffer_list_aggregate_separator()
could perform a 0-byte malloc when called with a list of 0-length buffers
and a "" separator.  If other could would later try to access that buffer
memory, this would result in undefined behaviour.  To prevent this, always
malloc() 1 byte.

To simplify as we go, use alloc_buf() to allocate the buffer.  This has
the additional benefit that the actual buffer data (not the contents) is
zero-terminated, because alloc_buf() calls calloc() and we have 1 extra
byte of data.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
v2: add spaces around '+'

 src/openvpn/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Antonio Quartulli Dec. 28, 2017, 11:01 p.m. UTC | #1
Hi,

On 29/12/17 17:54, Steffan Karger wrote:
> As pointed out in finding OVPN-05 of the cryptograpy engineering audit
> (funded by Private Internet Access), buffer_list_aggregate_separator()
> could perform a 0-byte malloc when called with a list of 0-length buffers
> and a "" separator.  If other could would later try to access that buffer
> memory, this would result in undefined behaviour.  To prevent this, always
> malloc() 1 byte.
> 
> To simplify as we go, use alloc_buf() to allocate the buffer.  This has
> the additional benefit that the actual buffer data (not the contents) is
> zero-terminated, because alloc_buf() calls calloc() and we have 1 extra
> byte of data.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Jan. 11, 2018, 12:38 a.m. UTC | #2
Your patch has been applied to the master and release/2.4 branch.

commit 748902f46260fe11cb25726d2bf93bb06ad338f2 (master)
commit ef4b827d68d8fef75b04f23e6322aa9d2d4ad8a8 (release/2.4)
Author: Steffan Karger
Date:   Fri Dec 29 10:54:00 2017 +0100

     buffer_list_aggregate_separator(): prevent 0-byte malloc

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <1514541240-19536-1-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16106.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 2656702..cfe6f2c 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -1251,8 +1251,7 @@  buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
             struct buffer_entry *e = bl->head, *f;
 
             ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf.data = malloc(size);
-            check_malloc_return(f->buf.data);
+            f->buf = alloc_buf(size + 1); /* prevent 0-byte malloc */
             f->buf.capacity = size;
             for (i = 0; e && i < count; ++i)
             {