Re: NPF: fast kick

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 08/03/2018 à 13:28, Mindaugas Rasiukevicius a écrit :
> Hi,
>
> Please CC me when proposing NPF patches and please leave some extra time
> for response.
>
> Maxime Villard <max%m00nbsd.net@localhost> wrote:
> > Currently, NPF does not immediately kick malformed packets, and performs
> > very few sanity checks. Here is a patch [1] that fixes that.
> > <...>
>
> There are a few important points:
>
> - There is a good reason for NPF to be *able* to behave as a silent
> observer.  It can be used for deep packet inspection, packet analysis,
> accounting, etc.  Hence the reason why NPF performs minimalistic sanity
> checks.  Please do not assume that the only mode of operation for NPF
> is a traditional firewall.
>
> - Having said that, we should certainly have an option (and I agree it
> should be on by default) to perform extensive sanity checks and block
> anything unusual.  Just please make it a run-time *option* (for now, a
> config-level variable will do, and later I will make it changeable via
> npf.conf).

I don't understand your point. The checks I added enforce some basic aspects
of the specs; a packet that does not respect these aspects is just a malformed
packet, regardless of whether NPF is acting as a firewall or something else.

If NPF wasn't performing these checks, the packet would still be dropped by
the kernel afterwards.

We've already had too many problems because NPF basically doesn't sanitize
anything.

> - It is better to keep a logical separation between the packet handler
> which performs minimalistic sanity checks (just to extract the needed
> information for NPF, e.g. populate the npf_cache_t structure) and the
> function which performs a thorough validation.  So, can you please
> introduce something like npf_deep_validate() (with an option to skip
> it) where you abstract thorough protocol-level checks.

While I understand why you want a logical separation, I think that in all
fairness, the result is just shitty.

Typically, before my commit npc_hlen could point past the end of the mbuf. I
didn't even understand whether this was a real issue or not, because it gets
passed in many different components, and I wasn't able to make sure the
overflow was harmless (ie, make sure there are proper length checks in the
JIT code).

There is little interest in maintaining a logical separation if we are unable
to understand what happens in the complex machinery. As well, there is little
interest in passing a packet in this complex machinery if we know we'll end
up kicking it anyway because it is malformed to begin with.

So now let's perform basic sanitization early, outside of any ruleset, by
simply checking the return values of function calls that are *already there*.

Maxime