Summary:	ASTERISK-11481: [patch] RFC 3372 SIP-T receive implementation
Reporter:	Gaspar Zoltan (gasparz)	Labels:
Date Opened:	2008-02-20 07:47:50.000-0600	Date Closed:	2011-07-26 14:20:03
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) __20090526-issue-12036-trunk.patch ( 1) bug_12036.diff ( 2) isup.diff ( 3) isup.patch ( 4) trunk_ast_isup.diff
Description:	The patch enables you to receive sip-t packets and decodes some fields. The sip-t means that in the initial invite the content is multipart with 1) the sdp part 2) application/isup part this second part contains the IAM message from the SS7. This is cool because it contains some fields that are not available in the basic sip packet like: Origination line: 0- Plain Old Telephone Service (POTS) - non-coin service requiring no special treatment 70- Code 70 identifies a line connected to a pay station (including both coin and coinless stations) which does not use network provided coin control signaling.  II 70 is used to identify this type pay station line irrespective of whether the pay station is provided by a LEC or a non-LEC.  II 70 is transmitted from the originating end office on all calls made from these lines. For some carriers this is the method to send the payphone information. If you use toll free numbers this is a cool feature to have, so that you can bill the payphone too. Let me know how it worked for you. (For me it's working well). Cheers
Comments:	By: Joshua C. Colp (jcolp) 2008-02-29 12:11:36.000-0600 Could you please provide this as a patch against trunk using diff -u? Thanks. By: Joshua C. Colp (jcolp) 2008-03-12 10:26:59 gasparz: Still need this in unified format... what you have attached won't patch correctly. By: Gaspar Zoltan (gasparz) 2008-03-13 05:12:17 file: sorry about the previous mistake, but as everybody I'm always busy and I didn't check. isup.patch is in unified format (I hope) :) By: Jason Parker (jparker) 2008-04-11 14:06:47 The formatting on this patch makes it very difficult to read.  Could you please review the CODING-GUIDELINES document, and upload a patch with proper formatting? By: snuffy (snuffy) 2008-04-15 00:20:27 I've updated this patch to trunk. Changed a few things but dont have anything I can test patch with. It compiles but otherwise untested By: Leif Madsen (lmadsen) 2008-11-20 16:39:01.000-0600 Is there interest in moving this issue forward? By: Gaspar Zoltan (gasparz) 2008-11-24 08:09:41.000-0600 Well, we are using the patch in production and is working for us. I thought to post it somewhere, maybe somebody else needs it (giving something back to the comunity) By: Leif Madsen (lmadsen) 2008-11-24 08:29:21.000-0600 Thanks for the feedback gasparz. I've assigned this issue to mattf in the hopes it'll get resolved when he looks at the issue I've just related this to. By: Dmitry Andrianov (dimas) 2009-04-13 19:06:59 I have nothing to say about the feature itself (I'm not using it) but would like to put couple of comment about coding style: 1. + char content_type_isup[35] = "Content-Type: application/isup\0"; AFAIK there is no need to specify the size and it is more prone to errors. 2. there are lots of loops where condition looks like xxx < length(yyy). Nothing serious, just see no need making O(n) algorithm O^2(n)... 3. the  while(req->data->str[k+i+9] != '\r') loop will just produce crash with improperly (intentionally or not) formatted message. Also, nobody checks that multipart_message_boundary buffer is large enough. The straight way to remote exploitable DoS... I personally would just do strchr followed by the NULL check and strncpy. 4. Formatting issues: * no spaces after 'if' keyword, no spaces before 'while' in do/while... * there are binary operators (+) not surrounded by spaces. * other issues probably caused by mix of tabs and spaces. 5. The code is just full of constructs like:   isup.hop_counter[k] = 48 + (int)((unsigned char)(req->data->str[isup_offset+1])) / 10; If you need to cast to unsigned char that many times, why not to declare unsigned char *str = (unsigned char *) req->data->str; and use it? which will give   isup.hop_counter[k] = 48 + str[isup_offset+1] / 10; And also 48 may be better expressed as '0' - there are enough other magic constants in the code :) By: Dmitry Andrianov (dimas) 2009-04-13 19:20:01 Hmm... there are even more ways to crash the thing because some lengths are taken right from the isup (?) data which is (I guess) binary and no checks that these lengths are correct - that is there is enough data in the original packet and there is enough space in the destination buffer. For example this loop: for(i = isup_offset + 1; i < isup_offset + (int)((unsigned char)(req->data->str[isup_offset-1])); i++) { isup.called_party_no[k] = 48 + (int)((unsigned char)(req->data->str[i])) % 16; isup.called_party_no[k+1] = 48 + (int)((unsigned char)(req->data->str[i])) / 16; k += 2; } may read more than 200 bytes from the input writing more than 400 bytes into isup.called_party_no while size for this field is only 25. The same happens later with calling_party_no. I would say the code is written in very unsafe way... By: Leif Madsen (lmadsen) 2009-05-26 13:30:44 I just updated the patch to trunk. I have tested that it compiles, but did not do any testing. By: Leif Madsen (lmadsen) 2011-07-26 14:19:58.347-0500 Per the Asterisk maintenance timeline page at http://www.asterisk.org/asterisk-versions maintenance (bug) support for the 1.4 and 1.6.x branches has ended. For continued maintenance support please move to the 1.8 branch which is a long term support (LTS) branch. For more information about branch support, please see https://wiki.asterisk.org/wiki/display/AST/Asterisk+Versions If this is still an issue, please open a new issue so it can be re-triaged appropriately. Thanks!