| Message ID | 20180618095710.13306-1-ecklm94@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] evaluate: Detect address family in inet context | expand |
Hi Máté, Please, provide a meaning full patch description on why this is need and resubmit. Thanks! -- 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 Mon, Jun 18, 2018 at 11:57:10AM +0200, Máté Eckl wrote: > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > --- > src/evaluate.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/src/evaluate.c b/src/evaluate.c > index d6aff61..0564b44 100644 > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -2431,12 +2431,28 @@ static int evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt, > const struct datatype *dtype; > unsigned int len; > > - if (pctx->family == NFPROTO_IPV4) { > + switch (pctx->family) { > + case NFPROTO_IPV4: > dtype = &ipaddr_type; > len = 4 * BITS_PER_BYTE; > - } else { > + break; > + case NFPROTO_IPV6: > dtype = &ip6addr_type; > len = 16 * BITS_PER_BYTE; > + break; > + case NFPROTO_INET: > + if (strchr((*expr)->identifier, ':')) { I'd suggest you specify this in this syntax: tproxy ip to 1.1.1.1 for the bridge/netdev/inet families. From the kernel, this will also skip non-IP packets, so we don't need to build an IP dependency for this statement. -- 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 Wed, Jun 20, 2018 at 01:40:45PM +0200, Pablo Neira Ayuso wrote: > On Mon, Jun 18, 2018 at 11:57:10AM +0200, Máté Eckl wrote: > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > --- > > src/evaluate.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/src/evaluate.c b/src/evaluate.c > > index d6aff61..0564b44 100644 > > --- a/src/evaluate.c > > +++ b/src/evaluate.c > > @@ -2431,12 +2431,28 @@ static int evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt, > > const struct datatype *dtype; > > unsigned int len; > > > > - if (pctx->family == NFPROTO_IPV4) { > > + switch (pctx->family) { > > + case NFPROTO_IPV4: > > dtype = &ipaddr_type; > > len = 4 * BITS_PER_BYTE; > > - } else { > > + break; > > + case NFPROTO_IPV6: > > dtype = &ip6addr_type; > > len = 16 * BITS_PER_BYTE; > > + break; > > + case NFPROTO_INET: > > + if (strchr((*expr)->identifier, ':')) { > > I'd suggest you specify this in this syntax: > > tproxy ip to 1.1.1.1 > > for the bridge/netdev/inet families. > > From the kernel, this will also skip non-IP packets, so we don't need > to build an IP dependency for this statement. This patch solves a problem regardless of the tproxy functionality, as it was impossible to specify an address other than ipv6 in non-ip tables. Tproxy was only an example to demonstrate the error. If this patch is applied, there is no need for the 'ip' here (and I'd like to avoid it). Bridge and netdev tables are not supported to use tproxy in. -- 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 Wed, Jun 20, 2018 at 02:21:18PM +0200, Máté Eckl wrote: > On Wed, Jun 20, 2018 at 01:40:45PM +0200, Pablo Neira Ayuso wrote: > > On Mon, Jun 18, 2018 at 11:57:10AM +0200, Máté Eckl wrote: > > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > > --- > > > src/evaluate.c | 20 ++++++++++++++++++-- > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/evaluate.c b/src/evaluate.c > > > index d6aff61..0564b44 100644 > > > --- a/src/evaluate.c > > > +++ b/src/evaluate.c > > > @@ -2431,12 +2431,28 @@ static int evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt, > > > const struct datatype *dtype; > > > unsigned int len; > > > > > > - if (pctx->family == NFPROTO_IPV4) { > > > + switch (pctx->family) { > > > + case NFPROTO_IPV4: > > > dtype = &ipaddr_type; > > > len = 4 * BITS_PER_BYTE; > > > - } else { > > > + break; > > > + case NFPROTO_IPV6: > > > dtype = &ip6addr_type; > > > len = 16 * BITS_PER_BYTE; > > > + break; > > > + case NFPROTO_INET: > > > + if (strchr((*expr)->identifier, ':')) { > > > > I'd suggest you specify this in this syntax: > > > > tproxy ip to 1.1.1.1 > > > > for the bridge/netdev/inet families. > > > > From the kernel, this will also skip non-IP packets, so we don't need > > to build an IP dependency for this statement. > > This patch solves a problem regardless of the tproxy functionality, as it was > impossible to specify an address other than ipv6 in non-ip tables. Tproxy was > only an example to demonstrate the error. > > If this patch is applied, there is no need for the 'ip' here (and I'd like to > avoid it). Bridge and netdev tables are not supported to use tproxy in. For ip/ip6, tproxy to 1.1.1.1 is fine. But for bridge/netdev/inet, I think it makes explicit the dependency with either ip and ip6. So this is visible from the ruleset that this rule will only apply to either ip or ip6. And we'll skip having to generate a dependency for this, which is always more work, given that from the kernel we can just add: if (skb->protocol != htons(ETH_P_IP)) ... break verdict ... which is actually needed for safety reasons. I think your kernel patch is also lacking this, and a custom userspace program may add a tproxy expression to deal with IPV6 traffic, which may result in crashing the kernel. -- 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 Wed, Jun 20, 2018 at 02:26:36PM +0200, Pablo Neira Ayuso wrote: > On Wed, Jun 20, 2018 at 02:21:18PM +0200, Máté Eckl wrote: > > On Wed, Jun 20, 2018 at 01:40:45PM +0200, Pablo Neira Ayuso wrote: > > > On Mon, Jun 18, 2018 at 11:57:10AM +0200, Máté Eckl wrote: > > > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > > > --- > > > > src/evaluate.c | 20 ++++++++++++++++++-- > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/evaluate.c b/src/evaluate.c > > > > index d6aff61..0564b44 100644 > > > > --- a/src/evaluate.c > > > > +++ b/src/evaluate.c > > > > @@ -2431,12 +2431,28 @@ static int evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt, > > > > const struct datatype *dtype; > > > > unsigned int len; > > > > > > > > - if (pctx->family == NFPROTO_IPV4) { > > > > + switch (pctx->family) { > > > > + case NFPROTO_IPV4: > > > > dtype = &ipaddr_type; > > > > len = 4 * BITS_PER_BYTE; > > > > - } else { > > > > + break; > > > > + case NFPROTO_IPV6: > > > > dtype = &ip6addr_type; > > > > len = 16 * BITS_PER_BYTE; > > > > + break; > > > > + case NFPROTO_INET: > > > > + if (strchr((*expr)->identifier, ':')) { > > > > > > I'd suggest you specify this in this syntax: > > > > > > tproxy ip to 1.1.1.1 > > > > > > for the bridge/netdev/inet families. > > > > > > From the kernel, this will also skip non-IP packets, so we don't need > > > to build an IP dependency for this statement. > > > > This patch solves a problem regardless of the tproxy functionality, as it was > > impossible to specify an address other than ipv6 in non-ip tables. Tproxy was > > only an example to demonstrate the error. > > > > If this patch is applied, there is no need for the 'ip' here (and I'd like to > > avoid it). Bridge and netdev tables are not supported to use tproxy in. > > For ip/ip6, tproxy to 1.1.1.1 is fine. > > But for bridge/netdev/inet, I think it makes explicit the dependency > with either ip and ip6. So this is visible from the ruleset that this > rule will only apply to either ip or ip6. I thought that the address itself will be explicit enough to decide which of the families the rule deals with. I (as a user) would never think that any ipv4 address will be used for ipv6 socket lookup. If you disagree, I can specify it in case of address is specified, I just liked the idea of not having to specify this. > And we'll skip having to generate a dependency for this, which is > always more work, given that from the kernel we can just add: I might not understand clearly what you mean by dependency. I thought that ip, ip6 and inet tables only receive packets that have ipv4/ipv6 network layer header, so this dependency is satisfied by definition, doesn't it? I don't think I generated any dependency explicitly. > if (skb->protocol != htons(ETH_P_IP)) > ... break verdict ... > > which is actually needed for safety reasons. This is something that should appear in the eval function right? Isn't it the same as what I added there? switch (nft_pf(pkt)) { case NFPROTO_IPV4: switch (priv->family) { case NFPROTO_IPV4: case NFPROTO_INET: nft_tproxy_eval_v4(expr, regs, pkt); break; default: regs->verdict.code = NFT_BREAK; break; } break; #if IS_ENABLED(CONFIG_NF_TPROXY_IPV6) case NFPROTO_IPV6: switch (priv->family) { case NFPROTO_IPV6: case NFPROTO_INET: nft_tproxy_eval_v6(expr, regs, pkt); break; default: regs->verdict.code = NFT_BREAK; break; } break; #endif } > I think your kernel patch is also lacking this, and a custom userspace > program may add a tproxy expression to deal with IPV6 traffic, which > may result in crashing the kernel. Given the switch-case I don't think an ipv6 packet can pass to ipv4 evaluation. Am I missing something? -- 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 Fri, Jun 22, 2018 at 11:45:12AM +0200, Máté Eckl wrote: [...] > > if (skb->protocol != htons(ETH_P_IP)) > > ... break verdict ... > > > which is actually needed for safety reasons. > > This is something that should appear in the eval function right? Also in the kernel for safety reasons. > Isn't it the same as what I added there? This is needed because someone may use the raw kernel netlink interface (not libnftnl / nftables) to generate an incorrect combination such as allow IPv6 to be passed to tproxy in IPV4 mode which may crash the kernel. Well, it may be just result in a packet drop, but better be safe than sorry. -- 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 Fri, Jun 22, 2018 at 11:53:44AM +0200, Pablo Neira Ayuso wrote: > On Fri, Jun 22, 2018 at 11:45:12AM +0200, Máté Eckl wrote: > [...] > > > if (skb->protocol != htons(ETH_P_IP)) > > > ... break verdict ... > > > > > which is actually needed for safety reasons. > > > > This is something that should appear in the eval function right? > > Also in the kernel for safety reasons. I meant the kernel eval function (which evaluates packets against rules). > > Isn't it the same as what I added there? > > This is needed because someone may use the raw kernel netlink > interface (not libnftnl / nftables) to generate an incorrect > combination such as allow IPv6 to be passed to tproxy in IPV4 mode > which may crash the kernel. > > Well, it may be just result in a packet drop, but better be safe than > sorry. The code snippet included in my previous email was also from the kernel code and it seems to provide the same check as the 'if' you suggested. -- 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 Pablo, Could we make things clear about this discussion? I think we have some misunderstandings, and I am certainly unaware of some concerns you are trying to enforce. How about discussing them one by one? = Detect or specify address family in tproxy command = I submitted a patch to detect the address family in evaluate_addr in case the table is INET and print an error if table is not IPV4, IPV6 or INET. Do you disagree with this solution in general or only in case of tproxy? In general, I think it is better to have explicit checks here as the error message generated by the current solution may be misleading in a situation like mine (basically every time trying ot evaluate an ipv4 address to a rule in inet table). Regarding family detection, you say family should be specified in the rule in case the table is inet. Do you say this because of UX or technical reasons? I think, the technical part can easily be covered (as I do in stmt_evaluate_tproxy) with setting a family attribute after a successfull evaluation and sending it to the kernel. As a consequence the family of the rule match is not ambiguous technically. The UX part is more subjective by nature. In my opinion, if a user specifies an IPv4 address he/she expects that it will match IPv4 for packets and the same with IPv6. Therefor my intention is to leave explicit specification out. Do you think it is not this obvious? In tproxy case, the kernel will receive a family attribute with the value of IPV4 or IPV6 in case an address is specified so it will not be necessary to generate ip dependency to an ipv6 rule (although I don't think I generate any dependency explicitly) and this information is used for matching, so kernel has sufficient information of the family of the role. = Dependency = You mentioned some kind of dependency in some of your emails, but I don't know what you think of. Could you describe what a dependency is in this context? Is it in user- or kernelspace? For ip/ip6/inet tables isn't that default to process *only* packets with IPv4 or IPv6 header? If it is default, I don't understand why any other dependency should be generated, I can just check the family of the packet against the family of the, can't I? You bring bridge/netdev as examples, but tproxy is not supported in these tables (refused from nft_tproxy_init in the kernel), so I don't see why we should consider them. Regarding the address family detection, the evaluate_addr function only cares about L3 addresses so bridge/netdev seems to be irrelevant here, too. = Kernel, address family check = You had a comment in an email which I still don't really understand. I think your kernel patch is also lacking this, and a custom userspace program may add a tproxy expression to deal with IPV6 traffic, which may result in crashing the kernel. Do you mean that a program can add an expression with an IPv6 address with a family of NFPROTO_IPV4? Then I suppose the nft_validate_register_load should return error in the init function, shouldn't it? Do you mean that address may be specified whereas tproxy family is left on default? Yes, this was possible, I added a check for this in my v2 patch. Do you mean, that an IPv6 packet can be passed to the IPv4 evaluation function? Your code snippet seems to be related to this. It examined if the L3 header is IP header, but it really seems to be the same as what I do (at least in ip/ip6/inet tables). I compare the packet's family (from nft_pf()) and the tproxy family, and I pass the packet to the v4 or v6 eval function respectively. The L3 header should be present so this should work securely with such a strict init function. If none of these, could you please define the scenario with more details? In which point do you think my code is voulnerable and what is a scenario? I hope I grabbed the important points to make this situation clear. It would be nice if you could reply to all these notes and questions as I am confused about what we disagree on right now. Regards, Máté -- 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 Tue, Jun 26, 2018 at 03:54:51PM +0200, Máté Eckl wrote: [...] > Do you disagree with this solution in general or only in case of tproxy? > > In general, I think it is better to have explicit checks here as the error > message generated by the current solution may be misleading in a situation like > mine (basically every time trying ot evaluate an ipv4 address to a rule in inet > table). Please, don't do this character probing from the evaluation step. It's a hack. I would like not to see patches that address parser limitations from the evaluation phase. [...] > = Dependency = > > You mentioned some kind of dependency in some of your emails, but I don't know > what you think of. Could you describe what a dependency is in this context? Is > it in user- or kernelspace? > > For ip/ip6/inet tables isn't that default to process *only* packets with IPv4 or > IPv6 header? If it is default, I don't understand why any other dependency > should be generated, I can just check the family of the packet against the > family of the, can't I? I was refering to the inet family, specifically. > You bring bridge/netdev as examples, but tproxy is not supported in these tables > (refused from nft_tproxy_init in the kernel), so I don't see why we should > consider them. Regarding the address family detection, the evaluate_addr > function only cares about L3 addresses so bridge/netdev seems to be irrelevant > here, too. Right, there's no bridge/netdev in tproxy .I was arguing about the syntax for this command and commands that look similar, like dup and fwd. Never mind about the dependency, actually "Kernel, address family check" and "Dependency" are the same thing. > = Kernel, address family check = > > You had a comment in an email which I still don't really understand. > > I think your kernel patch is also lacking this, and a custom userspace > program may add a tproxy expression to deal with IPV6 traffic, which > may result in crashing the kernel. My point is: Just make sure that your code doesn't allow IPv6 packets to run through the IPv4 path, and the other way around. Please, just send a v2 of your patches that we can review, we can address your concerns in a follow up step. If there's any concern, we can revisit, don't worry. -- 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/evaluate.c b/src/evaluate.c index d6aff61..0564b44 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -2431,12 +2431,28 @@ static int evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt, const struct datatype *dtype; unsigned int len; - if (pctx->family == NFPROTO_IPV4) { + switch (pctx->family) { + case NFPROTO_IPV4: dtype = &ipaddr_type; len = 4 * BITS_PER_BYTE; - } else { + break; + case NFPROTO_IPV6: dtype = &ip6addr_type; len = 16 * BITS_PER_BYTE; + break; + case NFPROTO_INET: + if (strchr((*expr)->identifier, ':')) { + dtype = &ip6addr_type; + len = 16 * BITS_PER_BYTE; + } + else { + dtype = &ipaddr_type; + len = 4 * BITS_PER_BYTE; + } + break; + default: + return stmt_binary_error(ctx, *expr, stmt, + "Invalid context family for address evaluation"); } return stmt_evaluate_arg(ctx, stmt, dtype, len, BYTEORDER_BIG_ENDIAN,
Signed-off-by: Máté Eckl <ecklm94@gmail.com> --- src/evaluate.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)