netdev - Re: [v12 PATCH 2/3] NETFILTER module xt_hmark, new target for HASH based fwmark

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 7 May 2012 10:20:42 +0200
From:	Hans Schillstrom <hans.schillstrom@...csson.com>
To:	Pablo Neira Ayuso <pablo@...filter.org>
CC:	"kaber@...sh.net" <kaber@...sh.net>,
	"jengelh@...ozas.de" <jengelh@...ozas.de>,
	"netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"hans@...illstrom.com" <hans@...illstrom.com>
Subject: Re: [v12 PATCH 2/3] NETFILTER module xt_hmark, new target for HASH based fwmark

On Monday 07 May 2012 00:57:38 Pablo Neira Ayuso wrote:
> Hi Hans,
> 
> [...]
> > > > > Regarding ICMP traffic, I think we can use the ID field for the
> > > > > hashing as well. Thus, we handle ICMP like other protocols.
> > > >
> > > > Yes why not, I can give it a try.
> > > >
> >
> > I think we wait with this one..
> 
> I see. This is easy to add for the conntrack side, but it will require
> some extra code for the packet-based solution.

Actually I think there is very little gain to spread with type 
and then we must add a user mode possibility to turn it off 
i.e. a --hmark-icmp-type-mask 

> Not directly related to this but, I know that your intention is to
> make this as flexible as possible. However, I still don't find how I
> would use the port mask feature in any of my setups.  Basically, I
> don't come up with any useful example for this situation.

We have plenty of rules where just source port mask is zero.
and the dest-port-mask is 0xfffc (or 0xffff)


> I'm also telling this because I think that ICMP support will be
> easier to add if port masking is removed.
> 
> [...]
> > This is what I have done.
> >
> > - I reduced the code size a little bit by combining the hmark_ct_set_htuple_ipvX into one func.
> >   by adding a hmark_addr6_mask() and hmark_addr_any_mask()
> >   Note that using "otuple->src.l3num" as param 1 in both src and dst is not a typo.
> >   (it's not set in the rtuple)
> 
> Good one, this made the code even smaller.
> 
> > - Made the if (dst < src) swap() in the hmark_hash() since it should be used by every caller.
> 
> Not really, you don't need for the conntrack part. The original tuple
> is always the same, not matter where the packet is coming from. I have
> removed this again so it only affects packet-based hashing.

Yes original tuple is always the same but not always less than the rtuple.
If you have two nodes that should produce the same hmark,
one with conntrack an one without you must make a compare to make it consistent.

> 
> > - Moved the L3 check a little bit earlier.
> 
> good.
> 
> > - changed return values for fragments.
> 
> With this, you're giving up on trying to classify fragments. Do you
> really want this?
> 
> From my point of view, if your firewalls (assuming they are the HMARK
> classification) are stateless, it still makes sense to me to classify
> fragments using the XT_HMARK_METHOD_L3_4.

I do agree, it is back to "return 0" again.

> 
> > - Added nhoffs to: hmark_set_tuple_ports(skb, (ip->ihl * 4) + nhoff, t, info);
> >   to get icmp working
> 
> good catch.
> 
> Below, some minor changes that I made to your patch (you can find a
> new version enclosed to this email).
> 
> [...]
> > +#ifndef XT_HMARK_H_
> > +#define XT_HMARK_H_
> > +
> > +#include <linux/types.h>
> > +
> > +enum {
> > +     XT_HMARK_NONE,
> > +     XT_HMARK_SADR_AND,
> > +     XT_HMARK_DADR_AND,
> > +     XT_HMARK_SPI_AND,
> > +     XT_HMARK_SPI_OR,
> > +     XT_HMARK_SPORT_AND,
> > +     XT_HMARK_DPORT_AND,
> > +     XT_HMARK_SPORT_OR,
> > +     XT_HMARK_DPORT_OR,
> > +     XT_HMARK_PROTO_AND,
> > +     XT_HMARK_RND,
> > +     XT_HMARK_MODULUS,
> > +     XT_HMARK_OFFSET,
> > +     XT_HMARK_CT,
> > +     XT_HMARK_METHOD_L3,
> > +     XT_HMARK_METHOD_L3_4,
> > +     XT_F_HMARK_SADR_AND    = 1 << XT_HMARK_SADR_AND,
> > +     XT_F_HMARK_DADR_AND    = 1 << XT_HMARK_DADR_AND,
> > +     XT_F_HMARK_SPI_AND     = 1 << XT_HMARK_SPI_AND,
> > +     XT_F_HMARK_SPI_OR      = 1 << XT_HMARK_SPI_OR,
> > +     XT_F_HMARK_SPORT_AND   = 1 << XT_HMARK_SPORT_AND,
> > +     XT_F_HMARK_DPORT_AND   = 1 << XT_HMARK_DPORT_AND,
> > +     XT_F_HMARK_SPORT_OR    = 1 << XT_HMARK_SPORT_OR,
> > +     XT_F_HMARK_DPORT_OR    = 1 << XT_HMARK_DPORT_OR,
> > +     XT_F_HMARK_PROTO_AND   = 1 << XT_HMARK_PROTO_AND,
> > +     XT_F_HMARK_RND         = 1 << XT_HMARK_RND,
> > +     XT_F_HMARK_MODULUS     = 1 << XT_HMARK_MODULUS,
> > +     XT_F_HMARK_OFFSET      = 1 << XT_HMARK_OFFSET,
> > +     XT_F_HMARK_CT          = 1 << XT_HMARK_CT,
> > +     XT_F_HMARK_METHOD_L3   = 1 << XT_HMARK_METHOD_L3,
> > +     XT_F_HMARK_METHOD_L3_4 = 1 << XT_HMARK_METHOD_L3_4,
> 
> I've defined:
> 
> #define XT_HMARK_FLAG(flag) (1 << flag)
> 
> So we save all those extra _F_ defintions, they look redundant.

OK, I had to change the user mode code to keep up with this change...
The user code part is also included now.

[snip]

>+static inline u32
>+hmark_addr_mask(int l3num, const __u32 *addr32, const __u32 *mask)
>+{
>+       switch (l3num) {
              ^
Added a space here

>+       case AF_INET:
>+               return *addr32 & *mask;
>+       case AF_INET6:
>+               return hmark_addr6_mask(addr32, mask);


-- 
Regards
Hans Schillstrom <hans.schillstrom@...csson.com>

View attachment "0001-netfilter-add-xt_hmark-target-for-hash-based-skb-mar.patch" of type "text/x-patch" (13630 bytes)

View attachment "0001-netfilter-userspace-part-for-target-HMARK.patch" of type "text/x-patch" (23980 bytes)