[ovs-dev] [PATCH] conntrack: Fix checks for TCP, UDP, and IPv6 header sizes.

[Bhargava Shastry]

My point is "miniflow_extract" has these checks that indicate a failed
parsing attempt for the packets in question. For example,

```C
else if (OVS_LIKELY(nw_proto == IPPROTO_ICMP)) {
   if (OVS_LIKELY(size >= ICMP_HEADER_LEN)) {
	do_something_with_valid_icmp_packet();
   }
   // Signaling of failed parsing attempt does not take place  //
   // i.e., no else corresponding to above predicate	       //
}

...

out:
   dst->map = mf.map
```

So when you know that a packet is malformed during flow extraction
itself, why would you let the packet float around in your downstream
packet processing pipeline? Similar argument for malformed TCP/UDP packets.

I'm afraid I don't know the code well, but I feel that a failed parsing
attempt must be signaled by flow_extract _somehow_ and no subsequent
processing of the invalid packet should take place. This is because, in
the absence of a fail signal, downstream processing code (e.g.
conntrack) implicitly trusts all packets to be valid. Of course, fixing
the bug at conntrack will close the specific holes in point, but there
may be bugs elsewhere that share the same root cause. Am I making sense?

Regards,
Bhargava

On 03/04/2017 05:03 PM, Ben Pfaff wrote:
> What bug do you see in the flow extract code?
> 
> On Sat, Mar 04, 2017 at 10:09:26AM +0100, Bhargava Shastry wrote:
>> Hi Ben,
>>
>> Question regarding patch: Shouldn't the fix be applied in flow extract code itself? I think the malformedness is evident during flow extraction. Might save you a few cycles/more secure.
>>
>>
>> On March 4, 2017 6:18:54 AM GMT+01:00, Ben Pfaff <blp at ovn.org> wrote:
>>> On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote:
>>>> 2017-03-03 14:08 GMT-08:00 Ben Pfaff <blp at ovn.org>:
>>>>> Otherwise a malformed packet could cause a read up to about 40
>>> bytes past
>>>>> the end of the packet.  The packet would still likely be dropped
>>> because
>>>>> of checksum verification.
>>>>>
>>>>> Reported-by: Bhargava Shastry <bshastry at sec.t-labs.tu-berlin.de>
>>>>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>>>>
>>>> Oops, thanks for the fix, Ben
>>>>
>>>> Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
>>>>
>>>> One minor comment below,
>>>>
>>>> Acked-by: Daniele Di Proietto <diproiettod at vmware.com>
>>>>
>>>>
>>>>> ---
>>>>>  lib/conntrack.c | 14 ++++++++++++--
>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>>>>> index 9bea3d93e4ad..9c1dd63648b8 100644
>>>>> --- a/lib/conntrack.c
>>>>> +++ b/lib/conntrack.c
>>>>> @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const
>>> void *data, size_t size,
>>>>>                  const char **new_data)
>>>>>  {
>>>>>      const struct ovs_16aligned_ip6_hdr *ip6 = data;
>>>>> +    if (size < sizeof *ip6) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>
>>>> We can read 'ip6->ip6_nxt' even though there's not enough data.  It
>>>> cannot happen
>>>> for regular TCP and UDP packets (those are covered my
>>>> miniflow_extract), but only
>>>> when parsing the nested l3 header in an ICMP error message.
>>>>
>>>> The code has the same check two lines below, maybe we can reuse that.
>>>> Technically
>>>> the check is necessary only if new_data != NULL, as explained by the
>>> comment
>>>> above, but perhaps it's more clear to always perform it.
>>>
>>> Thanks for pointing out the duplicate check.  I guess that I did not
>>> read the code carefully enough.
>>>
>>> Usually, I would argue to always do the check, but I prefer to make
>>> this
>>> a minimal change.
>>>
>>> I will post a v2.
>>
>> -- 
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 
Bhargava Shastry <bshastry at sec.t-labs.tu-berlin.de>
Security in Telecommunications
TU Berlin / Telekom Innovation Laboratories
Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany
phone: +49 30 8353 58235