On Tue, Dec 11, 2018 at 6:41 AM Alin Năstac <alin.nastac@xxxxxxxxx> wrote:
>
> 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.
Correction: It looks for the expectation created by the INVITE (where
all the necessary info is stored) and perform the SNAT accordingly.
As I said earlier, the LAN RTP packets are hitting the expectation
created by the peer's reply to the INVITE.
>
> >
> > > + 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
> > >
