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 <[email protected]> > --- 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 <[email protected]> Thanks, fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
