Re: NPF: fast kick

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Maxime Villard <max%m00nbsd.net@localhost> wrote:
> > 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.

Just a note: my reply was also with your other email thread "NPF: TCP
options" in mind.  Perhaps this caused some confusion.

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

Sure, but if NPF is used for deep packet inspection, analysis, accounting
or monitoring -- this is a desirable behaviour.  If my application is
observing traffic at some transient point in the network, then I do not
want it to drop the packets.  Even if the packets are malformed, because
the main purpose of such application is to be packet *observer*.  NPF can
operate as a *filter*, but it may as well operate as an *observer*.  I am
just saying -- please do not disregard the latter use case and give user
an option.

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

Yes, but was kind of a rational choice.  I think the core packet handlers
of NPF should still perform the minimum sanity checks, required merely for
NPF itself.  Keep the core code simple and usable for packet observation.

There can be many defence mechanisms.  Protecting the end hosts against
malformed or "unusual" packets is one of them.  I am all for adding more
aggressive checks.  But again -- please give user an option to control
this.  I might *want* to send malformed packets, for whatever reason.

> 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*.

I do not see what logical separation has to do with npf_hlen bug or
"complex machinery".  If you mean basic sanity checks such that NPF
could parse the information it needs -- sure; if there are cases where
ranges are not checked or overflows can happen -- those are bugs and
should be fixed.

Since NPF primarily operates from layer 3 and does not yet support L2
filtering, I think your extra checks for IP fragments are reasonable
(although I would rename the NPF_FMTERR to NPF_L3ERR as it reflects the
intention better).  However, your suggested checks on the TCP options
should be optional.  This is the point where we should have logical
separation i.e. L4 checks abstracted in a separate routine, potentially
with a way to configure how aggressive they should be (besides an option
to disable it completely).

-- 
Mindaugas