[PATCH net] netfilter: nf_conntrack: fix rt_gateway checks for h323

	After the change "Adjust semantics of rt->rt_gateway"
(commit f8126f1d51) we should properly match the nexthop when
destinations are directly connected because rt_gateway can be 0.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	This patch needs a closer look from the Netfilter team.

	It restores the check as it was committed originally,
i.e. to compare nexthops. I'm not sure what is the desired logic,
it can depend on the following:

- two directly connected hosts (rt_gateway=0) can be from different
subnets or not

- one party A is the gateway (rt_gateway=0), another party uses
this gateway (rt_gateway=A)

	May be someone that knows this code better can comment
if the check should be different.

 net/netfilter/nf_conntrack_h323_main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 1b30b0d..962795e 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c

@@ -753,7 +753,8 @@ static int callforward_do_filter(const union nf_inet_addr *src,
 				   flowi4_to_flowi(&fl1), false)) {
 			if (!afinfo->route(&init_net, (struct dst_entry **)&rt2,
 					   flowi4_to_flowi(&fl2), false)) {
-				if (rt1->rt_gateway == rt2->rt_gateway &&
+				if (rt_nexthop(rt1, fl1.daddr) ==
+				    rt_nexthop(rt2, fl2.daddr) &&
 				    rt1->dst.dev  == rt2->dst.dev)
 					ret = 1;
 				dst_release(&rt2->dst);

-- 
1.7.3.4

Hi Julian,

On Wed, Oct 10, 2012 at 02:00:47AM +0300, Julian Anastasov wrote:

	After the change "Adjust semantics of rt->rt_gateway"
(commit f8126f1d51) we should properly match the nexthop when
destinations are directly connected because rt_gateway can be 0.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	This patch needs a closer look from the Netfilter team.

	It restores the check as it was committed originally,
i.e. to compare nexthops. I'm not sure what is the desired logic,
it can depend on the following:

- two directly connected hosts (rt_gateway=0) can be from different
subnets or not

- one party A is the gateway (rt_gateway=0), another party uses
this gateway (rt_gateway=A)

	May be someone that knows this code better can comment
if the check should be different.

Your patch gets it working like before David's change in the
rt_gateway semantics.

I think the H.323 helper is doing "its best" to handle the following
call-forwarding scenario:

1) A calls B.
2) B replies to A that the alternate address is C.
3) A calls C.

Now assume that:

1) all traffic between A and B goes through the firewall.

and

2) all traffic between A and C don't go through the firewall.

If you want a picture, see section 5.2 of this site:

http://people.netfilter.org/zhaojingmin/h323_conntrack_nat_helper/#_Toc133598073

That code below is trying to detect if A and C don't go through the
firewall, just to skip the creation of one useless expectation (since
they can communicate without going through the firewall).

With the code below (function callforward_do_filter):

a) if A and C are on-link, then:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck, we create the expectation even if we don't need it.

b) if A and C are behind the same next hop:
rt_nexthop(rt1, fl1.daddr) == rt_nexthop(rt2, fl2.daddr)
We don't create the expectation.

c) if A is on-link and C is behind next hop:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck again, we create the expectation again.

This seems documented. We could make it better if we would have a way
to guess that A and C do not need to communicate through the firewall.

I'll take your patch, it leaves things just like it was before (which
was not really good).

Please, let me know if I'm missing anything. Thanks.

	Hello Pablo,

On Wed, 17 Oct 2012, Pablo Neira Ayuso wrote:

Hi Julian,

On Wed, Oct 10, 2012 at 02:00:47AM +0300, Julian Anastasov wrote:

quoted

	After the change "Adjust semantics of rt->rt_gateway"
(commit f8126f1d51) we should properly match the nexthop when
destinations are directly connected because rt_gateway can be 0.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	This patch needs a closer look from the Netfilter team.

	It restores the check as it was committed originally,
i.e. to compare nexthops. I'm not sure what is the desired logic,
it can depend on the following:

- two directly connected hosts (rt_gateway=0) can be from different
subnets or not

- one party A is the gateway (rt_gateway=0), another party uses
this gateway (rt_gateway=A)

	May be someone that knows this code better can comment
if the check should be different.

Your patch gets it working like before David's change in the
rt_gateway semantics.

I think the H.323 helper is doing "its best" to handle the following
call-forwarding scenario:

1) A calls B.
2) B replies to A that the alternate address is C.
3) A calls C.

Now assume that:

1) all traffic between A and B goes through the firewall.

and

2) all traffic between A and C don't go through the firewall.

If you want a picture, see section 5.2 of this site:

http://people.netfilter.org/zhaojingmin/h323_conntrack_nat_helper/#_Toc133598073

That code below is trying to detect if A and C don't go through the
firewall, just to skip the creation of one useless expectation (since
they can communicate without going through the firewall).

With the code below (function callforward_do_filter):

a) if A and C are on-link, then:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck, we create the expectation even if we don't need it.

b) if A and C are behind the same next hop:
rt_nexthop(rt1, fl1.daddr) == rt_nexthop(rt2, fl2.daddr)
We don't create the expectation.

c) if A is on-link and C is behind next hop:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck again, we create the expectation again.

This seems documented. We could make it better if we would have a way
to guess that A and C do not need to communicate through the firewall.

I'll take your patch, it leaves things just like it was before (which
was not really good).

Please, let me know if I'm missing anything. Thanks.

	When created the patch I forgot that this file has
history also in net/ipv4/netfilter/ but anyways. It is
possible to add more checks, for example, checking for
same subnet with inet_addr_onlink when rt_gateway=0:

	if (rt1->dst.dev == rt2->dst.dev &&
	    ((rt1->rt_uses_gateway &&
	      rt1->rt_gateway == rt2->rt_gateway) ||
	     inet_addr_onlink(__in_dev_get_rcu(rt1->dst.dev),
			      src->ip, dst->ip)))

	Note that rt_gateway can be non-0 even for
directly connected hosts when rt is not cached. That
is why the rt_uses_gateway check is used instead of
rt_gateway!=0.

	But if creating expectation is considered harmless
then better to use just the rt_nexthop check because
checking for subnets is too risky, hosts can use different
subnet masks. By this way we reduce the risk to connect
internal hosts without expectation.

quoted

 net/netfilter/nf_conntrack_h323_main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 1b30b0d..962795e 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c

@@ -753,7 +753,8 @@ static int callforward_do_filter(const union nf_inet_addr *src,
 				   flowi4_to_flowi(&fl1), false)) {
 			if (!afinfo->route(&init_net, (struct dst_entry **)&rt2,
 					   flowi4_to_flowi(&fl2), false)) {
-				if (rt1->rt_gateway == rt2->rt_gateway &&
+				if (rt_nexthop(rt1, fl1.daddr) ==
+				    rt_nexthop(rt2, fl2.daddr) &&
 				    rt1->dst.dev  == rt2->dst.dev)
 					ret = 1;
 				dst_release(&rt2->dst);

-- 
1.7.3.4

--

Regards

--
Julian Anastasov [off-list ref]

On Wed, Oct 17, 2012 at 12:03:05PM +0300, Julian Anastasov wrote:
[...]

On Wed, 17 Oct 2012, Pablo Neira Ayuso wrote:

[...]

quoted

On Wed, Oct 10, 2012 at 02:00:47AM +0300, Julian Anastasov wrote:

quoted

	After the change "Adjust semantics of rt->rt_gateway"
(commit f8126f1d51) we should properly match the nexthop when
destinations are directly connected because rt_gateway can be 0.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	This patch needs a closer look from the Netfilter team.

	It restores the check as it was committed originally,
i.e. to compare nexthops. I'm not sure what is the desired logic,
it can depend on the following:

- two directly connected hosts (rt_gateway=0) can be from different
subnets or not

- one party A is the gateway (rt_gateway=0), another party uses
this gateway (rt_gateway=A)

	May be someone that knows this code better can comment
if the check should be different.

Your patch gets it working like before David's change in the
rt_gateway semantics.

I think the H.323 helper is doing "its best" to handle the following
call-forwarding scenario:

1) A calls B.
2) B replies to A that the alternate address is C.
3) A calls C.

Now assume that:

1) all traffic between A and B goes through the firewall.

and

2) all traffic between A and C don't go through the firewall.

If you want a picture, see section 5.2 of this site:

http://people.netfilter.org/zhaojingmin/h323_conntrack_nat_helper/#_Toc133598073

That code below is trying to detect if A and C don't go through the
firewall, just to skip the creation of one useless expectation (since
they can communicate without going through the firewall).

With the code below (function callforward_do_filter):

a) if A and C are on-link, then:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck, we create the expectation even if we don't need it.

b) if A and C are behind the same next hop:
rt_nexthop(rt1, fl1.daddr) == rt_nexthop(rt2, fl2.daddr)
We don't create the expectation.

c) if A is on-link and C is behind next hop:
rt_nexthop(rt1, fl1.daddr) != rt_nexthop(rt2, fl2.daddr)

Bad luck again, we create the expectation again.

This seems documented. We could make it better if we would have a way
to guess that A and C do not need to communicate through the firewall.

I'll take your patch, it leaves things just like it was before (which
was not really good).

Please, let me know if I'm missing anything. Thanks.

	When created the patch I forgot that this file has
history also in net/ipv4/netfilter/ but anyways. It is
possible to add more checks, for example, checking for
same subnet with inet_addr_onlink when rt_gateway=0:

	if (rt1->dst.dev == rt2->dst.dev &&
	    ((rt1->rt_uses_gateway &&
	      rt1->rt_gateway == rt2->rt_gateway) ||
	     inet_addr_onlink(__in_dev_get_rcu(rt1->dst.dev),
			      src->ip, dst->ip)))

	Note that rt_gateway can be non-0 even for
directly connected hosts when rt is not cached. That
is why the rt_uses_gateway check is used instead of
rt_gateway!=0.

	But if creating expectation is considered harmless
then better to use just the rt_nexthop check because
checking for subnets is too risky, hosts can use different
subnet masks. By this way we reduce the risk to connect
internal hosts without expectation.

It creates an expection for the IP parsed as result of the call
forwarding that will expire after 240 seconds. So it consumes memory
but it will expire after that time.

I prefer not to modify this code unless someone using the H.323 helper
reports a problem.

Thanks for your comments. I'll take your patch and extend the
description a bit to explain the current situation of that chunk of
code for the history.

quoted

quoted

 net/netfilter/nf_conntrack_h323_main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 1b30b0d..962795e 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c

@@ -753,7 +753,8 @@ static int callforward_do_filter(const union nf_inet_addr *src,
 				   flowi4_to_flowi(&fl1), false)) {
 			if (!afinfo->route(&init_net, (struct dst_entry **)&rt2,
 					   flowi4_to_flowi(&fl2), false)) {
-				if (rt1->rt_gateway == rt2->rt_gateway &&
+				if (rt_nexthop(rt1, fl1.daddr) ==
+				    rt_nexthop(rt2, fl2.daddr) &&
 				    rt1->dst.dev  == rt2->dst.dev)
 					ret = 1;
 				dst_release(&rt2->dst);

-- 
1.7.3.4

--

Regards

--
Julian Anastasov [off-list ref]

	Hello,

On Wed, 17 Oct 2012, Pablo Neira Ayuso wrote:

quoted

	But if creating expectation is considered harmless
then better to use just the rt_nexthop check because
checking for subnets is too risky, hosts can use different
subnet masks. By this way we reduce the risk to connect
internal hosts without expectation.

It creates an expection for the IP parsed as result of the call
forwarding that will expire after 240 seconds. So it consumes memory
but it will expire after that time.

I prefer not to modify this code unless someone using the H.323 helper
reports a problem.

	Agreed

Thanks for your comments. I'll take your patch and extend the
description a bit to explain the current situation of that chunk of
code for the history.

	Yes, it needs better description. Thanks!

quoted

quoted

quoted

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 1b30b0d..962795e 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c

@@ -753,7 +753,8 @@ static int callforward_do_filter(const union nf_inet_addr *src,
 				   flowi4_to_flowi(&fl1), false)) {
 			if (!afinfo->route(&init_net, (struct dst_entry **)&rt2,
 					   flowi4_to_flowi(&fl2), false)) {
-				if (rt1->rt_gateway == rt2->rt_gateway &&
+				if (rt_nexthop(rt1, fl1.daddr) ==
+				    rt_nexthop(rt2, fl2.daddr) &&
 				    rt1->dst.dev  == rt2->dst.dev)
 					ret = 1;
 				dst_release(&rt2->dst);

-- 
1.7.3.4

Regards

--
Julian Anastasov [off-list ref]