[RFC] SIP conntrack handler and TCP fragmentation

[RFC] SIP conntrack handler and TCP fragmentation

[Ulrich Weber]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

Hi all,

we had a customer with a Cisco Phone using SIP over TCP
which subscribed to 15 Phone numbers.

Since 15 subscribe calls dont fit in one packet, these
requests were fragmented in multiple TCP packets.

One of these fragments gets rejected then by nf_conntrack_sip with:
nf_ct_sip: dropping packet: cannot parse cseq

The Cisco phone will send retransmits, which got dropped too,
times out after some time, reconnects and same games
starts again...

I know the proper solution would be TCP defragmentation
in the nf_conntrack_sip kernel module. However I'm not
sure if this is worth the effort.
What about just accepting unparsable TCP SIP packets?

Cheers
 Ulrich

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-nf_conntrack_sip-relax-SIP-validation-for-TCP.patch --]
[-- Type: text/x-patch; name="0001-nf_conntrack_sip-relax-SIP-validation-for-TCP.patch", Size: 1099 bytes --]

From 084a77a72bfc1c5c655f51618bfe885a216ce88b Mon Sep 17 00:00:00 2001
From: Ulrich Weber <ulrich.weber@riverbed.com>
Date: Wed, 12 Oct 2016 09:27:23 +0200
Subject: [PATCH] nf_conntrack_sip: relax SIP validation for TCP

Due TCP fragmentation we cant assume full SIP messages all the time.
Therefore Accept all packets, even unparsable.

Signed-off-by: Ulrich Weber <ulrich.weber@gmail.com>
---
 net/netfilter/nf_conntrack_sip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 621b81c..7700556 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1562,7 +1562,10 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff,
 			hooks->seq_adjust(skb, protoff, tdiff);
 	}
 
-	return ret;
+	/* Due TCP fragmentation we cant assume full SIP messages all the time.
+	 * Therefore Accept all packets, even unparsable.
+	 */
+	return NF_ACCEPT;
 }
 
 static int sip_help_udp(struct sk_buff *skb, unsigned int protoff,
-- 
2.7.4

Re: [RFC] SIP conntrack handler and TCP fragmentation

[Florian Westphal]

Ulrich Weber <ulrich.weber@gmail.com> wrote:
> we had a customer with a Cisco Phone using SIP over TCP
> which subscribed to 15 Phone numbers.
> 
> Since 15 subscribe calls dont fit in one packet, these
> requests were fragmented in multiple TCP packets.
> 
> One of these fragments gets rejected then by nf_conntrack_sip with:
> nf_ct_sip: dropping packet: cannot parse cseq
> 
> The Cisco phone will send retransmits, which got dropped too,
> times out after some time, reconnects and same games
> starts again...
> 
> I know the proper solution would be TCP defragmentation
> in the nf_conntrack_sip kernel module. However I'm not
> sure if this is worth the effort.

I think an even better solution would be a SIP proxy that can
inject expectations to keep datapath in kernel and only deals with
the signalling traffic.

> What about just accepting unparsable TCP SIP packets?

I wonder why this patch did not fix your problem:

3a7b21eaf4fb3c971bdb47a98f570550ddfe4471
Author: Patrick McHardy <kaber@trash.net>
netfilter: nf_ct_sip: don't drop packets with offsets pointing outside the packet

It specifically deals with this problem (l7 size larger than packet
size).

Re: [RFC] SIP conntrack handler and TCP fragmentation

[Ulrich Weber]

Hi Florian,

On 12.10.2016 09:52, Florian Westphal wrote:
> Ulrich Weber <ulrich.weber@gmail.com> wrote:
>> I know the proper solution would be TCP defragmentation
>> in the nf_conntrack_sip kernel module. However I'm not
>> sure if this is worth the effort.
> 
> I think an even better solution would be a SIP proxy that can
> inject expectations to keep datapath in kernel and only deals with
> the signalling traffic.
Yes your right, is there already one capable of doing this?
But then again, I dont want to put a proxy on an embedded box ;)

> 
>> What about just accepting unparsable TCP SIP packets?
> 
> I wonder why this patch did not fix your problem:
> 
> 3a7b21eaf4fb3c971bdb47a98f570550ddfe4471
> Author: Patrick McHardy <kaber@trash.net>
> netfilter: nf_ct_sip: don't drop packets with offsets pointing outside the packet
> 
> It specifically deals with this problem (l7 size larger than packet
> size).
>From reading the code this fixed the problem when Content-Length
points to one of the next TCP fragments.

In our case Content-Length is always 0 with a couple of SUBSCRIBE calls.
E.g. a TCP packet starting with this will break the SIP connection tracking:

INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE
Content-Length: 0

The previous TCP packet was accepted by SIP connection tracking
because it had no Content-Length field.

Cheers
 Ulrich

Re: [RFC] SIP conntrack handler and TCP fragmentation

[Florian Westphal]

Ulrich Weber <uweber.linux@gmail.com> wrote:
> From reading the code this fixed the problem when Content-Length
> points to one of the next TCP fragments.

Right.

> In our case Content-Length is always 0 with a couple of SUBSCRIBE calls.
> E.g. a TCP packet starting with this will break the SIP connection tracking:
> 
> INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE
> Content-Length: 0

Ugh.

I guess it makes sense to detect this and then accept for this case too.

> The previous TCP packet was accepted by SIP connection tracking
> because it had no Content-Length field.

Perhaps we should treat Content-length of 0 like "no Conent-Length
field".

Re: [RFC] SIP conntrack handler and TCP fragmentation

[Ulrich Weber]

On 12.10.2016 13:41, Florian Westphal wrote:
> Ulrich Weber <uweber.linux@gmail.com> wrote:
>> From reading the code this fixed the problem when Content-Length
>> points to one of the next TCP fragments.
> 
> Right.
> 
>> In our case Content-Length is always 0 with a couple of SUBSCRIBE calls.
>> E.g. a TCP packet starting with this will break the SIP connection tracking:
>>
>> INVITE,NOTIFY,OPTIONS,REFER,REGISTER,UPDATE,SUBSCRIBE
>> Content-Length: 0
> 
> Ugh.
> 
> I guess it makes sense to detect this and then accept for this case too.
> 
>> The previous TCP packet was accepted by SIP connection tracking
>> because it had no Content-Length field.
> 
> Perhaps we should treat Content-length of 0 like "no Conent-Length
> field".
> 
Ahh, I found the root of the problem ;)

Since the payload doesnt start with SIP/2.0, its interpreted as
a response. Starting with INVITE its interpreted as an INVITE
request. Since there is no CSeq header found, its dropped then as invalid.

Possible proper solutions would be either
a) add trailing space to sip_handlers commands
b) check for trailing SIP/2.0 in request line

Do you think a or b would break existing setups?