[v4] netfilter: nf_conntrack_sip: add sip_external_media logic

Message ID 20181224071519.24568-1-alin.nastac@gmail.com
State Accepted
Delegated to: Pablo Neira
Headers show
Series
  • [v4] netfilter: nf_conntrack_sip: add sip_external_media logic
Related show

Commit Message

Alin Năstac Dec. 24, 2018, 7:15 a.m.
When enabled, the sip_external_media logic will leave SDP
payload untouched when it detects that interface towards INVITEd
party is the same with the one towards media endpoint.

The typical scenario for this logic is when a LAN SIP agent has more
than one IP address (uses a different address for media streams than
the one used on signalling stream) and it also forwards calls to a
voice mailbox located on the WAN side. In such case sip_direct_media
must be disabled (so normal calls could be handled by the SIP
helper), but media streams that are not traversing this router must
also be excluded from address translation (e.g. call forwards).

Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 net/netfilter/nf_conntrack_sip.c | 42 ++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Pablo Neira Ayuso Feb. 15, 2019, 12:02 p.m. | #1
Hi Alin,

On Mon, Dec 24, 2018 at 08:15:19AM +0100, Alin Nastac wrote:
> When enabled, the sip_external_media logic will leave SDP
> payload untouched when it detects that interface towards INVITEd
> party is the same with the one towards media endpoint.
> 
> The typical scenario for this logic is when a LAN SIP agent has more
> than one IP address (uses a different address for media streams than
> the one used on signalling stream) and it also forwards calls to a
> voice mailbox located on the WAN side. In such case sip_direct_media
> must be disabled (so normal calls could be handled by the SIP
> helper), but media streams that are not traversing this router must
> also be excluded from address translation (e.g. call forwards).

This patch got stuck in my queue right before holidays. I'm very sorry
about that.

Still one more question: Now that we have explicit helper assignment
via rule, and assuming automatic helper assignment is deprecated
(actually, disabled by default these days since it is unsecure [1]).

Would it be possible to skip this via explicit ruleset policy?

Thanks.

[1] https://home.regit.org/netfilter-en/secure-use-of-helpers/
Alin Năstac Feb. 15, 2019, 1:20 p.m. | #2
Hi Pablo,

On Fri, Feb 15, 2019 at 1:02 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Alin,
>
> On Mon, Dec 24, 2018 at 08:15:19AM +0100, Alin Nastac wrote:
> > When enabled, the sip_external_media logic will leave SDP
> > payload untouched when it detects that interface towards INVITEd
> > party is the same with the one towards media endpoint.
> >
> > The typical scenario for this logic is when a LAN SIP agent has more
> > than one IP address (uses a different address for media streams than
> > the one used on signalling stream) and it also forwards calls to a
> > voice mailbox located on the WAN side. In such case sip_direct_media
> > must be disabled (so normal calls could be handled by the SIP
> > helper), but media streams that are not traversing this router must
> > also be excluded from address translation (e.g. call forwards).
>
> This patch got stuck in my queue right before holidays. I'm very sorry
> about that.
>
> Still one more question: Now that we have explicit helper assignment
> via rule, and assuming automatic helper assignment is deprecated
> (actually, disabled by default these days since it is unsecure [1]).
>
> Would it be possible to skip this via explicit ruleset policy?

Parameters such as sip_direct_signalling and sip_external_media
(latter being implemented in this patch) are global switches. I guess
we can implement them as sip helper parameters configurable through
the rule that enables the helper, but I haven't found yet a helper
that has such parameters ("-j CT --helper xxx" rules don't allow
passing any additional helper parameters). Probably their values will
have to be stored in nf_ct_sip_master struct associated with the
master conntrack.

For instance, ftp helper use such global switch called loose. How
would you propose to pass the value of this parameter in a helper
assignment rule?
Pablo Neira Ayuso Feb. 15, 2019, 3:07 p.m. | #3
Hi Alin,

On Fri, Feb 15, 2019 at 02:20:14PM +0100, Alin Năstac wrote:
> On Fri, Feb 15, 2019 at 1:02 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > On Mon, Dec 24, 2018 at 08:15:19AM +0100, Alin Nastac wrote:
> > > When enabled, the sip_external_media logic will leave SDP
> > > payload untouched when it detects that interface towards INVITEd
> > > party is the same with the one towards media endpoint.
> > >
> > > The typical scenario for this logic is when a LAN SIP agent has more
> > > than one IP address (uses a different address for media streams than
> > > the one used on signalling stream) and it also forwards calls to a
> > > voice mailbox located on the WAN side. In such case sip_direct_media
> > > must be disabled (so normal calls could be handled by the SIP
> > > helper), but media streams that are not traversing this router must
> > > also be excluded from address translation (e.g. call forwards).
> >
> > This patch got stuck in my queue right before holidays. I'm very sorry
> > about that.
> >
> > Still one more question: Now that we have explicit helper assignment
> > via rule, and assuming automatic helper assignment is deprecated
> > (actually, disabled by default these days since it is unsecure [1]).
> >
> > Would it be possible to skip this via explicit ruleset policy?
> 
> Parameters such as sip_direct_signalling and sip_external_media
> (latter being implemented in this patch) are global switches.
> I guess we can implement them as sip helper parameters configurable
> through the rule that enables the helper, but I haven't found yet a
> helper that has such parameters ("-j CT --helper xxx" rules don't
> allow passing any additional helper parameters). Probably their
> values will have to be stored in nf_ct_sip_master struct associated
> with the master conntrack.

Hm, I was wondering if we could restrict the rule that comes with "-j
CT --helper xxx" to skip these flows, but we need to inspect them to
classify them as media flow... right? Given you check for the route to
see if the media endpoint is reachable through the same interface, I'm
thinking if it's possible to make it from the policy side. If helper
is the only place where we can do this, that's fine too, I'm just
exploring :-).

Regarding what you mention above...

> For instance, ftp helper use such global switch called loose. How
> would you propose to pass the value of this parameter in a helper
> assignment rule?

In nft, these switches won't need to be global anymore, it's not yet
implemented but we already have a design for this. I'm going to add
this to the GSoC list idea, we're applying this year and it can be a
good task for someone.

Anyway, that would be away from the scope of your patch, so I would
take it as is and we will find anyone to revisit this to implement the
local switch idea.

Thanks!
Alin Năstac Feb. 15, 2019, 8:37 p.m. | #4
Hi Pablo,

On Fri, Feb 15, 2019 at 4:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Alin,
>
> On Fri, Feb 15, 2019 at 02:20:14PM +0100, Alin Năstac wrote:
> > On Fri, Feb 15, 2019 at 1:02 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> [...]
> > > On Mon, Dec 24, 2018 at 08:15:19AM +0100, Alin Nastac wrote:
> > > > When enabled, the sip_external_media logic will leave SDP
> > > > payload untouched when it detects that interface towards INVITEd
> > > > party is the same with the one towards media endpoint.
> > > >
> > > > The typical scenario for this logic is when a LAN SIP agent has more
> > > > than one IP address (uses a different address for media streams than
> > > > the one used on signalling stream) and it also forwards calls to a
> > > > voice mailbox located on the WAN side. In such case sip_direct_media
> > > > must be disabled (so normal calls could be handled by the SIP
> > > > helper), but media streams that are not traversing this router must
> > > > also be excluded from address translation (e.g. call forwards).
> > >
> > > This patch got stuck in my queue right before holidays. I'm very sorry
> > > about that.
> > >
> > > Still one more question: Now that we have explicit helper assignment
> > > via rule, and assuming automatic helper assignment is deprecated
> > > (actually, disabled by default these days since it is unsecure [1]).
> > >
> > > Would it be possible to skip this via explicit ruleset policy?
> >
> > Parameters such as sip_direct_signalling and sip_external_media
> > (latter being implemented in this patch) are global switches.
> > I guess we can implement them as sip helper parameters configurable
> > through the rule that enables the helper, but I haven't found yet a
> > helper that has such parameters ("-j CT --helper xxx" rules don't
> > allow passing any additional helper parameters). Probably their
> > values will have to be stored in nf_ct_sip_master struct associated
> > with the master conntrack.
>
> Hm, I was wondering if we could restrict the rule that comes with "-j
> CT --helper xxx" to skip these flows, but we need to inspect them to
> classify them as media flow... right? Given you check for the route to
> see if the media endpoint is reachable through the same interface, I'm
> thinking if it's possible to make it from the policy side. If helper
> is the only place where we can do this, that's fine too, I'm just
> exploring :-).

Actually almost all RELATED conntracks are media streams except the
one created at registration time with dport set to the WAN side port
of the master conntrack (usually 5060).

However, in this patch I don't try to block media streams that
traverse the box, The goal here is to avoid mangling SDP payload that
have to be forwarded as it was transmitted by the LAN host, to allow
media streams between 2 WAN side endpoints to contact each other
directly. So you see this decision cannot be taken outside the helper
context, because it is not the kind of ACCEPT/DROP dilemma. Signalling
packet needs to be always ACCEPTed, but its SDP payload need to be
translated only when the IP address transmitted by the LAN host as its
media stream address is behind the NAT box.

In other words, sip_external_media=1 is modifying the
sip_direct_media=0 behavior:
  - master conntrack NATed, sip_direct_media=0, sip_external_media=0 :
SDP payload is always mangled by nf_nat_sip.c
  - master conntrack NATed, sip_direct_media=0, sip_external_media=1 :
SDP payload is mangled when new media stream(s) will traverse the box
Pablo Neira Ayuso Feb. 16, 2019, 9:48 a.m. | #5
On Fri, Feb 15, 2019 at 09:37:00PM +0100, Alin Năstac wrote:
> Hi Pablo,
> 
> On Fri, Feb 15, 2019 at 4:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi Alin,
> >
> > On Fri, Feb 15, 2019 at 02:20:14PM +0100, Alin Năstac wrote:
> > > On Fri, Feb 15, 2019 at 1:02 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > [...]
> > > > On Mon, Dec 24, 2018 at 08:15:19AM +0100, Alin Nastac wrote:
> > > > > When enabled, the sip_external_media logic will leave SDP
> > > > > payload untouched when it detects that interface towards INVITEd
> > > > > party is the same with the one towards media endpoint.
> > > > >
> > > > > The typical scenario for this logic is when a LAN SIP agent has more
> > > > > than one IP address (uses a different address for media streams than
> > > > > the one used on signalling stream) and it also forwards calls to a
> > > > > voice mailbox located on the WAN side. In such case sip_direct_media
> > > > > must be disabled (so normal calls could be handled by the SIP
> > > > > helper), but media streams that are not traversing this router must
> > > > > also be excluded from address translation (e.g. call forwards).
> > > >
> > > > This patch got stuck in my queue right before holidays. I'm very sorry
> > > > about that.
> > > >
> > > > Still one more question: Now that we have explicit helper assignment
> > > > via rule, and assuming automatic helper assignment is deprecated
> > > > (actually, disabled by default these days since it is unsecure [1]).
> > > >
> > > > Would it be possible to skip this via explicit ruleset policy?
> > >
> > > Parameters such as sip_direct_signalling and sip_external_media
> > > (latter being implemented in this patch) are global switches.
> > > I guess we can implement them as sip helper parameters configurable
> > > through the rule that enables the helper, but I haven't found yet a
> > > helper that has such parameters ("-j CT --helper xxx" rules don't
> > > allow passing any additional helper parameters). Probably their
> > > values will have to be stored in nf_ct_sip_master struct associated
> > > with the master conntrack.
> >
> > Hm, I was wondering if we could restrict the rule that comes with "-j
> > CT --helper xxx" to skip these flows, but we need to inspect them to
> > classify them as media flow... right? Given you check for the route to
> > see if the media endpoint is reachable through the same interface, I'm
> > thinking if it's possible to make it from the policy side. If helper
> > is the only place where we can do this, that's fine too, I'm just
> > exploring :-).
> 
> Actually almost all RELATED conntracks are media streams except the
> one created at registration time with dport set to the WAN side port
> of the master conntrack (usually 5060).
> 
> However, in this patch I don't try to block media streams that
> traverse the box, The goal here is to avoid mangling SDP payload that
> have to be forwarded as it was transmitted by the LAN host, to allow
> media streams between 2 WAN side endpoints to contact each other
> directly. So you see this decision cannot be taken outside the helper
> context, because it is not the kind of ACCEPT/DROP dilemma. Signalling
> packet needs to be always ACCEPTed, but its SDP payload need to be
> translated only when the IP address transmitted by the LAN host as its
> media stream address is behind the NAT box.
>
> In other words, sip_external_media=1 is modifying the
> sip_direct_media=0 behavior:
>   - master conntrack NATed, sip_direct_media=0, sip_external_media=0 :
> SDP payload is always mangled by nf_nat_sip.c
>   - master conntrack NATed, sip_direct_media=0, sip_external_media=1 :
> SDP payload is mangled when new media stream(s) will traverse the box

Applied, thanks for explaining.

Patch

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c8d2b6688a2a..f067c6b50857 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -21,6 +21,8 @@ 
 #include <linux/tcp.h>
 #include <linux/netfilter.h>
 
+#include <net/route.h>
+#include <net/ip6_route.h>
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_conntrack_expect.h>
@@ -54,6 +56,11 @@  module_param(sip_direct_media, int, 0600);
 MODULE_PARM_DESC(sip_direct_media, "Expect Media streams between signalling "
 				   "endpoints only (default 1)");
 
+static int sip_external_media __read_mostly = 0;
+module_param(sip_external_media, int, 0600);
+MODULE_PARM_DESC(sip_external_media, "Expect Media streams between external "
+				     "endpoints (default 0)");
+
 const struct nf_nat_sip_hooks *nf_nat_sip_hooks;
 EXPORT_SYMBOL_GPL(nf_nat_sip_hooks);
 
@@ -861,6 +868,41 @@  static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
 		if (!nf_inet_addr_cmp(daddr, &ct->tuplehash[dir].tuple.src.u3))
 			return NF_ACCEPT;
 		saddr = &ct->tuplehash[!dir].tuple.src.u3;
+	} else if (sip_external_media) {
+		struct net_device *dev = skb_dst(skb)->dev;
+		struct net *net = dev_net(dev);
+		struct rtable *rt;
+		struct flowi4 fl4 = {};
+#if IS_ENABLED(CONFIG_IPV6)
+		struct flowi6 fl6 = {};
+#endif
+		struct dst_entry *dst = NULL;
+
+		switch (nf_ct_l3num(ct)) {
+			case NFPROTO_IPV4:
+				fl4.daddr = daddr->ip;
+				rt = ip_route_output_key(net, &fl4);
+				if (!IS_ERR(rt))
+					dst = &rt->dst;
+				break;
+
+#if IS_ENABLED(CONFIG_IPV6)
+			case NFPROTO_IPV6:
+				fl6.daddr = daddr->in6;
+				dst = ip6_route_output(net, NULL, &fl6);
+				if (dst->error) {
+					dst_release(dst);
+					dst = NULL;
+				}
+				break;
+#endif
+		}
+
+		/* Don't predict any conntracks when media endpoint is reachable
+		 * through the same interface as the signalling peer.
+		 */
+		if (dst && dst->dev == dev)
+			return NF_ACCEPT;
 	}
 
 	/* We need to check whether the registration exists before attempting