[Freeswitch-dev] possible bug in CSeq generation logic

[Ken Rice]

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