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

[Antonio Q. <a...@un...>]

Hi,

On 29/12/17 17:26, Steffan Karger wrote:
> Hi,
> 
> On 28-12-17 23:51, Antonio Quartulli wrote:
>> 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 <ste...@fo...>
> 
> 
>> 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?
> 
> Data is pushed to buffer lists from e.g. tls_send_payload(), where I'm
> not sure that 'sending' 0-byte payload is an invalid operation.  So
> instead of trying to figure out if that's possible, I prefer to just
> make this function deal with it.

Yeah, makes sense: better let the external layer deal with the logic.

> 
> (Consider e.g. 0/n record splitting, which might want to 'send'
> 0-length data.)
> 
> I'll send follow-up patches to take care of your other remarks.
> 

Thanks!

Cheers,



-- 
Antonio Quartulli