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@xxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
