[Freeswitch-dev] possible bug in CSeq generation logic

Tue Aug 7 15:04:51 UTC 2018

It seems like getting a negative value is only possible if it rolled over
but its designed to reset well before the 32 bit signed mark.  Is this
somehow triggered by time changes on the box?
There is more nuance than just forcing the bit because many phones have
crazy ideas about cseq having to be incremental.  I think it would be
better to roll the offset back down to 0 if some error is detected so it
will only impact phones once.  Many phones break until you reset the whole
registration if they are unhappy with the cseq.  The reason for all the
complexity in the first place is that the cseq must be continual with time
even when FS is off, because you can reboot FS and keep the existing phone
registration in tact including subscriptions to presence.  But if the CSEQ
starts over many phones freak out and stop working.

On Tue, Aug 7, 2018 at 1:57 AM, Ken Rice <krice at freeswitch.org> wrote:

> open i a jira so it can get tracked. also check jira to see if its already
> been reported/fixed
>
> Sent from my iPhone
>
> On Aug 7, 2018, at 00:28, Royce Mitchell III <royce3 at gmail.com> wrote:
>
> I would be happy to open a ticket for this, but given that I'm a newb when
> it comes to the source, I'd like some confirmation of what I've found
> before I do.
>
> According to RFC3261 8.1.1.5 "The sequence number value MUST be
> expressible as a 32-bit unsigned integer and MUST be less than 2**31"
>
> I also checked RFC2543 just for fun and similar wording is there.
>
> A client just informed me that he is seeing errors in the field where
> FreeSWITCH is generating CSeq headers with a negative sequence number and
> those requests are getting rejected by other endpoints.
>
> =====================
>
>
> In digging through the code, I found MASTER/src/mod/endpoints/mod_sofia/sofia_presence.c
> line 2134 defines a function sofia_presence_get_cseq() which appears to be
> responsible for creating the sequence value that ends up in the CSeq
> headers.
>
> A cursory glance at this logic leads me to believe that it is definitely
> capable of creating a sequence number with bit 31 set, which would be in
> violation of the RFC.
>
> I believe a simple fix with little side-effect would be to add the
> following line right before last_cseq is set ( line 2149 in MASTER ):
>
>   }
>   }
> + callsequence &= 0x7FFFFFFF; // RFC3261 8.1.1.5 prohibits bit 31 to be
> set
>   profile->last_cseq = callsequence;
>
> =====================
>
>
> Another small thing is that FreeSWITCH is generating a negative value in
> certain scenarios when outputting a CSeq header. This means that the
> sequence value is improperly being interpreted as a signed integer. This
> shouldn't matter because the high order bit isn't supposed to be set, but
> regardless the code is still technically wrong.
>
> In MASTER/libs/sofia-sip/libsofia-sip-ua/sip/sip_basic.c I found a
> definition of sip_cseq_e() which correctly interprets the value as unsigned.
>
> There are a few logging examples that are wrong that I found using a
> simple grep, although these line #'s are probably off because it's from an
> old checkout. The "CSeq %d" in each of the following probably needs to be a
> "CSeq %u"
>
> nta\sl_utils_log.c:76       "%s%s "URL_FORMAT_STRING" (CSeq %d %s)\n",
> nta\sl_utils_log.c:100      "%s%03u %s (CSeq %d %s)\n",
> nta\sl_utils_print.c:69     "%s%s "URL_FORMAT_STRING" (CSeq %d %s)\n",
> nta\sl_utils_print.c:86     "%s%03u %s (CSeq %d %s)\n",
>
>
> However in MASTER/src/mod/endpoints/mod_sofia/sofia.c I found the
> following:
>
> line 381:
>
> -switch_snprintf(sip_cseq, sizeof(sip_cseq), "%d", sip->sip_cseq->cs_seq);
> +switch_snprintf(sip_cseq, sizeof(sip_cseq), "%u", sip->sip_cseq->cs_seq);
>
> line 1707:
>
> -switch_snprintf(sip_cseq, sizeof(sip_cseq), "%d", sip->sip_cseq->cs_seq);
> +switch_snprintf(sip_cseq, sizeof(sip_cseq), "%u", sip->sip_cseq->cs_seq);
>
> =====================
>
>
> Something like the following might also be a consideration, but probably
> only in 1.8:
>
> In MASTER/libs/sofia-sip/libsofia-sip-ua/sip/sip_basic.c line 1288 add
> something like the following:
>
> if ( seq & 0x80000000 )
> {
>     SU_DEBUG_1(("%s: CSeq header creation failed - sequence value %u
> violates RFC3261/8.1.1.5", __func__, seq ));
>     return NULL;
> }
>
>
>
> P.S. I'd really like to see at least the first 2 changes proposed above (
> callsequence masking and format string fixes ) back-ported to 1.6.
>
>
>
>
>
> Royce Mitchell, IT Consultant
> ITAS Solutions
> royce3 at itas-solutions.com
>
> _________________________________________________________________________
> Professional FreeSWITCH Services
> sales at freeswitch.com
> https://freeswitch.com
>
> Official FreeSWITCH Sites
> https://freeswitch.com/oss
> https://freeswitch.org/confluence
> https://cluecon.com
>
> FreeSWITCH-dev mailing list
> FreeSWITCH-dev at lists.freeswitch.org
> http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
> UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
> https://freeswitch.com
>
>
> _________________________________________________________________________
> Professional FreeSWITCH Services
> sales at freeswitch.com
> https://freeswitch.com
>
> Official FreeSWITCH Sites
> https://freeswitch.com/oss
> https://freeswitch.org/confluence
> https://cluecon.com
>
> FreeSWITCH-dev mailing list
> FreeSWITCH-dev at lists.freeswitch.org
> http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
> UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
> https://freeswitch.com
>

-- 
Anthony Minessale II
Founder, FreeSWITCH.
http://freeswitch.com

https://youtu.be/l_hOxzCt6X4
https://www.youtube.com/watch?v=oAxXgyx5jUw
https://www.youtube.com/watch?v=9XXgW34t40s
https://www.youtube.com/watch?v=NLaDpGQuZDA
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freeswitch.org/pipermail/freeswitch-dev/attachments/20180807/d96da655/attachment.html>