Hi Pablo,
On Wed, Dec 12, 2018 at 5:13 PM Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>
> Hi Alin,
>
> On Tue, Dec 11, 2018 at 08:27:45AM +0100, Alin Năstac wrote:
> > On Tue, Dec 11, 2018 at 6:41 AM Alin Năstac <alin.nastac@xxxxxxxxx> wrote:
> [...]
> > > > 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 --
> [...]
> > > > > @@ -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.
>
> Thanks a lot for explaining.
>
> One thing we could probably do is to add nf_nat_rtp_rtcp_expected()
> instead of using the generic nf_nat_sip_expected() which is also
> called for the REGISTER case. I think we only need this code below for
> the RTP/RTCP case as you explained.
That's true, but signalling expectations do not have a paired
expectation anyway.
If you want to exclude this logic for them, it is enough to condition
it with exp->helper != nfct_help(ct)->helper.
What variant do you prefer?
>
> > > > > + 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
> > > > >
