Extensions for ICMP[6] with sport, dport

Extensions for ICMP[6] with sport, dport

[Rick van Rein]

Hello Patrick McHardy / NFT,

I'm using NetFilter for static firewalling.  Ideally with ICMP, for
which I found that a minor extension might be helpful, adding selectors
for icmp|icmp6|l4proto sport|dport.  This avoids painstaking detail to
carry ICMP, and may be helpful to have mature firewalls more easily.
Would you agree that this is a useful extension?

Interpretation of IP content is valid for error types; for ICMP, those
are 3,11,12,31, for ICMP6, those are 1,2,3,4; this should be checked
elsewhere in the ruleset.  The code supports "l4proto" selection of ICMP
with the same rules as TCP et al.  (But a better implementation of
"l4proto" in meta.c would skip IP option headers and ICMP headers with
error types to actually arrive at layer 4, IMHO).

A sketch of code is below; I am unsure about the [THDR_?PORT] but I
think the "sport" and "dport" should be interpreted in reverse for ICMP,
as it travels upstream.  That would match "l4proto sport" match ICMP
along with the TCP, UDP, SCTP and DCCP to which it relates.  It also
seems fair that ICMP with a "dport" targets the port at the ICMP target,
so the originator of the initial message.


If you want me to continue on this, I need to find a way into
git.kernel.org and how to offer code.  Just point me to howto's.  I also
could write a Wiki about Stateful Filter WHENTO-and-HOWTO.


Cheers,
 -Rick


struct icmphdr_udphdr {
	struct icmphdr ih;
	struct udphdr uh;
};

const struct proto_desc proto_icmp = {
	…
        .templates      = {
		…
		/* ICMP travels upstream; we reverse sport/dport for icmp/l4proto */
                [THDR_SPORT]            = INET_SERVICE(“sport", struct
icmphdr_udphdr, uh.dest  ),
                [THDR_DPORT]            = INET_SERVICE(“dport", struct
icmphdr_udphdr, uh.source),
		// Unsure about these indexes…
        },
	…
};

struct icmp6hdr_udphdr {
	struct icmp6hdr ih;
	struct udphdr uh;
};


const struct proto_desc proto_icmp6 = {
	…
        .templates      = {
		…
		/* ICMP travels upstream; we reverse sport/dport for icmp6/l4proto */
                [THDR_SPORT]            = INET_SERVICE(“sport", struct
icmphdr_udphdr, uh.dest),
                [THDR_DPORT]            = INET_SERVICE(“dport", struct
icmphdr_udphdr, uh.source),
		// Unsure about these indexes…
        },
	…
};

Re: Extensions for ICMP[6] with sport, dport

[Duncan Roe]

On Mon, Jun 08, 2020 at 07:31:01PM +0200, Rick van Rein wrote:
> Hello Patrick McHardy / NFT,
>
> I'm using NetFilter for static firewalling.  Ideally with ICMP, for
> which I found that a minor extension might be helpful, adding selectors
> for icmp|icmp6|l4proto sport|dport.  This avoids painstaking detail to
> carry ICMP, and may be helpful to have mature firewalls more easily.
> Would you agree that this is a useful extension?
>
> Interpretation of IP content is valid for error types; for ICMP, those
> are 3,11,12,31, for ICMP6, those are 1,2,3,4; this should be checked
> elsewhere in the ruleset.  The code supports "l4proto" selection of ICMP
> with the same rules as TCP et al.  (But a better implementation of
> "l4proto" in meta.c would skip IP option headers and ICMP headers with
> error types to actually arrive at layer 4, IMHO).
>
> A sketch of code is below; I am unsure about the [THDR_?PORT] but I
> think the "sport" and "dport" should be interpreted in reverse for ICMP,
> as it travels upstream.  That would match "l4proto sport" match ICMP
> along with the TCP, UDP, SCTP and DCCP to which it relates.  It also
> seems fair that ICMP with a "dport" targets the port at the ICMP target,
> so the originator of the initial message.
>
>
> If you want me to continue on this, I need to find a way into
> git.kernel.org and how to offer code.  Just point me to howto's.  I also
> could write a Wiki about Stateful Filter WHENTO-and-HOWTO.
>
>
> Cheers,
>  -Rick
>
>
> struct icmphdr_udphdr {
> 	struct icmphdr ih;
> 	struct udphdr uh;
> };
>
> const struct proto_desc proto_icmp = {
> 	???
>         .templates      = {
> 		???
> 		/* ICMP travels upstream; we reverse sport/dport for icmp/l4proto */
>                 [THDR_SPORT]            = INET_SERVICE(???sport", struct
> icmphdr_udphdr, uh.dest  ),
>                 [THDR_DPORT]            = INET_SERVICE(???dport", struct
> icmphdr_udphdr, uh.source),
> 		// Unsure about these indexes???
>         },
> 	???
> };
>
> struct icmp6hdr_udphdr {
> 	struct icmp6hdr ih;
> 	struct udphdr uh;
> };
>
>
> const struct proto_desc proto_icmp6 = {
> 	???
>         .templates      = {
> 		???
> 		/* ICMP travels upstream; we reverse sport/dport for icmp6/l4proto */
>                 [THDR_SPORT]            = INET_SERVICE(???sport", struct
> icmphdr_udphdr, uh.dest),
>                 [THDR_DPORT]            = INET_SERVICE(???dport", struct
> icmphdr_udphdr, uh.source),
> 		// Unsure about these indexes???
>         },
> 	???
> };
Hi Rick,

Usually people submit patches to netfilter-devel using git format-patch and
git send-email.

You should submit patches against the nf-next tree, which you can clone from
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git

Cheers ... Duncan.

Re: Extensions for ICMP[6] with sport, dport

[Florian Westphal]

Rick van Rein <rick@openfortress.nl> wrote:

[ dropped patrick from cc ]

> A sketch of code is below; I am unsure about the [THDR_?PORT] but I
> think the "sport" and "dport" should be interpreted in reverse for ICMP,
> as it travels upstream.  That would match "l4proto sport" match ICMP
> along with the TCP, UDP, SCTP and DCCP to which it relates.  It also
> seems fair that ICMP with a "dport" targets the port at the ICMP target,
> so the originator of the initial message.
> 
> 
> If you want me to continue on this, I need to find a way into
> git.kernel.org and how to offer code.  Just point me to howto's.  I also
> could write a Wiki about Stateful Filter WHENTO-and-HOWTO.

I think instead of this specific use case it would be preferrable to
tackle this in a more general way, via more generic "ip - in foo"
matching.

See
https://people.netfilter.org/2019/wiki/index.php/General_Agenda#match_packets_inside_tunnels

for a summary of inner header matching.

I suspect that for this case we would want something like

filter forward inner ip in icmp tcp dport 42

It would require lots of kernel changes, for example a new displaycement
register and changes to existing payload expression to use it, so it
would access the embedded tcp header.

Re: Extensions for ICMP[6] with sport, dport

[Rick van Rein]

Hey Florian,

You are generalising ICMP matching to tunnel matching, and then conclude that it is quite complex.

Yes, there is some overlap, but ICMP is also quite different:

 - Encapsulated traffic travels in reverse compared to a tunnel
 - ICMP-contained content must be NAT-reversed, unlike tunnel content
 - ICMP carries cut-off headers: IP header + 8 bytes of its payload
 - In practice, the contents of interest would be ip|ip6 and th
 - References "icmp ip" and "icmp th" are simple-yet-enough
 - General tunnel logic may be less efficient?

I originally proposed to treat ICMP as a side-case to TCP et al, but that won't fly.  There's also an embedded saddr/daddr pair to be treated.  This also means that the silent reversal of sport/dport is not needed, which is a relief.

> I think instead of this specific use case it would be preferrable to
> tackle this in a more general way, via more generic "ip - in foo"
> matching.

Given the differences above, do you still think so?

I would argue that these provide (not 100% hard) reasons to treat ICMP differently from tunnels.  Possibly syntaxes, in line with what "nft" does now, could say things like

ip protocol icmp
icmp protocol { tcp, udp, sctp, dccp }
icmp th daddr set
   icmp th dport map @my-nat-map

This looks like an extension of the nft command, under my assumption that it computes fixed offsets.  There may be more trouble with two-variable comparisons, which would cover paranoid checks like

icmp daddr = saddr
icmp saddr = daddr
icmp th dport = sport
icmp th sport = dport


If this ends up being kernel work, then I'm afraid I will have to let go.


Thanks,
 -Rick

Re: Extensions for ICMP[6] with sport, dport

[Florian Westphal]

Rick van Rein <rick@openfortress.nl> wrote:
>  - Encapsulated traffic travels in reverse compared to a tunnel
>  - ICMP-contained content must be NAT-reversed, unlike tunnel content

nf_nat takes care of this automatically.

> 
> I would argue that these provide (not 100% hard) reasons to treat ICMP differently from tunnels.  Possibly syntaxes, in line with what "nft" does now, could say things like
> 
> ip protocol icmp
> icmp protocol { tcp, udp, sctp, dccp }

What would that do?  "ip protocol" of embedded ip header?

> icmp th daddr set
>    icmp th dport map @my-nat-map

th daddr looks weird to me, but syntax could
be changed later.

Dependency generation and delineraization would
likely be more of a challenge.

> If this ends up being kernel work, then I'm afraid I will have to let go.

It can probably be done using fixed offsets for this
specific case but its likely a lot of work wrt. dependency
checking and providing readable syntax for "nft list" again.

Re: Extensions for ICMP[6] with sport, dport

[Rick van Rein]

Hi Florian,

>>  - ICMP-contained content must be NAT-reversed, unlike tunnel content
> nf_nat takes care of this automatically.

Hmyeah, I know.  I am really trying with stateless NAT, which I know is not the general advise.  Intending to push p2p applications, I fear that things like a TCP SYN from two ends at the same time are asking for trouble.

>> ip protocol icmp
>> icmp protocol { tcp, udp, sctp, dccp }
>
> What would that do?  "ip protocol" of embedded ip header?

Yes, that's what I meant with this.

>> icmp th daddr set
>>    icmp th dport map @my-nat-map
>
> th daddr looks weird to me, but syntax could
> be changed later.

Yes, I made a mistake.  It should have said "icmp ip daddr set".

On a side note, I found a good reason for "th daddr" instead of "@th,16,16" that goes beyond readability: its typing can be used in a map with inet_service, unlike @th,16,16 which is a variably-sized integer.

>> If this ends up being kernel work, then I'm afraid I will have to let go.
>
> It can probably be done using fixed offsets for this
> specific case but its likely a lot of work wrt. dependency
> checking and providing readable syntax for "nft list" again.

Thanks for warning me upfront.  I will try to stay away from it then until I really have the time.

Best,
 -Rick