[Freeswitch-dev] possible bug in CSeq generation logic

Tue Aug 7 19:23:32 UTC 2018

I finally got a chance to look at his packet captures, and it turns out the
other device is the one actually generating invalid CSeq values. FreeSWITCH
is appropriately rejecting the packets.

However, I'm concerned that FreeSWITCH *could* generate invalid CSeq values
based on what I found and the %d formats are definitely wrong.

Royce Mitchell, IT Consultant
ITAS Solutions
royce3 at itas-solutions.com

On Tue, Aug 7, 2018 at 10:04 AM, Anthony Minessale <
anthony.minessale at gmail.com> wrote:

> 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
>
> _________________________________________________________________________
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freeswitch.org/pipermail/freeswitch-dev/attachments/20180807/01788215/attachment-0001.html>