[Home]

Summary:ASTERISK-13148: [patch] Wrong usage of sscanf with use of uninitialized variable caused accidental parsing of RTP/SAVP
Reporter:Folke Ashberg (folke)Labels:
Date Opened:2008-12-01 07:36:42.000-0600Date Closed:2008-12-02 11:42:05.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/CodecHandling
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-sipbg-sscanf-1.4.22.diff
( 1) asterisk-sipbg-sscanf-1.6.0.1.diff
( 2) asterisk-sipbg-sscanf-trunk-r159896.diff
Description:Hi folks!

We are Nortel VoIP-PBX Supporter and have a customer with as SIP-Trunk between Nortel CS1000 and Asterisk. Worked fine. After an upgrade on Nortel PBX to a new MediaGatewayCard (MC32s) no calls could be established. The Nortel offers after the upgrade next to RTP also SRTP.

But asterisk accidentaly parses also the m=audio line with RTP/SAVP in the incoming SDP-Message causing to refused the call ('488 Not acceptable').

It was because a sscanf is used in chan_sip.c / process_sdp() to compare and examine the 'm=audio' in a wrong way. As soon as the line starts with 'audio ' followed by a number, sscanf returns '1' regardless of the rest of the line. The result of '%n' variable of sscanf was then used unitialized.

With my fix now chan_sip ignores now the 'm=audio ... RTP/SAVP' lines and everything is fine.

see details

patch available



****** ADDITIONAL INFORMATION ******

Details:

As defined in RFC4566 the Nortel PBX sends two m=audio ... lines:

m=audio 5452 RTP/AVP 8 0 18 101 111
[..]
m=audio 5452 RTP/SAVP 8 0 18 101 111
[..]

asterisk (1.4.21) outputs
[Nov 27 13:30:41] WARNING[18824]: chan_sip.c:5150 process_sdp: Error in codec string 'io 5434 RTP/SAVP 8 0 18 101 111'

I found that in asterisk/chan_sip.c in process_sdp the function sscanf is not used correctly.

Original:

# if ((sscanf(m, "audio %d/%d RTP/AVP %n", &x, &numberofports, &len) == 2) ||
#    (sscanf(m, "audio %d RTP/AVP %n", &x, &len) == 1)) {

The 2nd 'm=audio' line matches up to 'audio 5452 RTP/', but not further. The result of sscanf is 1, so the condidion is true. But 'len' isn't initialized!

Then len is used:
# for (codecs = m + len; ....


sscanf(3):
%n: [..] The C standard says: "Execution of a %n directive does not increment the assignment count returned at the completion of execution" but the Corrigendum seems to contradict this. Probably it is wise not to make any assumptions on the effect of %n conversions on the return value.

I initialize now 'len' to -1 before sscanf and then i check if it has been set to something.

# len=-1;
# if (((sscanf(m, "audio %d/%d RTP/AVP %n", &x, &numberofports, &len) == 2) &&  len>=0) ||
# ((sscanf(m, "audio %d RTP/AVP %n", &x, &len) == 1) && len>=0)) {


Bug is in 1.4.22, 1.6.0.1 and SVN/trunk

The same bug affects also the similar 'm=video' section and also the 'm=image' and 'm=text' in 1.6.

I'm not sure if my workaround is proof of concept on ther C-compilers or OSses, but im sure the way you handle the m=xxx lines is wrong. And it's not specific to my SecureRTP-Problem, also every other crap after 'm=xxx %d'would be thought to be correct and parsion tries anywhere in memory.

so long

Folke Ashberg
ETK networks solution GmbH
Comments:By: Digium Subversion (svnbot) 2008-12-02 11:42:05.000-0600

Repository: asterisk
Revision: 160297

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r160297 | tilghman | 2008-12-02 11:42:04 -0600 (Tue, 02 Dec 2008) | 10 lines

When the text does not match exactly (e.g. RTP/SAVP), then the %n conversion
fails, and the resulting integer is garbage.  Thus, we must initialize the
integer and check it afterwards for success.
(closes issue ASTERISK-13148)
Reported by: folke
Patches:
      asterisk-sipbg-sscanf-1.4.22.diff uploaded by folke (license 626)
      asterisk-sipbg-sscanf-1.6.0.1.diff uploaded by folke (license 626)
      asterisk-sipbg-sscanf-trunk-r159896.diff uploaded by folke (license 626)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=160297