Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect translation of tel: uri to sip: uri in tel2sip() #1173

Closed
doncarr opened this issue Jun 29, 2017 · 25 comments
Closed

Incorrect translation of tel: uri to sip: uri in tel2sip() #1173

doncarr opened this issue Jun 29, 2017 · 25 comments
Assignees

Comments

@doncarr
Copy link

doncarr commented Jun 29, 2017

Description

I received a wireshark trace from a team member showing that a tel: uri was translated to a sip: uri in a very strange (invalid) way:

INVITE tel:491234567891;phone-context=ims.mnc001.mcc001.3gppnetwork.org

Was translated to:

INVITE sip:491234567891;phone-context=ims.mnc001.mcc001.3gppnetwork.org@ims.mnc001.mcc001.3gppnetwork.org;user=phone

Troubleshooting

I first thought this was definitely an error, and only the phone number should be copied to the sip: uri. But, on further reading of the specification, (https://www.ietf.org/rfc/rfc3261.txt), I see that it is in fact legal to leave all parameters in. Though, when the phone-context parameter is simply a domain, it would at least seem the cleanest thing would be to remove it when the exact same domain is also inserted after the @ in the sip message.

So, looking to find where the message is translated, I saw in the scscf.cfg configuration file that there is a translation specified.

I found that "tel2sip" is mapped to the C function of the same name tel2sip()

In this function, the translation will do exactly what I found in the trace, in other words, it will leave in all parameters separated by semi-colon, inserted before the @ symbol.

But, reading the spec, I see that the spec is not being followed in any case. The spec states that all parameters included when translating, must be switched to alphabetic order, with the exception that
isdn-subaddress and post-dial, must occur first and in that order. There is no code in tel2sip() to perform that sorting.

Also, the parser removes '(', ')', '.', and '-' in the phone number, as specified, but, these same characters could be scattered anywhere in invalid places, and they are just silently removed, '+' can scattered anywhere and it is just left alone, Other characters can be scattered anywhere and they will just be left as-is, no error. So, I feel there should be some minimal checking of the phone number, trying to follow the spec (if we could just execute the BNF from the spec!).

Reproduction

This can be reproduced by sending a tel: uri with parameters inserted separated by semicolon.

Debugging Data

(none)

Log Messages

(none)

SIP Traffic

See attached file

Possible Solutions

The solution would be to modify tel2sip(), adding the sorting of parameters, putting the two exceptions first if they exist, before inserting back into the string.

We should also throw errors on obvious bad formatting and ilegal characters, etc, in the input tel: uri

Should we remove the "phone-context" if it is a domain and also exactly the same as the domain inserted after the @?

Additional Information

  • Kamailio Version - output of kamailio -v
version: kamailio 5.0.0-dev5 (x86_64/linux) da4ef4-dirty
flags: STATS: Off, USE_TCP, USE_TLS, USE_SCTP, TLS_HOOKS, USE_RAW_SOCKS, 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: da4ef4 -dirty
compiled on 10:37:40 May 12 2017 with gcc 5.4.0

SIP spec:
https://www.ietf.org/rfc/rfc3261.txt

Wireshark output:

wireshark-tel2sip

In RFC 3966, Tel URI format:
 
   The "tel" URI has the following syntax:
 
   telephone-uri        = "tel:" telephone-subscriber
   telephone-subscriber = global-number / local-number
   global-number        = global-number-digits *par
   local-number         = local-number-digits *par context *par
   par                  = parameter / extension / isdn-subaddress
   isdn-subaddress      = ";isub=" 1*uric
   extension            = ";ext=" 1*phonedigit
   context              = ";phone-context=" descriptor
   descriptor           = domainname / global-number-digits
   global-number-digits = "+" *phonedigit DIGIT *phonedigit
   local-number-digits  =
      *phonedigit-hex (HEXDIG / "*" / "#")*phonedigit-hex
   domainname           = *( domainlabel "." ) toplabel [ "." ]
   domainlabel          = alphanum
                          / alphanum *( alphanum / "-" ) alphanum
   toplabel             = ALPHA / ALPHA *( alphanum / "-" ) alphanum
   parameter            = ";" pname ["=" pvalue ]
   pname                = 1*( alphanum / "-" )
   pvalue               = 1*paramchar
   paramchar            = param-unreserved / unreserved / pct-encoded
   unreserved           = alphanum / mark
   mark                 = "-" / "_" / "." / "!" / "~" / "*" /
                          "'" / "(" / ")"
   pct-encoded          = "%" HEXDIG HEXDIG
   param-unreserved     = "[" / "]" / "/" / ":" / "&" / "+" / "$"
   phonedigit           = DIGIT / [ visual-separator ]
   phonedigit-hex       = HEXDIG / "*" / "#" / [ visual-separator ]
   visual-separator     = "-" / "." / "(" / ")"
   alphanum             = ALPHA / DIGIT
   reserved             = ";" / "/" / "?" / ":" / "@" / "&" /
                          "=" / "+" / "$" / ","
   uric                 = reserved / unreserved / pct-encoded
  • Operating System:
# uname -a
Linux epc-enablers 4.4.0-51-generic #72-Ubuntu SMP Thu Nov 24 18:29:54 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
@miconda
Copy link
Member

miconda commented Jun 29, 2017

I expect that tel2uri() was implemented based on the needs of initial developer as documented at:

It can be improved if one needs/wants more.

Note also that specs are not implemented ad-litteram in a single function. The design of the application has been from the beginning to offer small tools (functions/statements) that can be blended in config script to get what you need. There are many functions and transformations that can help you replace/remove unwanted characters from user part of R-URI, such as subst_user(...), {re.subst} or {s...} transformations, etc. With all these I am pretty sure you can transfrorm tel to sip uri as per specs without using tel2sip() function.

To be clear: I am not against enhancing tel2sip() to do more than it does now, but wanted to highlight that many spec requirements can be done with a combination of functions and expressions. I am not the author of tel2sip() and actually I haven't used it many times. I do URI normalization with script operations.

@doncarr
Copy link
Author

doncarr commented Jun 29, 2017

@miconda, thanks very much! I will take a stab at an improvement and submit, starting with sorting the parameters according to the spec. This is good for me to understand the spec and kamailio code, and a small part of, over time, making kamailio better so that it can/will be used in more places.

I do understand that there are trade-offs in enforcing esoteric details of the spec, and, in any case, it would be impossible (and not even desirable) to try to enforce everything.

@doncarr
Copy link
Author

doncarr commented Jul 3, 2017

@miconda, I have written a routine that puts the parameters for the tel: uri into an array with key/value separated, then uses qsort to sort. With this, they can be put into the sip: uri in correct sorted order. The sort is very esoteric, specific to the requirements for sip: uri when converting from a tel: uri, so, I would suggest we add it to the existing conversion routine. Though, it would be very easy to make it a separate callable script called before the existing routine, to sort the tel: uri. Also, if inside the current routine, at the same time that we have the parameters split up, we can easily check if phone-context is present, and, if it is a domain, we can delete the phone-context parameter, and, move the domian to the standard place after the @ symbol.

Please let me know how we can move forward. I can create a modified tel2sip() routine with the sorting of tel: parameters and a check to eliminate duplicate domains. Then, do testing here, and, finally send to you for your perusal before accepting.

Thank you very much!

@kamailio-sync
Copy link

kamailio-sync commented Jul 3, 2017 via email

@doncarr
Copy link
Author

doncarr commented Jul 3, 2017

At the link below, you can go to 19.1.6. It is a very strange ordering giving priority to two parameters that should be first, if present.

https://www.ietf.org/rfc/rfc3261.txt

19.1.6 Relating SIP URIs and tel URLs
. . . .

   To mitigate this problem, elements constructing telephone-subscriber
   fields to place in the userinfo part of a SIP or SIPS URI SHOULD fold
   any case-insensitive portion of telephone-subscriber to lower case,
   and order the telephone-subscriber parameters lexically by parameter
   name, excepting isdn-subaddress and post-dial, which occur first and
   in that order.  (All components of a tel URL except for future-
   extension parameters are defined to be compared case-insensitive.)

@miconda
Copy link
Member

miconda commented Jul 3, 2017

@doncarr - it can be also a new function, like tel_to_sip_uri(), so if one wants to use the old/lightweight version can still do it. I guess the new function is not going to share too much code with the old onw.

@doncarr
Copy link
Author

doncarr commented Jul 3, 2017

Actually, I intended to use most of the existing function, just inserting a sort, and, removal of duplicate domain. But, yes, it could be a new function, leaving the existing one exactly as-is.

It would also be nice for the update to be active without having to edit the configuration file. I will leave it totally up to you on the best way to integrate.

@kamailio-sync
Copy link

kamailio-sync commented Jul 3, 2017 via email

@doncarr
Copy link
Author

doncarr commented Jul 7, 2017

I have a fix just about ready for your review. As noted, there can be some weird things in the telephone world!

There are a couple of not well defined issues. They more or less state that the objective of the sip: uri, is that two equivalent uris will always be the equivalent string, thus the need to sort the parameters and converting to lower case. That much is very clear. But, applying this same reasoning to the "phone-context" parameter, if it is a domain, then, it should be moved after the '@' symbol and not repeated as a "phone-context" parameter, as to have an equivalent string to one without the phone-context parameter.

For example, the following two are obviously equivalent, but, are not the same string:

sip:491234567891;phone-context=ims.tel.org@ims.tel.org;user=phone
sip:491234567891@ims.tel.org;user=phone

Also, they do not specifically state that visual separators should be removed from the "phone-context" parameter when it is a global phone number prefix, but, without removing them, we would again have the problem of equivalent sip uris not being equivalent strings.

For example, the following two are obviously equivalent, but, not the same string:

sip:1234567;phone-context=+52-33@ims.tel.org;user=phone
sip:1234567;phone-context=+5233@ims.tel.org;user=phone

If that all sounds good to you, I will submit a fix after more testing.

I have not messed with checking for clearly bad use of visual separators, I just silently remove all visual separators wherever they appear, as was done with the existing code.

Please let me know if you want any changes to the reasoning. You could also give me example tel: uris you would like me to test before submitting the fix.

Thanks for everything!

Don.

@doncarr
Copy link
Author

doncarr commented Jul 11, 2017

@kamailio-sync, @miconda,

Are you interested in the fix? I agree it is a bit esoteric, but, I think worth it to help reduce problems in the future. Especially removing the double domain name in the sip uri, and removing visual separators from the phone-context when it is a global number. Well, the parameters really should be sorted to meet the spec, but, not sure if any program would ever have a problem if they were not sorted. It is nice if equivalent uris look exactly the same.

Please let me know, Don.

@miconda
Copy link
Member

miconda commented Jul 12, 2017

@doncarr - yes, make a pull request, I suggested to be a new function so the lighweight version stays around and can be used by those that know it fits their needs.

FYI: kamailio-sync is an account associated with sr-dev mailing list just in case someone replies there, instead of using the github webforms for issues. In the above case, it was @juha-h responding on mailing list (as shown in the signature when expanding the dots at the end of comments).

@kamailio-sync
Copy link

kamailio-sync commented Jul 12, 2017 via email

@doncarr
Copy link
Author

doncarr commented Jul 12, 2017

@juha-h, @miconda ,

On making a "normalizing" function. I could do that to remove the visual separators from the "phone-context" field when it is a phone number, and, also, pre-sort the parameters in the tel: uri, so that when the current (light weight) function pulls them, the parameters are already in the sorted order, and there are no visual separators in the "phone-context" field.

A little more tricky, I could also delete the "phone-context" field from the tel: uri when it is a domain, assuming that the host name was already pulled out. But, if this was done before domain was extracted, it would cause an error (basically losing the domain). Currently, the domain in the "hostpart" variable is inserted after the "@" sign, and the contents in the "phone-context" field is inserted as is.

Also, in this case, there can be no logic in deciding which domain to select (the one in the "hostpart", or the one in the "phone-context" It seems that the hostpart is currently pulled using cscf_get_realm_from_uri(), so, theoretically, the variable "hostpart" and the contents of "phone-context" should always be the same.

Ok, in any case, I think it would be a little confusing to have a pre-filter that creates a special format of a tel: uri such that an existing translator will end up creating a standards compliant sip: uri.

I think it would be best to give a simple/fast version and a standards compliant version.

The existing function does try to do part of the standards in that it converts everything to lower case, and, removes visual separators from the phone number digits before the first semi-colon.

@miconda
Copy link
Member

miconda commented Jul 13, 2017

I would also go straight for a function that give the final result. Having a two step approach is not useful and adds to complexity of the config. The existing function should be used by those that fit their need when they know what they expect. The new one, by those that want to ensure the result is standards compliant, even for unknown sources.

@doncarr
Copy link
Author

doncarr commented Jul 13, 2017

@miconda, thanks, that is exactly what I have now, so, will submit it when I can. Below is the code that I wrote with a new function named "std_tel2sip()", as a preview I used most of the old procedure where possible, and tried to keep the same style.

I have been given a task with a short deadline, so, will not be able to work on this anymore right now. I will get back to it as soon as possible.

Don.

/*
 * Compare function to sort tel: uri options acording to standard
 * before inserting into sip: uri
 *
 * See "RFC 3261 SIP: Session Initiation Protocol June 2002"  
 *     19.1.6 Relating SIP URIs and tel URLs
 */
typedef struct
{
        char *name;
        char *value;
} tel_param_t;
#define MAX_TEL_PARAMS (10)

int compare_tel_options(const void *v1, const void *v2)
{
        tel_param_t *p1 = (tel_param_t *) v1;
        tel_param_t *p2 = (tel_param_t *) v2;

        if (0 == strcasecmp(p1->name, "isdn-subaddress"))
        {
                return -1;
        }
        else if (0 == strcasecmp(p2->name, "isdn-subaddress"))
        {
                return 1;
        }
        else if (0 == strcasecmp(p1->name, "post-dial"))
        {
                return -1;
        }
        else if (0 == strcasecmp(p2->name, "post-dial"))
        {
                return 1;
        }
        else
        {
                return strcasecmp(p1->name, p2->name);
        }
}

/*
 *   Remove visual separators from the phone number 
 *   Assume it has been validated as a number containing 
 *   ONLY leading '+', digits, and visual separators.
 */
static void remove_visual_separators_from_phone(char *p)
{
        char *p2;
        p2 = p;
        while (*p != '\0')
        {
                 /* Skip all visual separators */
                 while ((*p != '\0') && ((*p == '.') || (*p == '-') || (*p == '(') || (*p == ')')))
                 {
                         p++; 
                 }
                 *p2 = *p; /* Until the first visual separator, these both point to  the same place. */
                           /* but, more efficient to just do it than an if statement each time. */
                 /* If we arrived at a terminator in the inner loop, time to exit */
                 if (*p == '\0') return;
                 /* Now we increment both pointers. */
                 p++;
                 p2++;
        }
        *p2 = '\0';  /* Make sure that the string is terminated after the last valid digit. */
}

/*
 * Check if this is a phone number.
 *   Assume possible leading '+'
 *   Assume separators '.', '-', '(', or ')' could be present.
 */
static int is_number(const char *p)
{
        if (*p == '+') p++;
        while (*p != '\0')
        {
                 if ((!isdigit(*p)) && (*p != '.') && (*p != '-') && (*p != '(') && (*p != ')')) return 0;
                 p++;
        }
        return 1;
}

/*
 * Converts URI, if it is tel URI, to SIP URI.  Returns 1, if
 * conversion succeeded or if no conversion was needed, i.e., URI was not
 * tel URI.  Returns -1, if conversion failed.  Takes SIP URI hostpart from
 * second parameter and (if needed) writes the result to third parameter.
 * This one attempts to be standards compliant and sort tel: uri parameters
 * copied to the sip: uri in the manner defined in the standard. It also 
 * deletes the "phone-context" parameter if it is a domain, and, takes visual
 * separators from the "phone-context" parameter if it is a telephone number.
 */
int std_tel2sip(struct sip_msg* _msg, char* _uri, char* _hostpart, char* _res)
{
        str uri, hostpart, tel_uri, sip_uri;
        char *at;
        int i, j, in_tel_parameters = 0;
        pv_spec_t *res;
        pv_value_t res_val;

        /* get parameters */
        if (get_str_fparam(&uri, _msg, (fparam_t*)_uri) < 0) {
                LM_ERR("failed to get uri value\n");
        }
        if (get_str_fparam(&hostpart, _msg, (fparam_t*)_hostpart) < 0) {
                LM_ERR("failed to get hostpart value\n");
        }
        res = (pv_spec_t *)_res;

        /* check if anything needs to be done */
        if (uri.len < 4) return 1;
        if (strncasecmp(uri.s, "tel:", 4) != 0) return 1;

        /* reserve memory for clean tel uri */
        tel_uri.s = pkg_malloc(uri.len+1);
        if (tel_uri.s == 0) {
                LM_ERR("no more pkg memory\n");
                return -1;
        }

        /* Remove visual separators before converting to SIP URI. Don't remove
         * visual separators in TEL URI parameters (after the first ";") */
        for (i=0, j=0; i < uri.len; i++) {
                if (in_tel_parameters == 0) {
                        if (uri.s[i] == ';')
                                in_tel_parameters = 1;
                }
                if (in_tel_parameters == 0) {
                        if ((uri.s[i] != '-') && (uri.s[i] != '.') &&
                                        (uri.s[i] != '(') && (uri.s[i] != ')'))
                                tel_uri.s[j++] = tolower(uri.s[i]);
                } else {
                        tel_uri.s[j++] = tolower(uri.s[i]);
                }
        }
        tel_uri.s[j] = '\0';
        tel_uri.len = strlen(tel_uri.s);

        /*** Start Code to sort tel: params *******/
        tel_param_t params[MAX_TEL_PARAMS];
        char *tmp_ptr = tel_uri.s + 4; // skip tel:

        int n_tel_params = 0;
        for (int i=0; i < MAX_TEL_PARAMS; i++)
        {
           tmp_ptr = strchr(tmp_ptr, ';');
           if (tmp_ptr == NULL)
           {
             break;
           }
           *tmp_ptr = '\0';
           tmp_ptr++;
           n_tel_params++;
           params[i].name = tmp_ptr;
        }
        for (int i=0; i < n_tel_params; i++)
        {
          tmp_ptr = strchr(params[i].name, '=');
          if (tmp_ptr == NULL)
          {
            params[i].value = "";
          }
          else
          {
             *tmp_ptr = '\0';
             tmp_ptr++;
             params[i].value = tmp_ptr;
          }
          if ((0 == strcasecmp(params[i].name, "phone-context")) && (is_number(params[i].value)))
          {
            remove_visual_separators_from_phone(params[i].value);
          }
           
        }
        if (n_tel_params > 1)
        {
          qsort(&params[0], n_tel_params, sizeof(tel_param_t), compare_tel_options);
        }
        /*** End Code to sort tel: params ******/

        /* reserve memory for resulting sip uri */
        sip_uri.len = 4 + tel_uri.len - 4 + 1 + hostpart.len + 1 + 10;
        sip_uri.s = pkg_malloc(sip_uri.len+1);
        if (sip_uri.s == 0) {
                LM_ERR("no more pkg memory\n");
                pkg_free(tel_uri.s);
                return -1;
        }

        /* create resulting sip uri */
        at = sip_uri.s;
        append_str(at, "sip:", 4);
        /** Original code tel: parameters NOT sorted 
        append_str(at, tel_uri.s + 4, tel_uri.len - 4);
        *****/
        /***** Start Changed Code for sorted tel: parameters ****/
        append_str(at, tel_uri.s + 4, strlen(tel_uri.s + 4)); /* This string was terminated after the number */
        /** Now we need to insert sorted tel: parameters **/
        for (int i=0; i < n_tel_params; i++)
        {
          /* If the phone context is a domain, it has already been extracted and is in the "host part" */
          if ((0 != strcasecmp(params[i].name, "phone-context")) || (is_number(params[i].value)))
          {
            append_chr(at, ';');
            append_str(at, params[i].name, strlen(params[i].name));
            append_chr(at, '=');
            append_str(at, params[i].value, strlen(params[i].value));
          }
        }
        /***** End Changed Code for sort tel: parameters ****/
        append_chr(at, '@');
        append_str(at, hostpart.s, hostpart.len);
        append_chr(at, ';');
        append_str(at, "user=phone", 10);

        /* tel_uri is not needed anymore */
        pkg_free(tel_uri.s);

        /* set result pv value and write sip uri to result pv */
        res_val.rs = sip_uri;
        res_val.flags = PV_VAL_STR;
        if (res->setf(_msg, &res->pvp, (int)EQ_T, &res_val) != 0) {
                LM_ERR("failed to set result pvar\n");
                pkg_free(sip_uri.s);
                return -1;
        }

        /* free allocated pkg memory and return */
        pkg_free(sip_uri.s);
        return 1;
}

@miconda
Copy link
Member

miconda commented Sep 3, 2017

Any chance for a PR sometime soon, so this item can be closed here?

@doncarr
Copy link
Author

doncarr commented Sep 12, 2017 via email

@ngvoice
Copy link
Member

ngvoice commented Sep 12, 2017

Hi,

i've reviewed the code and made some minor improvements. I will commit a slightly modified version later this week.

Thanks,
Carsten

@doncarr
Copy link
Author

doncarr commented Sep 12, 2017 via email

@henningw
Copy link
Contributor

henningw commented Mar 3, 2018

Hi Carsten, was this already commited? Somehow I did not found it, maybe I did not looked carefully enough.

@doncarr
Copy link
Author

doncarr commented Mar 4, 2018 via email

@henningw
Copy link
Contributor

henningw commented Nov 1, 2018

I did a quick search for some of the functions from the code block above, did not found them in git master.

@ngvoice - did you managed to commit this already?

@ngvoice ngvoice self-assigned this Nov 2, 2018
@ngvoice
Copy link
Member

ngvoice commented Nov 2, 2018

I've reviewed the code and tested it intensively. I will make patch of it soon. Due to a busy schedule, this patch did not make it upstream yet....

@doncarr
Copy link
Author

doncarr commented Nov 3, 2018 via email

@henningw
Copy link
Contributor

Close this one - please submit your code as a new pull request if you are still interested on working on this.

herlesupreeth added a commit to herlesupreeth/kamailio that referenced this issue May 6, 2021
herlesupreeth added a commit to herlesupreeth/kamailio that referenced this issue Jun 2, 2021
herlesupreeth added a commit to herlesupreeth/kamailio that referenced this issue Jun 2, 2021
herlesupreeth added a commit to herlesupreeth/kamailio that referenced this issue Jun 2, 2021
axelsommerfeldt pushed a commit to axelsommerfeldt/kamailio_herlesupreeth that referenced this issue Mar 23, 2023
nevian427 pushed a commit to nevian427/kamailio that referenced this issue May 24, 2023
axelsommerfeldt pushed a commit to axelsommerfeldt/kamailio_herlesupreeth that referenced this issue Jul 14, 2023
axelsommerfeldt pushed a commit to axelsommerfeldt/kamailio_herlesupreeth that referenced this issue Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants