netfilter: nf_nat_sip: fix RTP/RTCP source port translations

Perform the same SNAT translation on RTP/RTCP conntracks regardless of
who sends the first datagram.

Prior to this change, RTP packets send by the peer who required source
port translation were forwarded with unmodified source port when this
peer started its voice/video stream first.
---
 net/netfilter/nf_nat_sip.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

Hi Alin,

On Mon, Nov 05, 2018 at 02:54:53PM +0100, Alin Nastac wrote:
> Perform the same SNAT translation on RTP/RTCP conntracks regardless of
> who sends the first datagram.
> 
> Prior to this change, RTP packets send by the peer who required source
> port translation were forwarded with unmodified source port when this
> peer started its voice/video stream first.

Do you have more detailed description, eg. scenario triggering this
problem to understand better what this is fixing.

Not telling that this is not fixing anything, but this fix looks
slightly hairy.

BTW, I need a Signed-off-by: tag here.

Thanks!

> ---
>  net/netfilter/nf_nat_sip.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
> index 1f30860..a1e23cc 100644
> --- a/net/netfilter/nf_nat_sip.c
> +++ b/net/netfilter/nf_nat_sip.c
> @@ -18,6 +18,7 @@
>  
>  #include <net/netfilter/nf_nat.h>
>  #include <net/netfilter/nf_nat_helper.h>
> +#include <net/netfilter/nf_conntrack_core.h>
>  #include <net/netfilter/nf_conntrack_helper.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
>  #include <net/netfilter/nf_conntrack_seqadj.h>
> @@ -316,6 +317,9 @@ static void nf_nat_sip_seq_adjust(struct sk_buff *skb, unsigned int protoff,
>  static void nf_nat_sip_expected(struct nf_conn *ct,
>  				struct nf_conntrack_expect *exp)
>  {
> +	struct nf_conn_help *help = nfct_help(ct->master);
> +	struct nf_conntrack_expect *pair_exp;
> +	int range_set_for_snat = 0;
>  	struct nf_nat_range2 range;
>  
>  	/* This must be a fresh one. */
> @@ -327,15 +331,38 @@ static void nf_nat_sip_expected(struct nf_conn *ct,
>  	range.min_addr = range.max_addr = exp->saved_addr;
>  	nf_nat_setup_info(ct, &range, NF_NAT_MANIP_DST);
>  
> -	/* Change src to where master sends to, but only if the connection
> -	 * actually came from the same source. */
> -	if (nf_inet_addr_cmp(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3,
> +	/* Do SRC manip according with the parameters found in the
> +	 * paired expected conntrack. */
> +	spin_lock_bh(&nf_conntrack_expect_lock);
> +	hlist_for_each_entry(pair_exp, &help->expectations, lnode) {
> +		if (pair_exp->tuple.src.l3num == nf_ct_l3num(ct) &&
> +		    pair_exp->tuple.dst.protonum == ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum &&
> +		    nf_inet_addr_cmp(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3, &pair_exp->saved_addr) &&
> +		    ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.all == pair_exp->saved_proto.all) {
> +			range.flags = (NF_NAT_RANGE_MAP_IPS | NF_NAT_RANGE_PROTO_SPECIFIED);
> +			range.min_proto.all = range.max_proto.all = pair_exp->tuple.dst.u.all;
> +			range.min_addr = range.max_addr = pair_exp->tuple.dst.u3;
> +			range_set_for_snat = 1;
> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&nf_conntrack_expect_lock);
> +
> +	/* When no paired expected conntrack has been found, change src to
> +	 * where master sends to, but only if the connection actually came
> +	 * from the same source. */
> +	if (!range_set_for_snat &&
> +	    nf_inet_addr_cmp(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3,
>  			     &ct->master->tuplehash[exp->dir].tuple.src.u3)) {
>  		range.flags = NF_NAT_RANGE_MAP_IPS;
>  		range.min_addr = range.max_addr
>  			= ct->master->tuplehash[!exp->dir].tuple.dst.u3;
> -		nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
> +		range_set_for_snat = 1;
>  	}
> +
> +	/* Perform SRC manip. */
> +	if (range_set_for_snat)
> +		nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
>  }
>  
>  static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
> -- 
> 2.7.4
>

Hi Pablo,

On Tue, Nov 27, 2018 at 12:57 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Alin,
>
> On Mon, Nov 05, 2018 at 02:54:53PM +0100, Alin Nastac wrote:
> > Perform the same SNAT translation on RTP/RTCP conntracks regardless of
> > who sends the first datagram.
> >
> > Prior to this change, RTP packets send by the peer who required source
> > port translation were forwarded with unmodified source port when this
> > peer started its voice/video stream first.
>
> Do you have more detailed description, eg. scenario triggering this
> problem to understand better what this is fixing.

The scenario fixed by this patch involves a regular SIP call, but one
that requires port
translation for the RTP conntrack. For instance, suppose you have 2
SIP agents in the
LAN, both connected to the same SIP proxy:
  - agent S1  starts first and its REGISTER phase will create a
permanent expected
conntrack on dport 5060 for allowing SIP packets to be forwarded to S1
regardless of
their source IP address or port
  - on agent S2  registration, its permanent expected conntrack will
confict with the S1
signalling expected conntrack, so it will be translated to port 1024

When S1 initiates a call using RTP/RTCP port range 1024-1025, SIP
helper will find
that port 1024 is taken over by S2's signalling expected conntrack, so
it translates it
to port range 1026-1027. All goes well if the RTP conntrack is
initiated by a packet
originated from the SIP proxy, but when the first RTP packet is sent
by S1 (usually
the peer that initiates the call is the one that sends the first RTP
packet), it is sent
towards SIP proxy with unmodified source port (1024 iso 1026).

> Not telling that this is not fixing anything, but this fix looks
> slightly hairy.

I tried to find a less hairy solution, but the information necessary
to fix RTP SNAT is not
stored in the expected conntrack created by the S1 INVITE, it is
available in the paired
expected conntrack created by the SIP proxy reply.

> BTW, I need a Signed-off-by: tag here.

Ah, I keep forgetting this, sorry! I will send it signed in a couple of hours.