Hi, On 01/07/17 20: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.kar...@fox-it.com>
Does it really make sense to continue the execution if we receive a list
of empty buffers? Shouldn't we report this as an error?
> ---
> src/openvpn/buffer.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 2700ad1..ecbdc1b 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -1261,8 +1261,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 */
the '+' sign should be surrounded by spaces.
Cheers,
> f->buf.capacity = size;
> for (i = 0; e && i < count; ++i)
> {
>
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
