Re: NPF: fast kick

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

Le 12/03/2018 à 20:46, Mindaugas Rasiukevicius a écrit :
> 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.

The kernel performs sanity checks on IP packets even before invoking NPF.
This "accounting malformed packets" thing does not hold in the first place,
because at several points the kernel can drop the packets before, and after,
invoking NPF.

If the basic sanity checks I added in NPF (which consist, as I already said,
in checking the return values of function calls that are _already there_)
were to be an option, we would have to go through the ruleset machinery, which
is exactly where bugs can happen. That wouldn't be "fast kick", it would be
"slow, buggy kick". (heh!)

Not checking the return values of the nbuf functions, just for the sake of
some hypothetical "observer" thing that doesn't hold in the first place,
doesn't make any sense, apart from wanting more bugs by default.

A real "observer" would have to be some pseudo device, to which a copy of the
packet is given at the very entry point (L2) - in order to ensure that neither
the kernel nor any driver has sanitized anything. Perhaps we already have this,
I didn't really check. In all cases, NPF is never going to achieve that, as
long as it doesn't get called before anything else - which will likely never
happen, because it would be wrong.

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

What I'm saying is that there are overflows - npc_hlen that is beyond the
nbuf -, but I'm not sure whether they are real bugs.

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

You are mixing the "NPF: fast kick" and "NPF: TCP options" threads, but my
point is different in each.

For the latter, my point is that the processing done by NPF and by the kernel
should be the same, because if there is a divergence then there is a way to
bypass certain normalization procedures.

So NPF's behavior should be aligned to that of the kernel; that is to say, NPF
should ignore TCP options with uncommon lengths - which does not mean dropping
the packet. (We can discuss about changing the kernel's behavior to be that
of NPF, but as I said in my answer to Joerg, the kernel's behavior is the one
that is the most "common".)

Maxime