| Message ID | 20180328092725.13045-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] proto: permit icmp-in-ipv6 and icmpv6-in-ipv4 | expand |
Hi, On Wed, Mar 28, 2018 at 11:27:25AM +0200, Florian Westphal wrote: > Jozsef points out that > meta l4proto icmp icmp type destination-unreachable > > is hard to read. So, lets just add icmp/icmpv6 to > ip/ip6 protocol base so users can just go with > > icmp type destination-unreachable Does this then lead to generating protocol dependency in e.g. inet table? Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Phil Sutter <phil@nwl.cc> wrote: > > is hard to read. So, lets just add icmp/icmpv6 to > > ip/ip6 protocol base so users can just go with > > > > icmp type destination-unreachable > > Does this then lead to generating protocol dependency in e.g. inet > table? Whats the expected behaviour there? Currently you will get a dependency via payload_gen_special_dependency(), i.e. icmpv6 in inet will not match icmpv6-in-ipv4. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 06:56:08AM +0200, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > > is hard to read. So, lets just add icmp/icmpv6 to > > > ip/ip6 protocol base so users can just go with > > > > > > icmp type destination-unreachable > > > > Does this then lead to generating protocol dependency in e.g. inet > > table? > > Whats the expected behaviour there? I was just curious. :) > Currently you will get a dependency via > payload_gen_special_dependency(), i.e. icmpv6 in inet will > not match icmpv6-in-ipv4. Sounds good! I think the most intuitive behaviour would be: family | rule | effect --------------------------------------------------------------- ip | icmp type foo | icmp-in-ipv4 | icmpv6 type foo | icmpv6-in-ipv4 --------------------------------------------------------------- ip6 | icmp type foo | icmp-in-ipv6 | icmpv6 type foo | icmpv6-in-ipv6 --------------------------------------------------------------- inet | icmp type foo | icmp-in-ipv4 or icmp-in-ipv6 | icmpv6 type foo | icmpv6-in-ipv4 or icmpv6-in-ipv4 --------------------------------------------------------------- I guess this differs from the current state only in the 'or' part of inet family, right? Or does nftables reject plain icmp/icmpv6 payload matches in inet family if l3proto has not been specified? Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 08:39:40AM +0200, Phil Sutter wrote: > On Thu, Mar 29, 2018 at 06:56:08AM +0200, Florian Westphal wrote: > > Phil Sutter <phil@nwl.cc> wrote: > > > > is hard to read. So, lets just add icmp/icmpv6 to > > > > ip/ip6 protocol base so users can just go with > > > > > > > > icmp type destination-unreachable > > > > > > Does this then lead to generating protocol dependency in e.g. inet > > > table? > > > > Whats the expected behaviour there? > > I was just curious. :) > > > Currently you will get a dependency via > > payload_gen_special_dependency(), i.e. icmpv6 in inet will > > not match icmpv6-in-ipv4. > > Sounds good! I think the most intuitive behaviour would be: > > family | rule | effect > --------------------------------------------------------------- > ip | icmp type foo | icmp-in-ipv4 > | icmpv6 type foo | icmpv6-in-ipv4 > --------------------------------------------------------------- > ip6 | icmp type foo | icmp-in-ipv6 > | icmpv6 type foo | icmpv6-in-ipv6 These two above look good to me. > --------------------------------------------------------------- > inet | icmp type foo | icmp-in-ipv4 or icmp-in-ipv6 > | icmpv6 type foo | icmpv6-in-ipv4 or icmpv6-in-ipv4 So you mean, we just look at the transport protocol field? That would simplify things since we would not need dependencies at all. We discussed that we should actually enforce ipv4 if you ask for 'icmp type foo' from inet, otherwise we would just let things like a forged IPv6 header with ICMP go through, which may be a bit counterintuitive to users? I mean, for a tight ruleset from inet chains, users would need to do 'meta protocol ip icmp type foo' to make sure forged packets don't go through. > --------------------------------------------------------------- > > I guess this differs from the current state only in the 'or' part of > inet family, right? Or does nftables reject plain icmp/icmpv6 payload > matches in inet family if l3proto has not been specified? If l3proto is not specified, the idea is to infer it from icmp. We should still allow things like: meta protocol ip6 icmp type foo which is broken right now, since we should allow users to make any arbitrary matching, even if it will be unlikely to see this packet. So if we stick to this path, this needs to be fixed as it is broken right now. Makes sense to you? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Mar 30, 2018 at 12:17:56PM +0200, Pablo Neira Ayuso wrote: > On Thu, Mar 29, 2018 at 08:39:40AM +0200, Phil Sutter wrote: [...] > > Sounds good! I think the most intuitive behaviour would be: > > > > family | rule | effect > > --------------------------------------------------------------- > > ip | icmp type foo | icmp-in-ipv4 > > | icmpv6 type foo | icmpv6-in-ipv4 > > --------------------------------------------------------------- > > ip6 | icmp type foo | icmp-in-ipv6 > > | icmpv6 type foo | icmpv6-in-ipv6 > > These two above look good to me. > > > --------------------------------------------------------------- > > inet | icmp type foo | icmp-in-ipv4 or icmp-in-ipv6 > > | icmpv6 type foo | icmpv6-in-ipv4 or icmpv6-in-ipv4 > > So you mean, we just look at the transport protocol field? > > That would simplify things since we would not need dependencies at all. > > We discussed that we should actually enforce ipv4 if you ask for 'icmp > type foo' from inet, otherwise we would just let things like a forged > IPv6 header with ICMP go through, which may be a bit counterintuitive > to users? I mean, for a tight ruleset from inet chains, users would > need to do 'meta protocol ip icmp type foo' to make sure forged > packets don't go through. Yes, that approach makes sense in that it avoids pitfalls and simplifies things for users. OTOH it is inconsistent: | add rule inet t c tcp dport 22 accept will allow SSH via IPv4 as well as IPv6, but | add rule inet t c icmp type echo-request accept will allow ICMP pings via IPv4 only. Sure, since ICMP/ICMPv6 are so tightly bound to IPv4/IPv6, I guess it's OK to have a special case for them. > > --------------------------------------------------------------- > > > > I guess this differs from the current state only in the 'or' part of > > inet family, right? Or does nftables reject plain icmp/icmpv6 payload > > matches in inet family if l3proto has not been specified? > > If l3proto is not specified, the idea is to infer it from icmp. > > We should still allow things like: > > meta protocol ip6 icmp type foo which is broken right now, since we > should allow users to make any arbitrary matching, even if it will be > unlikely to see this packet. So if we stick to this path, this needs > to be fixed as it is broken right now. So from my perspective the (sensible) special treatment of icmp/icmpv6 expressions in inet family adds complexity to the code (due to dependency generation/elimination) and is buggy (as you stated). If we drop it, code is simplified and the bugs are "fixed" along the way. To me that's motivation enough to go that path and maybe reconsider it if someone complains. After all, the concept of inet family is new in Linux firewalling, so I'd expect users to give their ruleset an extra thought when migrating to it. Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/proto.c b/src/proto.c index a54090a..8cf29d2 100644 --- a/src/proto.c +++ b/src/proto.c @@ -591,6 +591,7 @@ const struct proto_desc proto_ip = { .checksum_key = IPHDR_CHECKSUM, .protocols = { PROTO_LINK(IPPROTO_ICMP, &proto_icmp), + PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), PROTO_LINK(IPPROTO_ESP, &proto_esp), PROTO_LINK(IPPROTO_AH, &proto_ah), PROTO_LINK(IPPROTO_COMP, &proto_comp), @@ -718,6 +719,7 @@ const struct proto_desc proto_ip6 = { PROTO_LINK(IPPROTO_TCP, &proto_tcp), PROTO_LINK(IPPROTO_DCCP, &proto_dccp), PROTO_LINK(IPPROTO_SCTP, &proto_sctp), + PROTO_LINK(IPPROTO_ICMP, &proto_icmp), PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6), }, .templates = {
Jozsef points out that meta l4proto icmp icmp type destination-unreachable is hard to read. So, lets just add icmp/icmpv6 to ip/ip6 protocol base so users can just go with icmp type destination-unreachable and let nft fill in needed dependency. After this patch, the recent patch to not remove the dependency can be reverted again. Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html