[ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

Darrell Ball dlu998 at gmail.com
Wed Aug 28 16:37:51 UTC 2019


On Wed, Aug 28, 2019 at 1:43 AM Vishal Deep Ajmera <
vishal.deep.ajmera at ericsson.com> wrote:

> That is interesting
>
> i just tried applying on top of tree and I see that the git applies some
> changes (2 lines)
>
> in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the
> patch I sent out.
>
> My guess is that the surrounding lines are identical in the 2 functions
> and I had other
>
> patches in the same branch shifting the patch downward, hence git applied
> the changes
>
> to extract_l4_icmp6() rather than extract_l4_icmp()
>
>
>
> I'll make the changes on a clean branch and resend.
>
>
>
> Thanks. I applied this patch and looks ok to me.
>

Thanks for confirming


>
>
> JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
>
> ICMP6 does not have this restriction; see
> https://tools.ietf.org/html/rfc4443
>
>
>
> In my opinion, we should limit the check to < 8 bytes even in case of
> ICMPv6 as that is all
>
> is required from the TCP header to extract port numbers and aligns it with
> ICMPv4.
>
> Specially because RFC is not mandating minimum size for L4 header in case
> of ICMPv6.
>

For V6, the ICMP error L4 length can even be zero, legitimately, with a
large extension header
presence, theoretically; practically, these are almost certainly crafted
packets. The existing
check for ICMP6 is mostly permissive but reasonable.

The ICMP6 sanity check can be enhanced. For the most part, it can be made
more strict while
handling the above corner case perfectly.

I don't think we should mess with enhancing ICMP6 as part of this bug fix
for ICMPv4.
Thats why this patch leaves ICMP6 unmodified.


>
>


More information about the dev mailing list