Summary:	ASTERISK-02192: [patch] Avoid duplicate IP addresses/registrations in sip_friends database
Reporter:	Roy Sigurd Karlsbakk (rkarlsba)	Labels:
Date Opened:	2004-08-05 05:52:36	Date Closed:	2011-06-07 14:10:05
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) mysqlsipfriends.patch ( 1) mysqlsipfriends.patch ( 2) newstuff.patch ( 3) newstuff2.patch ( 4) sanittychange.sql ( 5) sip.patch
Description:	With sip_friends, users with dynamic IPs can potentially generate duplicate ip-addresses in database, generating fuzz. This is an ugly fix that just removes any occurence of the user's ip address at register time. It also changes \" to ' in all SQL statements, according to SQL93. ****** ADDITIONAL INFORMATION ****** GPL and given away to Mark Patch is agaist current CVSHEAD
Comments:	By: Roy Sigurd Karlsbakk (rkarlsba) 2004-08-05 06:14:09 mysqlsipfriends.patch is a patch that fixes contrib/scripts/sip-friends.sql, removing NOT NULL constraints on columns that may be NULL. sanitychange.sql changes this on a running database. By: zoa (zoa) 2004-08-05 11:57:17 I think this could cause an "error 407, unauthorized" for some people, in case they have for some reason 2 iphones registering using the same username and password - people who don't need incoming calls. (I'm thinking of people using a demo account that is given out to multiple persons for example.) I just want to be sure this doesnt break some people's config undocumented. What do the others think of this ? By: Tilghman Lesher (tilghman) 2004-08-05 17:15:33 If you have multiple SIP accounts on a single phone, this will prevent the phone from using more than one SIP account (i.e. you could have SIP/sales, SIP/support, SIP/myname all registered from the same phone). If the SIP client doesn't support unregister, that's the problem, and this is a hack to get around that limitation. By: Tilghman Lesher (tilghman) 2004-08-05 17:17:34 It will also break multiple SIP clients coming from behind a NAT. By: zoa (zoa) 2004-08-06 04:21:31 any chance of making this an option ? By: Roy Sigurd Karlsbakk (rkarlsba) 2004-08-06 07:16:34 + Changed the database UPDATE to only remove duplicate pairs of user/IP - added a config option to enable/disable this functionality - added a config option to allow blocking users, while still keeping them in the database (someone didn't pay etc). All feedback is appreciated :) roy By: Mark Spencer (markster) 2004-08-06 10:13:30 I'm still not understanding really what the problem is that we're trying to solve here.  Is it the case that currently a single sip peer can have multiple lines in a database by registering from more than one location? By: Brian West (bkw918) 2004-08-06 19:52:05 personally sip_friends is a nightmare. Needs some love. By: Roy Sigurd Karlsbakk (rkarlsba) 2004-08-07 03:15:09 Mark: Yes Either we have to allow for multiple registration of the same userid, and that currently doesn't work, or we'll need to do it this way, by removing other registrations from the user at the point of registration. I prefer this way. The latest patch is a one-liner that fixes something i did while sleeping :P By: Mark Spencer (markster) 2004-08-07 09:24:28 According to the "sip-friends.sql" script, "name" is a primary key meaning there cannot be more than one line with the same "name" field. What am I missing here? By: zoa (zoa) 2004-08-09 01:00:08 hmm... I didnt check any C code, but if name is a primary key, it will indeed not accept a second one with the same name. But it will not update the ip on the original one either. P.S. how about making the primary key an md5 of name ? That should increase the lookup speed for the dbase for extremely large setups. By: Brian West (bkw918) 2004-08-09 01:07:14 md5 look up will be no faster.  The ideal solution is to stack the users/peers onto the internal struct as they are looked up. This also fixes the MWI.  They are dumped on reload... your SQL server will thank you! bkw By: Olle Johansson (oej) 2004-08-14 16:43:26 Hmmm. I think that we need a solution to prevent multiple registrations for all situations - both MYSQL_PEERS and [peer]s. I guess this have to wait to after 1.0. By: Brian West (bkw918) 2004-08-17 18:40:33 Why prevent?  We dont now.... By: Rob Gagnon (rgagnon) 2004-08-18 11:09:17 Name as the primary key is a good idea.  No need to change that. As to why someone would allow multiple simultaneous registrations for the same user:  If the same name registers more than once at the same time, how would asterisk know which user's phone to ring when a call is destined for them?  I would think we might need to check the RFC before allowing this to happen--there could be something in there. Also, Corydon beat me to the point I was going to make.  We have multiple ATA's behind a single NAT.  Each uses a different port number, but the same IP.  This would break that. By: Olle Johansson (oej) 2004-08-18 11:28:25 SIP is constructed to allow for multiple endpoints to register with the same URI. Having said that, I can also stand behind the decision not to support it in Asterisk at this point. Asterisk as a PBX is very device-centric and needs to know how many devices are out there, and who's answering what. Denying the second registration is a problem. If someone wants to change phone, the correct way to do that would be to UNREGISTER the current phone (expire=0) and then register the new one. With that theoretical scenario, we could deny the second registration from another device (Contact: is different). Now, the reality isn't like that. Phones crash, Asterisk crash. Very few phones actually UNREGISTER (Xlite do). So overriding the current registration when a new one comes in is propably the easiest way to avoid support. However, we could issue a warning somehow. Send a SIP message to the peer, log something in the log file, spit out something on the console, but still accept. We could add denial functionality, but is has to be configurable and the default will have to stay as it is today to make sure that we do not break installations out there. The default in sip.conf.sample, used for new installations, is another discussion. And, we need the *same* behaviour regardless on how we configure the peer. About this particular patch: I propose that we put it to the archives by closing this bug, leaving it accessible for those that want it. We do not have any consensus around this patch at this time as I see it. By: Olle Johansson (oej) 2004-08-20 16:56:06 Closing bug. Will stay in archive for future retrieval. Roy, if you have any questions about this, find us on IRC.