[PATCH nf 1/1] netfilter: h323,sip: Fix possible dead loop in nat_rtp_rtcp and nf_nat_sdp_media

From: Gao Feng <redacted>

When h323 and sip try to insert expect nodes, they would increase
the port by 2 for loop, and the loop condition is that "port != 0".
So when the start port is odd number, port never increases to zero.

Now make port as u32 instead of u_int16_t, and the loop condition is
"port <= USHRT_MAX".

Signed-off-by: Gao Feng <redacted>
---
 net/ipv4/netfilter/nf_nat_h323.c | 4 ++--
 net/netfilter/nf_nat_sip.c       | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7eb..010fc3e 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c

@@ -183,7 +183,7 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
 	struct nf_ct_h323_master *info = nfct_help_data(ct);
 	int dir = CTINFO2DIR(ctinfo);
 	int i;
-	u_int16_t nated_port;
+	u32 nated_port;
 
 	/* Set expectations for NAT */
 	rtp_exp->saved_proto.udp.port = rtp_exp->tuple.dst.u.udp.port;

@@ -218,7 +218,7 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
 
 	/* Try to get a pair of ports. */
 	for (nated_port = ntohs(rtp_exp->tuple.dst.u.udp.port);
-	     nated_port != 0; nated_port += 2) {
+	     nated_port <= USHRT_MAX; nated_port += 2) {
 		int ret;
 
 		rtp_exp->tuple.dst.u.udp.port = htons(nated_port);

diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 791fac4..0b24eb3 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c

@@ -548,7 +548,7 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
-	u_int16_t port;
+	u32 port;
 
 	/* Connection will come from reply */
 	if (nf_inet_addr_cmp(&ct->tuplehash[dir].tuple.src.u3,

@@ -571,7 +571,7 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 
 	/* Try to get same pair of ports: if not, try to change them. */
 	for (port = ntohs(rtp_exp->tuple.dst.u.udp.port);
-	     port != 0; port += 2) {
+	     port <= USHRT_MAX; port += 2) {
 		int ret;
 
 		rtp_exp->tuple.dst.u.udp.port = htons(port);

-- 
1.9.1

Hi,

2017-03-02 15:57 GMT+08:00  [off-list ref]:

From: Gao Feng <redacted>

When h323 and sip try to insert expect nodes, they would increase
the port by 2 for loop, and the loop condition is that "port != 0".
So when the start port is odd number, port never increases to zero.

This seems will never happen, since the RTP port has been ensured to
be even.

For example, at expect_rtp_rtcp():
   ...
   /* RTP port is even */
   rtp_port = port & ~htons(1);
   rtcp_port = port | htons(1);

And at set_expected_rtp_rtcp():
   ...
   base_port = ntohs(tuple.dst.u.udp.port) & ~1;
   rtp_port = htons(base_port);

Hi Liping,

On Thu, Mar 2, 2017 at 4:50 PM, Liping Zhang [off-list ref] wrote:

Hi,

2017-03-02 15:57 GMT+08:00  [off-list ref]:

quoted

From: Gao Feng <redacted>

When h323 and sip try to insert expect nodes, they would increase
the port by 2 for loop, and the loop condition is that "port != 0".
So when the start port is odd number, port never increases to zero.

This seems will never happen, since the RTP port has been ensured to
be even.

For example, at expect_rtp_rtcp():
   ...
   /* RTP port is even */
   rtp_port = port & ~htons(1);
   rtcp_port = port | htons(1);

And at set_expected_rtp_rtcp():
   ...
   base_port = ntohs(tuple.dst.u.udp.port) & ~1;
   rtp_port = htons(base_port);

Thanks your point, I actually did not attention these codes you mentioned.
But I checked the other codes, I think the dangerous may exist.

The expect class is NF_CT_EXPECT_CLASS_DEFAULT, and proto is
IPPROTO_UDP at the function "expect_rtp_rtcp",
And it makes sure the port is even number.

But look at the process_gcf, the port is got from the packet data at
function get_h225_addr.
So it may be odd number.
It also would add one expect node whose class is
NF_CT_EXPECT_CLASS_DEFAULT, and proto is IPPROTO_UDP.

BTW, the loop in nat_rtp_rtcp and nat_rtp_rtcp depend on one potential
condition which is protected by other functions.
It doesn't seem a good style.

Regards
Feng

Hi,
2017-03-02 18:18 GMT+08:00 Gao Feng [off-list ref]:
[...]

The expect class is NF_CT_EXPECT_CLASS_DEFAULT, and proto is
IPPROTO_UDP at the function "expect_rtp_rtcp",
And it makes sure the port is even number.

But look at the process_gcf, the port is got from the packet data at
function get_h225_addr.
So it may be odd number.
It also would add one expect node whose class is
NF_CT_EXPECT_CLASS_DEFAULT, and proto is IPPROTO_UDP.

The nat_rtp_rtcp() is only invoked by expect_rtp_rtcp, and nf_nat_sdp_media()
is only invoked by set_expected_rtp_rtcp. So the RTP port is ensured to be
even, as well as the rtp_exp->tuple.dst.u.udp.port.

Note: the rtp_exp is alloced by nf_ct_expect_alloc, and initialized by
nf_ct_expect_init, then passed to nat_rtp_rtcp or nf_nat_sdp_media.

So I cannot figure out why process_gcf will affect this? Or I missed something?

Hi Liping,

On Thu, Mar 2, 2017 at 7:18 PM, Liping Zhang [off-list ref] wrote:

Hi,
2017-03-02 18:18 GMT+08:00 Gao Feng [off-list ref]:
[...]

quoted

The expect class is NF_CT_EXPECT_CLASS_DEFAULT, and proto is
IPPROTO_UDP at the function "expect_rtp_rtcp",
And it makes sure the port is even number.

But look at the process_gcf, the port is got from the packet data at
function get_h225_addr.
So it may be odd number.
It also would add one expect node whose class is
NF_CT_EXPECT_CLASS_DEFAULT, and proto is IPPROTO_UDP.

The nat_rtp_rtcp() is only invoked by expect_rtp_rtcp, and nf_nat_sdp_media()
is only invoked by set_expected_rtp_rtcp. So the RTP port is ensured to be
even, as well as the rtp_exp->tuple.dst.u.udp.port.

Note: the rtp_exp is alloced by nf_ct_expect_alloc, and initialized by
nf_ct_expect_init, then passed to nat_rtp_rtcp or nf_nat_sdp_media.

So I cannot figure out why process_gcf will affect this? Or I missed something?

I was lost in codes and forgot to check the caller of nat_rtp_rtcp.
Thanks your explanations.

There is no any issue in current codes indeed.

Best Regards
Feng