netdev - Re: [PATCH 6/6] netfilter: nf_nat: Handle routing changes in MASQUERADE target

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] [day] [month] [year] [list]

Date:	Wed, 12 Dec 2012 19:37:16 +0100 (CET)
From:	Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>
To:	Andrew Collins <bsderandrew@...il.com>
cc:	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 6/6] netfilter: nf_nat: Handle routing changes in MASQUERADE
 target

On Tue, 11 Dec 2012, Andrew Collins wrote:

> On Tue, Dec 4, 2012 at 10:31 AM, <pablo@...filter.org> wrote:
> >
> > From: Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>
> >
> > When the route changes (backup default route, VPNs) which affect a
> > masqueraded target, the packets were sent out with the outdated source
> > address. The patch addresses the issue by comparing the outgoing interface
> > directly with the masqueraded interface in the nat table.
> >
> > Events are inefficient in this case, because it'd require adding route
> > events to the network core and then scanning the whole conntrack table
> > and re-checking the route for all entry.
> >
> > Signed-off-by: Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>
> > Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> 
> Jozsef, a small question about this change.  Should this same check
> not exist here:
> 
>         case IP_CT_NEW:
>                 /* Seen it before?  This can happen for loopback, retrans,
>                  * or local packets.
>                  */
>                 if (!nf_nat_initialized(ct, maniptype)) {
>                         unsigned int ret;
> 
>                         ret = nf_nat_rule_find(skb, hooknum, in, out, ct);
>                         if (ret != NF_ACCEPT)
>                                 return ret;
> -               } else
> +               } else {
>                         pr_debug("Already setup manip %s for ct %p\n",
>                                  maniptype == NF_NAT_MANIP_SRC ? "SRC" : "DST",
>                                  ct);
> +                       if (nf_nat_oif_changed(hooknum, ctinfo, nat, out)) {
> +                               nf_ct_kill_acct(ct, ctinfo, skb);
> +                               return NF_DROP;
> +                       }
> +               }
>                 break;
> 
> as well?  It's *significantly* less common than the case you fixed,
> and perhaps just letting the state time out is acceptable, but I've
> seen TCP connections get stuck with the wrong source address if we
> haven't hit ESTABLISHED at the point when the routing change occurs
> (most reproducible on high latency links).

It is a less common case, but I think you are right: the timeout can take 
several minutes. But instead of repeating the code segment, a "goto" case 
handling were better. Are you going to submit a patch?

Best regards,
Jozsef

-
E-mail  : kadlec@...ckhole.kfki.hu, kadlecsik.jozsef@...ner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html