| 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 | expand |
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/
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?
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!
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
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.
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
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(+)