Module: sip-router Branch: sr_3.0 Commit: 9d98ca32bb131c0fb190e012ed0bff3f9a26557a URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=9d98ca32...
Author: Juha Heinanen jh@tutpro.com Committer: Juha Heinanen jh@tutpro.com Date: Wed Dec 30 12:19:03 2009 +0200
modules_k/nathelper: handle_uri_alias() alias handling fix - handle_uri_alias() now finds ;alias r-uri param even if it is not the first param.
---
modules_k/nathelper/nathelper.c | 53 ++++++++++++++++++++++++-------------- 1 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/modules_k/nathelper/nathelper.c b/modules_k/nathelper/nathelper.c index fadc93f..27ef349 100644 --- a/modules_k/nathelper/nathelper.c +++ b/modules_k/nathelper/nathelper.c @@ -1501,41 +1501,54 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) static int handle_ruri_alias_f(struct sip_msg* msg, char* str1, char* str2) { - str params, uri, proto; - char buf[MAX_URI_SIZE], *val, *sep, *trans, *at, *next, *cur_uri; - unsigned int len, plen, alias_len, proto_type, cur_uri_len; + str uri, proto; + char buf[MAX_URI_SIZE], *val, *sep, *trans, *at, *next, *cur_uri, *rest; + unsigned int len, rest_len, val_len, alias_len, proto_type, cur_uri_len, + ip_port_len;
if ((msg->parsed_uri_ok == 0) && (parse_sip_msg_uri(msg) < 0)) { LM_ERR("while parsing Request-URI\n"); return -1; } - params = msg->parsed_uri.params; - if (params.len == 0) { + rest = msg->parsed_uri.params.s; + rest_len = msg->parsed_uri.params.len; + if (rest_len == 0) { LM_DBG("no params\n"); return 2; } - if ((params.len < ALIAS_LEN) || - (strncmp(params.s, ALIAS, ALIAS_LEN) != 0)) { + while (rest_len >= ALIAS_LEN) { + if (strncmp(rest, ALIAS, ALIAS_LEN) == 0) break; + sep = memchr(rest, 59 /* ; */, rest_len); + if (sep == NULL) { + LM_DBG("no alias param\n"); + return 2; + } else { + rest_len = rest_len - (sep - rest + 1); + rest = sep + 1; + } + } + + if (rest_len < ALIAS_LEN) { LM_DBG("no alias param\n"); return 2; }
/* set dst uri based on alias param value */ - val = params.s + ALIAS_LEN; - plen = params.len - ALIAS_LEN; - sep = memchr(val, 116 /* t */, plen); + val = rest + ALIAS_LEN; + val_len = rest_len - ALIAS_LEN; + sep = memchr(val, 116 /* t */, val_len); if (sep == NULL) { - LM_ERR("no 't' in alias param\n"); + LM_ERR("no 't' in alias param value\n"); return -1; } at = &(buf[0]); append_str(at, "sip:", 4); - len = sep - val; - alias_len = SALIAS_LEN + len + 2 /* tn */; - memcpy(at, val, len); - at = at + len; + ip_port_len = sep - val; + alias_len = SALIAS_LEN + ip_port_len + 2 /* tn */; + memcpy(at, val, ip_port_len); + at = at + ip_port_len; trans = sep + 1; - if ((len + 2 > plen) || (*trans == ';') || (*trans == '?')) { + if ((ip_port_len + 2 > val_len) || (*trans == ';') || (*trans == '?')) { LM_ERR("no proto in alias param\n"); return -1; } @@ -1543,7 +1556,7 @@ handle_ruri_alias_f(struct sip_msg* msg, char* str1, char* str2) if (proto_type != PROTO_UDP) { proto_type_to_str(proto_type, &proto); if (proto.len == 0) { - LM_ERR("unkown proto in alias param\n"); + LM_ERR("unknown proto in alias param\n"); return -1; } append_str(at, ";transport=", 11); @@ -1551,7 +1564,7 @@ handle_ruri_alias_f(struct sip_msg* msg, char* str1, char* str2) at = at + proto.len; } next = trans + 1; - if ((len + 2 < plen) && (*next != ';') && (*next != '?')) { + if ((ip_port_len + 2 < val_len) && (*next != ';') && (*next != '?')) { LM_ERR("invalid alias param value\n"); return -1; } @@ -1572,11 +1585,11 @@ handle_ruri_alias_f(struct sip_msg* msg, char* str1, char* str2) cur_uri_len = msg->first_line.u.request.uri.len; } at = &(buf[0]); - len = params.s - 1 /* ; */ - cur_uri; + len = rest - 1 /* ; */ - cur_uri; memcpy(at, cur_uri, len); at = at + len; len = cur_uri_len - alias_len - len; - memcpy(at, params.s + alias_len - 1, len); + memcpy(at, rest + alias_len - 1, len); uri.s = &(buf[0]); uri.len = cur_uri_len - alias_len; LM_DBG("rewriting r-uri to <%.*s>\n", uri.len, uri.s);
Hi Juha!
Thanks for the fix, the new version works fine for me.
Nevertheless, I get lots of warnings:
gcc -fPIC -DPIC -g -O9 -funroll-loops -Wcast-align -m32 -minline-all-stringops -falign-loops -ftree-vectorize -fno-strict-overflow -mtune=athlon64 -Wall -DNAME='"ser"' -DVERSION='"2.99.99-pre3"' -DARCH='"i386"' -DOS='linux_' -DOS_QUOTED='"linux"' -DCOMPILER='"gcc 4.3.2"' -D__CPU_i386 -D__OS_linux -DSER_VER=2099099 -DCFG_DIR='"/usr/local/etc/ser/"' -DPKG_MALLOC -DSHM_MEM -DSHM_MMAP -DDNS_IP_HACK -DUSE_IPV6 -DUSE_MCAST -DUSE_TCP -DDISABLE_NAGLE -DHAVE_RESOLV_RES -DUSE_DNS_CACHE -DUSE_DNS_FAILOVER -DUSE_DST_BLACKLIST -DUSE_NAPTR -DDBG_QM_MALLOC -DUSE_TLS -DTLS_HOOKS -DFAST_LOCK -DADAPTIVE_WAIT -DADAPTIVE_WAIT_LOOPS=1024 -DCC_GCC_LIKE_ASM -DHAVE_GETHOSTBYNAME2 -DHAVE_UNION_SEMUN -DHAVE_SCHED_YIELD -DHAVE_MSG_NOSIGNAL -DHAVE_MSGHDR_MSG_CONTROL -DHAVE_ALLOCA_H -DHAVE_TIMEGM -DHAVE_SCHED_SETSCHEDULER -DHAVE_EPOLL -DHAVE_SIGIO_RT -DSIGINFO64_WORKARROUND -DUSE_FUTEX -DHAVE_SELECT -DOPENSER_MOD_INTERFACE -DMOD_NAME='"nathelper"' -c nathelper.c -o nathelper.o nathelper.c: In function 'pv_get_rr_top_count_f': nathelper.c:1647: warning: unused variable 'header' nathelper.c:1646: warning: unused variable 'count' nathelper.c: At top level: nathelper.c:286: warning: 'rr_count_f' declared 'static' but never defined nathelper.c:287: warning: 'rr_top_count_f' declared 'static' but never defined
regards Klaus
Juha Heinanen schrieb:
Klaus Darilion writes:
Nevertheless, I get lots of warnings:
that is because i have in debian/rules QUIET=1 and didn't see those.
how can i achieve verbose re-compilation of one module? i have tried
QUIET=1 export QUIET make modules=modules_k/modules nathelper
but still it gets compiled quietly.
or would it be possible to make QUIET=1 to show not only errors, but also warnings?
-- juha
klaus,
after editing my debian/rules i managed to compile nathelper in verbose mode and didn't get any warnings:
make[2]: Entering directory `/usr/src/trunk-src/openxg-sip-proxy/modules_k/nathelper' gcc -fPIC -DPIC -g -O2 -DNAME='"sip-proxy"' -DVERSION='"2.99.99-pre3"' -DARCH='"i386"' -DOS='linux_' -DOS_QUOTED='"linux"' -DCOMPILER='"gcc 4.3.2"' -D__CPU_i386 -D__OS_linux -DSER_VER=2099099 -DCFG_DIR='"/etc/sip-proxy/"' -DPKG_MALLOC -DSHM_MEM -DSHM_MMAP -DDNS_IP_HACK -DUSE_MCAST -DUSE_TCP -DDISABLE_NAGLE -DHAVE_RESOLV_RES -DUSE_DNS_CACHE -DUSE_DNS_FAILOVER -DUSE_DST_BLACKLIST -DUSE_NAPTR -DF_MALLOC -DUSE_TLS -DTLS_HOOKS -DSTATISTICS -DFAST_LOCK -DADAPTIVE_WAIT -DADAPTIVE_WAIT_LOOPS=1024 -DCC_GCC_LIKE_ASM -DHAVE_GETHOSTBYNAME2 -DHAVE_UNION_SEMUN -DHAVE_SCHED_YIELD -DHAVE_MSG_NOSIGNAL -DHAVE_MSGHDR_MSG_CONTROL -DHAVE_ALLOCA_H -DHAVE_TIMEGM -DHAVE_SCHED_SETSCHEDULER -DHAVE_EPOLL -DHAVE_SIGIO_RT -DSIGINFO64_WORKARROUND -DUSE_FUTEX -DHAVE_SELECT -DUSE_SCTP -DOPENSER_MOD_INTERFACE -DMOD_NAME='"nathelper"' -c nathelper.c -o nathelper.o
i think you got the warnings from the test version that i emailed to you privately. causes of the warnings have since then been fixed in the git version.
-- juha
Hi Juha!
One more suggestion: I think a note should be added to README like:
Note: If you are using add_contact_alias() and handle_ruri_alias() this means that you are routing based on the alias parameter. Thus, make sure that an attacker can not spoof this paramter, e.g. screen the contact header and RURI for existing 'alias' parameters. Especially for initial requests make sure to route only on alias paramters which were added by your system.
Maybe add_contact_alias() should overwrite existing alias parameters?
regards klaus
Juha Heinanen schrieb:
Klaus Darilion writes:
klaus,
thanks for your comment and tests.
in the alias usage example that i gave, handle_ruri_alias() is only called on in-dialog requests. so i don't see any bigger security issue if r-uri uri has alias param than if it doesn't.
Maybe add_contact_alias() should overwrite existing alias parameters?
my opinion on this is that if someone wants to shoot him/hershelf in the foot, then be it.
-- juha
Juha Heinanen schrieb:
That means in your example it is safe to do. But other people will use (these useful functions) in different scenarios - e.g. adding the alias on an outbound proxy before forwarding it the a main proxy - thus users should be sensitive about security too - not only about functionality. And IMO *ser* lacks documentation of secure configuration.
regards klaus
-
jh＠tutpro.com -
Juha Heinanen
-
Klaus Darilion