Summary:	ASTERISK-03669: [patch] check for user agent before allowing authentication
Reporter:	paradise (paradise)	Labels:
Date Opened:	2005-03-11 04:35:51.000-0600	Date Closed:	2005-04-04 23:41:19
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) patch.txt ( 1) patch-v2.txt ( 2) patch-v3.txt ( 3) patch-v4.txt ( 4) patch-v5.txt
Description:	this patch adds the ability to consider a specific user agent on regsitration to *. so * can force user to use a specific sip device and even firmware version. [200] context=default type=friend host=dynamic username=200 secret=200 alloweduseragent=X-PRO [201] context=default type=friend host=dynamic username=201 alloweduseragent=snom190-3.56u ****** ADDITIONAL INFORMATION ****** Disclaimer on file. Paradise Dove (pardove@gamil.com) i also disclaim patches applied to bugs ASTERISK-3281 and ASTERISK-3500
Comments:	By: Kevin P. Fleming (kpfleming) 2005-03-11 12:13:11.000-0600 Can you implement this using a regex, so it's possible to allow patterns and multiple choices? I don't know if the overhead would be acceptable, though, since the checking has to be done on every INVITE/REGISTER/SUBSCRIBE. By: paradise (paradise) 2005-03-11 12:42:57.000-0600 - good idea about regex. i was working on another idea to have multiple choices: alloweduseragents=X-PRO&eyeBeam&snom190-3.56u and: alloweduseragents=all - yes, it should do checking on every INVITE/REGISTER at least. btw, it's possible to check the method type as well. By: Kevin P. Fleming (kpfleming) 2005-03-11 13:06:24.000-0600 I don't think there's any reason for 'all', since that's the same as not specifying it at all. If you try to use your own "multiple agent" syntax, then you'll have to provide a parser for it, and some way to escape the 'magic' character that separates the values. Using a regex would solve those problems. However, you will need to call the regcomp() function and store the result in the sip_user/sip_peer, rather than the regex itself, since then you just call regexec to do the comparison when the packet comes in. Make sure you call regfree() in sip_destroy_user/sip_destroy_peer so the regex compile buffer gets released. By: paradise (paradise) 2005-03-11 13:33:58.000-0600 - the "all" option is just to make the configuration more legible. - i dont want to make a realtime parser, because i do believe that  such a checking which is done on every SIP call may affect the performance. i just was thinking of do the main parsing at loading the configuration and storing the parsed option to sip_user/sip_peer. btw, if you think that using reg[xxxx] functions on each INVITE/REGISTER/... doesn't influence the performance, i have no insistence. By: Kevin P. Fleming (kpfleming) 2005-03-11 13:40:14.000-0600 I think regexec() is pretty fast, since the regex compiling has already been done. Keep in mind that if you support multiple entries, you will have to walk a list of possible matches on each packet, so that's not fast either. Using a regex will only be 'slow' for people who put in complicated regexes... if they use a simple string with no special characters, it will be barely slower than strcasecmp would be. By: paradise (paradise) 2005-03-11 13:48:52.000-0600 ok, i satisfied and will work on it. thanks By: Kevin P. Fleming (kpfleming) 2005-03-11 14:05:06.000-0600 Glad to hear it, I look forward to seeing your patch. As far as "all" being more 'legible', the Asterisk way to do things is to not specify the option at all if no value is required. That is the ultimate in legibility, since it is the most concise representation possible. By: paradise (paradise) 2005-03-11 17:30:59.000-0600 regex implemented! (patch-v2.txt) it has passed some tests. alloweduseragent=snom190-3.56[u,w]|snom200|X-PRO note that its case-sensitive. edited on: 03-11-05 17:32 By: Kevin P. Fleming (kpfleming) 2005-03-12 00:00:40.000-0600 You are passing the regex_t into check_auth, not a regex_t *. That will cause an extra copy of the regex_t to be made on the stack. Can you find a way to do this without having to use gotos in check_auth? How about just making a small function that takes a 'regex_t *' and a 'char *', and does the check, returning 1 if there is no match, or 0 if there is a match or the regex_t * is not initialized. Then, instead of:  res = 0;  goto checkuseragent; You can just use:  res = match_regex(alloweduseragent, p->useragent); There is a call to check_auth passing NULL for the 'alloweduseragent' parameter; how did that compile? Without changing that parameter to a 'regex_t *' (see above), this should have generated a compiler error... You can add REG_ICASE to make the regex-matching case-insensitive, which is probably better for the end users. Other than all those items, this looks good to me and would be a worthwhile addition :-) By: Kevin P. Fleming (kpfleming) 2005-03-12 00:05:19.000-0600 Actually, forgot a couple more: If the regex fails to compile, please issue a LOG_WARNING message for that. If the user specifies multiple alloweduseragent= lines, the previously compiled regex buffer(s) won't be freed. You will need to check the "used" variable before trying to compile the new pattern, and regfree() if it's non-zero. By: paradise (paradise) 2005-03-12 00:20:46.000-0600 thanks for the comments ;) i thought that on "sip reload" sip_destroy_peer/user is called!!! so i didn't check the ".used"  var on initialization. By: Kevin P. Fleming (kpfleming) 2005-03-12 01:06:41.000-0600 No, sip_peers are kept around across reloads, because we need to keep Call-IDs, sequence numbers and other bits so that we can conform to the SIP RFCs. sip_users are destroyed on reloads, however. You are correct, though, in that you should not be forcing the 'used' field to zero; if the sip_peer was just created, it will be zero because the entire structure was memset'ed to zero. If the structure was _not_ just created, there might be a regex already compiled there that must be freed before you (possibly) compile a new one. If the admin has removed the "alloweduseragent=" line before reloading, then you won't be compiling a new one, so having freed the old one you will be left with no compiled regex, which is exactly what should happen. However, you still need to check 'used' _again_ before compiling the regex, in case someone specifies it multiple times for the same peer/user. By: paradise (paradise) 2005-03-12 10:02:42.000-0600 new changes applied (patch-v3.txt) and works fine. as i just use the return value of regexec, it might be better to not have the match_regex function and use regexec directly. i have no idea about line chan_sip.c:5909 which passes NULL regex_t to check_auth. btw, it compiles with even no warning! By: Kevin P. Fleming (kpfleming) 2005-03-12 10:25:55.000-0600 Yes, passing NULL to check_auth is correct now, because that argument is now a pointer type, not a structure type. The point I was trying to make with match_regex was that it should check the 'used' field first, so that it can return 0 if the pattern buffer is empty. If regexec() will do itself, then yes, you can eliminate match_regex and just call regexec() directly. If you do decide to keep the match_regex function, please mark it 'static', as it has no need to be visible outside of chan_sip. By: paradise (paradise) 2005-03-12 10:59:10.000-0600 regexec does no extra checking so match_regex is needed and must do such a checking. its done in patch-v4.txt By: Kevin P. Fleming (kpfleming) 2005-03-12 11:09:34.000-0600 OK, this is looking pretty good, but you still missed one of my notes above: when alloweduseragent appears multiple times in a SIP peer/user entry, the second line will compile the pattern and overwrite the previous one in the buffer without freeing it first. Just add another 'if (alloweduseragent->used) regfree(...)' in the block that actually handles the alloweduseragent config option, and you should be all set. By: paradise (paradise) 2005-03-12 11:20:57.000-0600 it's already done: @@ -9251,6 +9277,8 @@ ast_copy_flags(peer, &global_flags, SIP_USEREQPHONE); peer->secret[0] = '\0'; peer->md5secret[0] = '\0'; +                if ( peer->alloweduseragent.used ) + regfree(&peer->alloweduseragent); and @@ -9080,6 +9101,8 @@ /* set the usage flag to a sane staring value*/ user->inUse = 0; user->outUse = 0; +                if ( user->alloweduseragent.used ) + regfree(&user->alloweduseragent); By: Kevin P. Fleming (kpfleming) 2005-03-12 11:24:26.000-0600 No, those lines are outside the config-entry processing loop, they will clear out the buffer if it has contents when the config processing starts. What I'm referring to is _inside_ the loop, if the user specifies alloweduseragent multiple times. By: paradise (paradise) 2005-03-12 11:48:28.000-0600 done in patch-v5.txt By: Kevin P. Fleming (kpfleming) 2005-03-12 13:19:35.000-0600 OK, looks good to me now. Thanks for the effort! By: Kevin P. Fleming (kpfleming) 2005-03-12 13:52:03.000-0600 change bug category, since it applies to more than registration now By: Olle Johansson (oej) 2005-03-13 09:15:28.000-0600 Please add comments in the source code that explains what you are doing! Also, I would like to see a global setting. Thank you! By: Kevin P. Fleming (kpfleming) 2005-03-13 09:50:05.000-0600 Global settings can be done using config-category inheritance, do we really need to be adding more true global settings (which complicate the code substantially)? By: paradise (paradise) 2005-03-13 11:12:57.000-0600 global setting is a good idea, but we'll need the "all" option too if global setting implemented. By: Kevin P. Fleming (kpfleming) 2005-03-13 11:19:17.000-0600 Exactly, which adds more complication. Using inheritance, if the user has done this: [defaults] alloweduseragent=snom And they want to allow a peer to use any user agent, they can do this: [peer1](defaults) alloweduseragent= This will override the setting from the defaults category and result in no pattern to match, which is exactly what they want. Adding a global setting makes this quite a bit more difficult to code, and you also get into the mess where the global setting is used when the user has not been identified/authenticated but the user-specific setting is used once they have been. Messy, messy :-( By: khb (khb) 2005-03-13 13:06:30.000-0600 Restricting access by useragent is rather counter-productive to the premise of SIP, flexible, open-standards-based communications.  We added the useragent configuration previously to give people a way to counter-act such measures by service providers. I am not sure that Asterisk should encourage this sort of behavior. Useragent: header was never meant as an access control feature. By: paradise (paradise) 2005-03-13 13:25:35.000-0600 ofcourse it's not a feature of/for SIP protocol, it's a feature for asterisk pbx. i think that it has many uses when an adiministrator wants to restrict his users to use a specific sip/soft phone. an administrator needs tools to do better adiministration! By: khb (khb) 2005-03-13 13:56:24.000-0600 Well, been on both sides of the isle, understand the issues quite well. Doing better administration is fine, which is what the User-Agent and Server fields are all about. Rather than "authenticating" and restricting against product strings, it would be a lot more constructive to build a configurable facility that turns features on the Asterisk side on or off based on *compatibility* with the product string. That's better administration and true openess. In that spirit I would encourage changing the default Asterisk User-Agent string to conform more closely with the intended semantics,  i.e. Asterisk-PBX/1.0.7 edited on: 03-13-05 14:04 By: Olle Johansson (oej) 2005-03-13 17:59:43.000-0600 khb: I agree with you that it's stupid to filter on Useragent and that we should change the useragent string (did that earlier in chan_sip2...). However, since some people still want to do this and they think it's a good idea and it doesn't break functionality, I can accept this patch. By: Olle Johansson (oej) 2005-03-13 21:34:24.000-0600 kpfleming: Without globals, autocreatepeer doesn't work as I expect it to... By: Kevin P. Fleming (kpfleming) 2005-03-14 08:35:18.000-0600 Oh yeah, autocreatepeer... Ick. Wouldn't it be more logical to make the user create a category in their sip.conf that holds the settings to be used when autocreatepeer is invoked? Something as simple as: [autocreatepeer](!) type=peer ... would do the trick quite nicely. It's backwards compatible, in that if you don't specify it then autocreatepeer relies on the global settings, but if you do, you can very strictly control the settings. By: Olle Johansson (oej) 2005-03-14 09:08:55.000-0600 kpfleming: Let's discuss a new architecture outside of this bug and keep the globals for now. A teaser: In chan_sip2, I had [template-autocreatepeer] :-) By: Kevin P. Fleming (kpfleming) 2005-03-14 09:34:36.000-0600 Ick... I hate globals :-) We need to decide on semantics then. The global option could: 1) Apply only to "unknown" and "autocreatepeer" peers. 2) Apply to all peers, unless they have the option specified in their category. From an implementation point of view, #1 will be far easier than #2. Neither of them are easy, though, since the regex compiled buffer cannot just be copied (unless you keep track of which one needs to be freed and when), and if we don't pre-compile the regex then the performance will be abysmal. Maybe the thing to do is to store the uncompiled regex in the user/peer/global variables, and compile it into a buffer in the sip_pvt structure. I don't know how 'early' the sip_pvt is allocated when an incoming request is being processed, but if it's early enough, it would serve as the buffer for checking the initial request, and then be present for checking subsequent requests in the same dialog. By: Mark Spencer (markster) 2005-03-26 18:30:58.000-0600 Is the consensus that this actually should be added? By: Brian West (bkw918) 2005-03-26 18:48:09.000-0600 Whats the point?  Just like callerid the useragent can be spoofed. /b By: Kevin P. Fleming (kpfleming) 2005-03-31 14:52:51.000-0600 I don't have any strong opinion either way; I can see where it may be useful, but as bkw mentioned it's also easily circumvented by reasonably knowledgable end users. By: Kevin P. Fleming (kpfleming) 2005-04-04 23:41:11 paradise: It appears that you are going to need to get some interest generated from the Asterisk community for this patch to have any chance of going into the tree; at this point it doesn't seem to provide much benefit, given the ease with which it can be circumvented. If you can get a reasonable amount of support from the community with some real-world applications for this, you can reopen the bug and we'll look into it again.