Date:   Fri, 01 Sep 2017 19:05:36 +0200
From:   Hannes Frederic Sowa <hannes@...essinduktion.org>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Tom Herbert <tom@...ntonium.net>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        alex.popov@...ux.com
Subject: Re: [PATCH net-next 2/2] flow_dissector: Add limits for encapsulation and EH

Tom Herbert <tom@...bertland.com> writes:

> On Fri, Sep 1, 2017 at 9:35 AM, Hannes Frederic Sowa
> <hannes@...essinduktion.org> wrote:
>> Hello Tom,
>>
>> Tom Herbert <tom@...ntonium.net> writes:
>>
>>> On Fri, Sep 1, 2017 at 6:32 AM, Hannes Frederic Sowa
>>> <hannes@...essinduktion.org> wrote:
>>>> Tom Herbert <tom@...ntonium.net> writes:
>>>>
>>>>> In flow dissector there are no limits to the number of nested
>>>>> encapsulations that might be dissected which makes for a nice DOS
>>>>> attack. This patch limits for dissecting nested encapsulations
>>>>> as well as for dissecting over extension headers.
>>>>
>>>> I was actually more referring to your patch, because the flow dissector
>>>> right now is not stack recursive. Your changes would make it doing
>>>> recursion on the stack.
>>>
>>> I don't believe those patches had any recursion.
>>
>> I was wrong with stack recursion, you handle that using the
>> FLOW_DISSECT_RET_PROTO_AGAIN return value thus leaving the stack frame
>> again, sorry.
>>
>> But otherwise the walk would be unlimited (based on the packet size) in
>> your first patchset, correct? See this malicious example:
>>
>> | IP1 | UDP1 | VXLAN1 | Ethernet | IP2 | UDP2 | VXLAN2 | ...
>>
> Without the limits patch I subsequently proposed, yes. However, this
> is true for all the other encapsulations anyway; there's is nothing
> unique about UDP encapsulations in this regard (hence with the limit
> patch should generally apply to all encapsulations).

I used this example to show its possible security implications. Other
encaps definitely have the same problems.

>> where IP1 == IP2, UDP1 == UDP2 and VXLAN1 != VXLAN2?
>>
>> Notice that because IP1 == IP2 and UDP1 == UDP2 it seems to me it would
>> hit the same socket again. We would be prone to overwrite vxlan id 1
>> with vxlan id 2 in the key thus the key would be malicious and traffic
>> could be injected into other tenant networks, if the encapsulated
>> packets within VXLAN1 could be generated by a malicious user?
>>
> This is why flow dissection is not an authoritative parsing of the
> packet. It can be wrong or misleading because it doesn't have all the
> context, doesn't necessarily parse the whole chain, and only returns
> one set of information (for instance only one pair of IP addresses
> when there may be more in a packet). It's just a best effort mechanism
> that is great for computing a hash for instance. If someone is
> steering a packet to a VM based on the output of flow dissector that
> is a bug; the only correct way to do this is to go through the normal
> receive protocol processing path.

I think it must be agreed upon what flow dissector is. Especially I am
concerned about its use in cls_flower (or vice versa this patch).

>> I was actually not concerned about the "recursion" but merely about
>> updating the values to the innermost values.
>>
> See my previous comment about use STOP_AT_ENCAP.

For an authorative parser, for which it gets used in flower right now
STOP_AT_ENCAP might not make too much sense, because it might want to
look at one additional level of encapsulation information. But if you
don't consider the dissector to be an authorative parser for packets, it
would be okay.

Btw., I fear this recursion problem exists right now also with flower's
use of lwt.

[...]

Thanks,
Hannes

Powered by blists - more mailing lists