tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: NPF: fast kick



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


Home | Main Index | Thread Index | Old Index