[PATCH 0/2] xfrm: fix buffer overflows

[Thomas Egerer]

Hello Thomas,

On 06/27/2016 08:25 PM, Thomas Haller wrote:
> On Mon, 2016-06-27 at 19:10 +0200, Thomas Egerer wrote:
>> Hello Thomas,
>>
>> On 06/25/2016 02:02 PM, Thomas Haller wrote:
>>> On Tue, 2016-05-31 at 17:29 +0200, Thomas Egerer wrote:
>>>> Hi *,
>>>>
>>>> we have found one definite and one potential buffer overflow
>>>> in libnl when adding xfrm states.
>>>> The definite one is triggered whenever an aead/auth (etc) key
>>>> is added to an xfrmnl_sa structure. The potential one is only
>>>> triggered if the same functions are called with alg_names
>>>> longer than 72/68 bytes + keysize. Then a strcpy call writes
>>>> beyond the appropriate data structures in struct xfrmnl_sa.
>>>>
>>>> Cheers,
>>>> Thomas
>>>>
>>>> Thomas Egerer (2):
>>>>   xfrm: fix buffer overflow when copying keys
>>>>   xfrm: check length of alg_name before strcpying it
>>>>
>>>
>>>
>>> Hi Thomas,
>>>
>>>
>>> thanks for the 2 patches. Merged both to master:
>>> https://github.com/thom311/libnl/commit/c009b20919562e6968969309049
>>> 064d59d35010a
>>>
>>>
>>>
>>> xfrmnl_sa_get_comp_params() et al. seems wrong too.
>>> How can somebody safely know the required size of the @key buffer?
>> If changing the API of this part of the code is not a problem, then
>> the prototype could be changed such that the key pointer (char *)
>> becomes a char ** which is allocated by the function and has to be
>> freed by the user. Another option -- adding another maxlen parameter
>> to indicate the size of the key buffer -- also means an API change.
> 
> 
> Hi,
> 
> I think changing ABI or API should be avoided.
> 
> But the current version cannot be called without danger of buffer-
> overflow (or can it? How?). Thus changing behavior to a completely
> broken version seems acceptable.
> 
> 
> How about
>  https://github.com/thom311/libnl/commits/xfrm-get-params-keylen
> ?
I like this approach. And nice job on the documentation.


Cheers,
Thomas