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@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@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

_________________________________________________________________________
Professional FreeSWITCH Services
sales@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@lists.freeswitch.org
http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
https://freeswitch.com



--