Hi Pablo.
On Tue, Dec 11, 2018 at 1:14 AM Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>
> Hi Alin,
>
> On Tue, Nov 27, 2018 at 10:09:20AM +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.
>
> Copying and pasting your more detailed description for reference (from
> your previous email here. BTW, no problem about long descriptions,
> specifically for helper patches).
Communication skill is not my strong point... ;-P
>
> Alin Nastac said:
> > 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).
> -- end of quote --
>
> Please, see my comments below.
>
> > Signed-off-by: Alin Nastac <alin.nastac@xxxxxxxxx>
> > ---
> > 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) {
>
> Let me rephrase the scenario with my own words, I'm assuming that:
>
> #1 S1 agent creates expectation via REGISTER: SIP proxy (SP) -> S1.
> using dport 5060 for the expectation.
> #2 S2 agent creates expectation via REGISTER: SP -> S2 with dport 1024
> for the expectation.
> #3 S1 creates expectation via INVITE to SP: SP -> S1 for RTP/RCTP traffic.
> As you said "all goes well if RTP conntrack is initiated by a
> packet originated from the SP", which makes sense since this
> traffic matches the expectation.
> #4 However, if first RTP packet is coming from S1, it does not match
> any expectation, so it goes through the firewall with no mangling -
> as you described. But, in such case, given we do not match an
> expectation, nf_nat_sip_expected() is not called.
>
> So what am I missing to exercise your new code from nf_nat_sip_expected()?
An INVITE creates an expectation for the RTP packets sent by the party
that receives the INVITE, while the reply to the INVITE creates
another expectation for the RTP packets sent by the call initiator.
These expectations are designed to perform the necessary DNAT, so the
expectations created by SIP packets sent from LAN to WAN are not doing
actually any translation (source IP address is translated anyway by
the MASQUERADE/SNAT rule). However, when RTP port of the LAN client
requires translation (source port that is, e.g. from 1024 to 1026),
the current version of nf_nat_sip_expected doesn't deliver it.
There is no problem when the WAN party is the one that sends the first
RTP packet because the LAN party's RTP packets will match the
conntrack created by the WAN party's RTP packets and therefore the
source port of the RTP packets sent from the LAN will be translated
correctly.
However, when LAN client sends the first RTP packet, there is no
conntrack created already so there will be no source port translation.
And this is where my patch intervenes... It looks for the expectation
created by the INVITE's reply (where all the necessary info is stored)
and perform the SNAT accordingly.
>
> > + 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
> >
