| Summary: | ASTERISK-16435: [patch] dynamic_exclude_static option results in ACL errors | ||||
| Reporter: | Mark Michelson (mmichelson) | Labels: | |||
| Date Opened: | 2010-07-26 09:44:24 | Date Closed: | 2010-07-27 10:16:45 | ||
| Priority: | Minor | Regression? | No | ||
| Status: | Closed/Complete | Components: | Channels/chan_sip/General | ||
| Versions: | Frequency of Occurrence | ||||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) 17717.patch ( 1) gdb-no-patch.txt ( 2) gdb-patch.txt | |||
| Description: | As reported on the Asterisk-dev mailing list by Dennis DeDonatis, the dynamic_exclude_static option in sip.conf will cause error messages to be printed. Here is my response to the original e-mail on -dev regarding the subject: "The error messages are a result of having the "dynamic_exclude_static" option enabled in sip.conf. The idea behind the option is to create a "contactdeny" ACL for the current peer's IP address so that if another endpoint tries to send a REGISTER to Asterisk with that same IP address in its contact header, Asterisk will deny the registration attempt. I took a look at the code in 1.6.2, and I'm convinced this option is not working as intended, at least not in most cases. The problem is that on an initial load of chan_sip, the current peer's IP address has not been set (it's all 0s) when the host is parsed. The result is that a contactdeny of 0.0.0.0/32 is likely getting created most of the time. On reloads, the problem may be worse because if you are changing the host line for a peer, then the code may end up creating an ACL based on the peer's old IP address instead of its new one. In 1.8.0-beta1, The section is different since we are using the new netsock2 API that allows for IPv6 addresses to be used. We attempt to get a string representation of the peer's IP address, which like in 1.6.2 is all 0s at the time the host line is parsed. The result is the literal string "(null)." We then try to pass this off to the ACL code, which of course says that it is a badly-formatted IP address. The result is the trio of messages starting with the getaddrinfo error, leading to the "Invalid IP address" warning, and leading then to the "Bad ACL entry" error. So it seems like the increased number of error messages in 1.8.0-beta1 has actually exposed a bug that exists in earlier Asterisks as well. The proper fix in both 1.6.2 and 1.8 is to wait and add the contactdeny ACL after the peer's IP address is known to be set to a valid value. This may be as easy as moving a block of code lower down in the build_peer() function than it currently is." | ||||
| Comments: | By: Mark Michelson (mmichelson) 2010-07-26 10:34:02 Added 17717.patch to the issue. This patch moves the contactdeny ACL creation to later in build_peer() when the address for the peer has been set. It passes my compile test, but I would like to hear feedback to know if there are any problems. By: Dennis DeDonatis (dennisd) 2010-07-26 21:17:53 The gdb-no-patch.txt is the backtrace before I applied the patch. The gdb-patch.txt is the backtrace after I applied the patch. :) By: Dennis DeDonatis (dennisd) 2010-07-26 21:24:05 Ugh. It restarted too quickly. The "no-patch" backtrace really is after the patch. I don't have a backtrace before the patch. If you need one, let me know. I was looking in the logs and didn't see those error messages. I set up safe asterisk to do the backtrace for me and it didn't catch the core file to rename it, so it didn't make the backtrace. Let me know if this helps or if I can get you anything else. By: Paul Belanger (pabelanger) 2010-07-26 21:42:05 @DennisD: Check out the latest 1.8 branch and retry your test. The crash was fixed by ASTERISK-16397 By: Dennis DeDonatis (dennisd) 2010-07-26 22:05:28 Ok, running SVN-branch-1.8-r279755M with the patch works great! :) By: Digium Subversion (svnbot) 2010-07-27 10:13:23 Repository: asterisk Revision: 279784 U branches/1.6.2/channels/chan_sip.c ------------------------------------------------------------------------ r279784 | mmichelson | 2010-07-27 10:13:22 -0500 (Tue, 27 Jul 2010) | 14 lines Fix bad behavior of dynamic_exclude_static option in sip.conf. We were attempting to create a contactdeny rule based on the peer's IP address before the peer's IP address had been set. By moving the processing further down in the function, we can ensure stuff works as we expect for it to. (closes issue ASTERISK-16435) Reported by: mmichelson Patches: 17717.patch uploaded by mmichelson (license 60) Tested by: DennisD ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=279784 By: Digium Subversion (svnbot) 2010-07-27 10:15:21 Repository: asterisk Revision: 279785 _U branches/1.8/ U branches/1.8/channels/chan_sip.c ------------------------------------------------------------------------ r279785 | mmichelson | 2010-07-27 10:15:21 -0500 (Tue, 27 Jul 2010) | 20 lines Merged revisions 279784 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r279784 | mmichelson | 2010-07-27 10:13:24 -0500 (Tue, 27 Jul 2010) | 14 lines Fix bad behavior of dynamic_exclude_static option in sip.conf. We were attempting to create a contactdeny rule based on the peer's IP address before the peer's IP address had been set. By moving the processing further down in the function, we can ensure stuff works as we expect for it to. (closes issue ASTERISK-16435) Reported by: mmichelson Patches: 17717.patch uploaded by mmichelson (license 60) Tested by: DennisD ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=279785 By: Digium Subversion (svnbot) 2010-07-27 10:16:44 Repository: asterisk Revision: 279786 _U trunk/ U trunk/channels/chan_sip.c ------------------------------------------------------------------------ r279786 | mmichelson | 2010-07-27 10:16:44 -0500 (Tue, 27 Jul 2010) | 27 lines Merged revisions 279785 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.8 ................ r279785 | mmichelson | 2010-07-27 10:15:22 -0500 (Tue, 27 Jul 2010) | 20 lines Merged revisions 279784 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r279784 | mmichelson | 2010-07-27 10:13:24 -0500 (Tue, 27 Jul 2010) | 14 lines Fix bad behavior of dynamic_exclude_static option in sip.conf. We were attempting to create a contactdeny rule based on the peer's IP address before the peer's IP address had been set. By moving the processing further down in the function, we can ensure stuff works as we expect for it to. (closes issue ASTERISK-16435) Reported by: mmichelson Patches: 17717.patch uploaded by mmichelson (license 60) Tested by: DennisD ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=279786 By: Dan Lukes (dan_lukes) 2011-08-22 19:14:10.926-0500 With such patch and unreachable DNS server, the Asterisk may abend. For more information see ASTERISK-18321 | ||||