[Freeswitch-dev] possible bug in CSeq generation logic

Tue Aug 7 19:59:14 UTC 2018

There is already a mechanism to roll over the value well below the int32
max so its safe logically but maybe not type wise.
%u is definitely better. We'll get that in there.  Its a non-issue since
the logic in check_presence_epoch() should stay well below.

On Tue, Aug 7, 2018 at 2:23 PM, Royce Mitchell III <royce3 at gmail.com> wrote:

> 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
>>
>
>
> _________________________________________________________________________
> 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/ba017eba/attachment-0001.html>