The topoh module uses its `mask_ip` parameter to tag and identify fields that have been encrypted with its header value. As of Kamailio 4.4.6 code has been added to make sure that `mask_ip` is properly used in the request URI, or `Via`, or any other appropriate SIP header variable before it attempts to perform decryption. Here is a code block that was added at line 738 in `modules/topoh/th_msg.c`
``` /* Do nothing if ruri is not encoded */ if ((REQ_LINE(msg).uri.len<th_uri_prefix.len) || (strncasecmp(REQ_LINE(msg).uri.s,th_uri_prefix.s,th_uri_prefix.len)!=0)) { LM_DBG("ruri [%.*s] is not encoded",REQ_LINE(msg).uri.len,REQ_LINE(msg).uri.s); return 0; } ```
This effectively skips any Request URIs that don't properly match the expected `th_uri_prefix` which is created using the `mask_ip` value assigned to `topoh`.
<!-- Explain what you did, what you expected to happen, and what actually happened. -->
So if I set topoh's `mask_ip` and assume I have set a `mask_key`:
``` modparam("topoh", "mask_ip", "192.168.99.184") ```
Then an `ACK` message like this gets detected and decoded as expected: ``` ACK sip:192.168.99.184;line=sr-1IFG6oxISo4wSekmUolOBKVwbolIboxd6JdwS7xiUekISKPm10NH18Rz1uBZtTpG SIP/2.0 ```
However, if one of my incoming carriers decides to be extra special and append port `:5060` to the request URI like this:
``` ACK sip:192.168.99.184:5060;line=sr-1IFG6oxISo4wSekmUolOBKVwbolIboxd6JdwS7xiUekISKPm10NH18Rz1uBZtTpG SIP/2.0 ```
It gets skipped by topoh because it no longer detects this variation of the URI and I get a message like this:
``` Aug 29 21:36:10 ip-172-31-4-69 /usr/sbin/kamailio[4629]: DEBUG: topoh [th_msg.c:742]: th_unmask_ruri(): ruri [sip:192.168.99.184:5060;line=sr-1IFG 6oxISo4wSekmUolOBKVwbolIboxd6JdwS7xiUekISKPm10NH18Rz1uBZtTpG] is not encoded ```
### Troubleshooting
I thought I might be smart and tried to change the `mask_ip` to `192.168.99.184:5060` but this is additionally blocked by the code that validates `Via` headers in `/modules/topoh/th_msg.c` line 393:
``` /* Skip if via is not encoded */ if (via->host.len!=th_ip.len || strncasecmp(via->host.s, th_ip.s, th_ip.len)!=0) { LM_DBG("via %d is not encoded",i); continue; } ```
It only compares the `host` part of the VIA with the `mask_ip` parameter which is `192.168.99.184:5060` (including the port) and therefore doesn't match and is skipped in decoding.
#### Reproduction
1. Set `topoh` module `mask_ip` to any acceptable IP address 2. Attempt to handle any traffic from a carrier that adds `:5060` automatically to the end of its request URIs 3. The call will go through and then drop ~90 seconds due to incessant attempts of the Kamailio server attempting to reach the bogus `mask_ip` address
#### Debugging Data
I believe the above information is fairly plain. I have included the pertinent debug logs, but it is fairly well describing why `topoh` is not decoding certain lines that it should be.
#### Log Messages
Example of a line that should be decoded which is not being decoded because the incoming carrier has added `:5060` to the `mask_ip` used in the Request URI. ``` Aug 29 21:36:10 ip-172-31-4-69 /usr/sbin/kamailio[4629]: DEBUG: topoh [th_msg.c:742]: th_unmask_ruri(): ruri [sip:192.168.99.184:5060;line=sr-1IFG 6oxISo4wSekmUolOBKVwbolIboxd6JdwS7xiUekISKPm10NH18Rz1uBZtTpG] is not encoded ```
#### SIP Traffic

Example of a call that repeatedly attempts to access bogus `mask_ip` in its route because it is not detected by topoh for decoding after it is passed through a carrier which adds `:5060` to its Request URIs.
### Possible Solutions
No workarounds :(
I also don't have the option of asking carriers to change their standing policy of appending the port ":5060" but the validation efforts in the `topoh` module could be expanded to accept an undesignated port number in the URI or `topoh` could include a `mask_port` parameter so that it builds its URI to expect one in the request URI and in `Via` header fields.
When I have more time to work on the validation C code I will include some suggestions, but others may have a more informed philosophical approach.
### Additional Information
To see all changes to `th_msg.c`: ``` git diff 4.4.5 4.4.6 -- modules/topoh/th_msg.c ```
``` version: kamailio 4.4.6 (x86_64/linux) becbde flags: STATS: Off, USE_TCP, USE_TLS, USE_SCTP, TLS_HOOKS, DISABLE_NAGLE, USE_MCAST, DNS_IP_HACK, SHM_MEM, SHM_MMAP, PKG_MALLOC, Q_MALLOC, F_MALLOC, TLSF_MALLOC, DBG_SR_MEMORY, USE_FUTEX, FAST_LOCK-ADAPTIVE_WAIT, USE_DNS_CACHE, USE_DNS_FAILOVER, USE_NAPTR, USE_DST_BLACKLIST, HAVE_RESOLV_RES ADAPTIVE_WAIT_LOOPS=1024, MAX_RECV_BUFFER_SIZE 262144, MAX_LISTEN 16, MAX_URI_SIZE 1024, BUF_SIZE 65535, DEFAULT PKG_SIZE 8MB poll method support: poll, epoll_lt, epoll_et, sigio_rt, select. id: becbde compiled on 10:23:24 Jun 16 2017 with gcc 4.4.7 ```
* **Operating System**:
``` CentOS release 6.8 (Final) Linux ip-172-31-4-69.us-west-2.compute.internal 2.6.32-642.3.1.el6.x86_64 #1 SMP Tue Jul 12 18:30:56 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux ```
Can you try with latest git branch 4.4? I backported some patches to make this off by default and have an option to enable by param.
Hi @miconda I just read through some of the history on this issue and see you have been adding commits around this issue from before June 25th. The other issue I saw #1165 appears to be the same problem. Couldn't we get around this issue by simply adding ":5060" to the after the ip when the th_uri_prefix is generated? If it is added with the port into the SIP message then I feel like it would be safe to assume the carriers will not try to modify the URI and screw up the prefix thus allowing you to keep your URI prefix safety checks by default. Any thoughts?
I believe the changes you made to 4.4 should fix my issue. I created a small temporary patch myself and I will be testing shortly by just appending ":5060" to the uri prefix to see if I can validate my thoughts.
This patch on 4.4.6 tag worked ok for me by simply forcing a port number on the Via and Request URI prefixes:
``` diff --git modules/topoh/topoh_mod.c modules/topoh/topoh_mod.c index 266cc46..7322cd6 100644 --- modules/topoh/topoh_mod.c +++ modules/topoh/topoh_mod.c @@ -160,7 +160,7 @@ static int mod_init(void) }
/* 'SIP/2.0/UDP ' + ip + ';' + param + '=' + prefix (+ '\0') */ - th_via_prefix.len = 12 + th_ip.len + 1 + th_vparam_name.len + 1 + th_via_prefix.len = 12 + th_ip.len + 6 + th_vparam_name.len + 1 + th_vparam_prefix.len; th_via_prefix.s = (char*)pkg_malloc(th_via_prefix.len+1); if(th_via_prefix.s==NULL) @@ -169,7 +169,7 @@ static int mod_init(void) goto error; } /* 'sip:' + ip + ';' + param + '=' + prefix (+ '\0') */ - th_uri_prefix.len = 4 + th_ip.len + 1 + th_uparam_name.len + 1 + th_uri_prefix.len = 4 + th_ip.len + 6 + th_uparam_name.len + 1 + th_uparam_prefix.len; th_uri_prefix.s = (char*)pkg_malloc(th_uri_prefix.len+1); if(th_uri_prefix.s==NULL) @@ -180,21 +180,21 @@ static int mod_init(void) /* build via prefix */ memcpy(th_via_prefix.s, "SIP/2.0/UDP ", 12); memcpy(th_via_prefix.s+12, th_ip.s, th_ip.len); - th_via_prefix.s[12+th_ip.len] = ';'; - memcpy(th_via_prefix.s+12+th_ip.len+1, th_vparam_name.s, + memcpy(th_via_prefix.s+12+th_ip.len, ":5060;", 6); + memcpy(th_via_prefix.s+12+th_ip.len+6, th_vparam_name.s, th_vparam_name.len); - th_via_prefix.s[12+th_ip.len+1+th_vparam_name.len] = '='; - memcpy(th_via_prefix.s+12+th_ip.len+1+th_vparam_name.len+1, + th_via_prefix.s[12+th_ip.len+6+th_vparam_name.len] = '='; + memcpy(th_via_prefix.s+12+th_ip.len+6+th_vparam_name.len+1, th_vparam_prefix.s, th_vparam_prefix.len); th_via_prefix.s[th_via_prefix.len] = '\0'; LM_DBG("VIA prefix: [%s]\n", th_via_prefix.s); /* build uri prefix */ memcpy(th_uri_prefix.s, "sip:", 4); memcpy(th_uri_prefix.s+4, th_ip.s, th_ip.len); - th_uri_prefix.s[4+th_ip.len] = ';'; - memcpy(th_uri_prefix.s+4+th_ip.len+1, th_uparam_name.s, th_uparam_name.len); - th_uri_prefix.s[4+th_ip.len+1+th_uparam_name.len] = '='; - memcpy(th_uri_prefix.s+4+th_ip.len+1+th_uparam_name.len+1, + memcpy(th_uri_prefix.s+4+th_ip.len, ":5060;", 6); + memcpy(th_uri_prefix.s+4+th_ip.len+6, th_uparam_name.s, th_uparam_name.len); + th_uri_prefix.s[4+th_ip.len+6+th_uparam_name.len] = '='; + memcpy(th_uri_prefix.s+4+th_ip.len+6+th_uparam_name.len+1, th_uparam_prefix.s, th_uparam_prefix.len); th_uri_prefix.s[th_uri_prefix.len] = '\0'; LM_DBG("URI prefix: [%s]\n", th_uri_prefix.s); ```
The port 5060 is the default one for udp/tcp/sctp, then there is 5061 for tls. I have seen implementations that adds it when missing and some that removes it when present. So it is not 100% problem free to add the port. Having the option to skip the prefix check is better and more flexible.
I will close this here, if you want to discuss more, perhaps sr-users mailing list is the best place, being a topic on SIP usage and patterns, than something like an issue in the code.
Closed #1222.