[ovs-dev] [PATCH] odp-util: Fix overflow of nested netlink attributes.

[Ilya Maximets]

On 11/13/20 6:00 PM, Flavio Leitner wrote:
> 
> Hi Ilya,
> 
> On Mon, Oct 19, 2020 at 10:03:00PM +0200, Ilya Maximets wrote:
>> Length of nested attributes must be checked before storing to the
>> header.  If current length exceeds the maximum value parsing should
>> fail, otherwise the length value will be truncated leading to
>> corrupted netlink message and out-of-bound memory accesses:
>>
>>   ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6310002cc838
>>          at pc 0x000000575470 bp 0x7ffc6c322d60 sp 0x7ffc6c322d58
>>   READ of size 1 at 0x6310002cc838 thread T0
>>   SCARINESS: 12 (1-byte-read-heap-buffer-overflow)
>>     #0 0x57546f in format_generic_odp_key lib/odp-util.c:2738:39
>>     #1 0x559e70 in check_attr_len lib/odp-util.c:3572:13
>>     #2 0x56581a in format_odp_key_attr lib/odp-util.c:4392:9
>>     #3 0x5563b9 in format_odp_action lib/odp-util.c:1192:9
>>     #4 0x555d75 in format_odp_actions lib/odp-util.c:1279:13
>>     ...
>>
>> Fix that by checking the length of nested netlink attributes before
>> updating 'nla_len' inside the header.  Additionally introduced
>> assertion inside nl_msg_end_nested() to catch this kind of issues
>> before actual overflow happened.
>>
>> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20003
>> Fixes: 65da723b40a5 ("odp-util: Format tunnel attributes directly from netlink.")
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
> 
> I looked at few things:
> 
> The new assert() discount the included header to check only
> the payload and there are many more places in netlink with
> asserts, so it looks okay to me.
> 
> The new test triggers the assert if the check nl_attr_oversized()
> is removed, so the test reproduces the issue and the assert
> catches that.
> 
> The -E2BIG return is handled by the callers.
> 
> It passes the tests I have.
> 
> Acked-by: Flavio Leitner <fbl at sysclose.org>

Thanks!
Applied and backported down to 2.5.

Best regards, Ilya Maximets.