sr-dev August 2015

sr-dev@lists.kamailio.org

30 participants
191 discussions

[kamailio] Fix read buffer overflow in parse_hname2 (#308)
by Chris Double 02 Sep '15

02 Sep '15

Fix buffer overflow in READ call by making a SAFE_READ that checks the actual length of the buffer. In the buffer overflow case parse_hname2 is called with 'begin' set to the string "Reason:". This string was originally allocated in in rval_get_str as length 6, contents "Reason\0'. The actual pkg_malloc is size of 7 to account for the null terminator. In the caller to parse_hname2 (modules/textops/textops.c line 2229) the null terminator is replaced with a ':' character. parse_hname2 hits the FIRST_QUARTERNIONS macro which expands to a bunch of case statements. The one for the Reason string looks like (macro expanded): case _reas_: p += 4; val = READ(p); switch(LOWER_DWORD(val)) { case _on1_: hdr->type = HDR_REASON_T; hdr->name.len = 6; return (p + 3); The overflow occurs in the READ call. READ is: (*(val + 0) + (*(val + 1) << 8) + (*(val + 2) << 16) + (*(val + 3) << 24)) With 'p' pointing to "Reason:", then p+4 is "on:". That's only three characters of allocated memory left(the : was originally the null character as explained above and the total pkg_malloc allocated length was 7). READ accesses 4 bytes so we go one past the end of the allocated area. The error is noticeable in a DBG_SYS_MALLOC build but not a PKG_MALLOC build - I assume the latter has a large arena allocated making the buffer overflow still valid memory. There are likely other buffer overflows in the READ usage in other cases in this function. I've [posted to the mailing list](http://lists.sip-router.org/pipermail/sr-dev/2015-August/030529.html) about the issue and whether a more general fix is possible: You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/308 -- Commit Summary -- * Fix read buffer overflow in parse_hname2 -- File Changes -- M parser/case_reas.h (2) M parser/parse_hname2.c (19) -- Patch Links -- https://github.com/kamailio/kamailio/pull/308.patch https://github.com/kamailio/kamailio/pull/308.diff --- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308

2 5

[kamailio] Change remove_ts_transaction so it updates head pointer of list (#311)
by Chris Double 01 Sep '15

01 Sep '15

If the transaction being removed is the first item in the list of transactions then we need to update the pointer to the head of the list so it does not have a stale reference. This is an attempt to fix the tsilo crashes we've been seeing. The crashes occur in ts_onreply while iterating over the transactions to remove the transaction for the TMCB_DESTROY callback. One of the transaction pointers is not a valid shared memory address and the process crashes. This crash results in the main kamailio process getting a SIGCHILD signal and it tries to shut down. This reaches code to free the transactions and it crashes in free_ts_urecord while trying to free the same transaction. Inspecting code I can't tell how remove_ts_transaction resets urecord->transactions to be the head of the list of the first transaction to be removed is the first item in the list. This would leave a dangling pointer there and seems likely to be the cause of the crash. You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/311 -- Commit Summary -- * Change remove_ts_transaction so it updates head pointer of list -- File Changes -- M modules/tsilo/ts_hash.c (4) -- Patch Links -- https://github.com/kamailio/kamailio/pull/311.patch https://github.com/kamailio/kamailio/pull/311.diff --- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/311

3 4

Buffer overflow in parse_hname2
by Chris Double 01 Sep '15

01 Sep '15

In parser/parse_hname2.c we've hit what looks to be a 1 byte buffer overflow error. This was picked up in an ASAN enabled build. This is on 4.3 branch. Investigation shows that parse_hname2 is called with 'begin' set to the string "Reason:". This string was originally allocated in in rval_get_str as length 6, contents "Reason\0'. The actual pkg_malloc is size of 7 to account for the null terminator. In the caller to parse_hname2 (modules/textops/textops.c line 2229) the null terminator is replaced with a ':' character. parse_hname2 hits the FIRST_QUARTERNIONS macro which expands to a bunch of case statements. The one for the Reason string looks like (macro expanded): case _reas_: p += 4; val = READ(p); switch(LOWER_DWORD(val)) { case _on1_: hdr->type = HDR_REASON_T; hdr->name.len = 6; return (p + 3); The overflow occurs in the READ call. READ is: #define READ(val) \ (*(val + 0) + (*(val + 1) << 8) + (*(val + 2) << 16) + (*(val + 3) << 24)) With 'p' pointing to "Reason:", then p+4 is "on:". That's only three characters of allocated memory left(the : was originally the null character as explained above and the total pkg_malloc allocated length was 7). READ accesses 4 bytes so we go one past the end of the allocated area. ASAN bails at this point but in a non-ASAN build it would reach the 'case _on1_' clause. __on1__ is defined as: #define _on1_ 0x203a6e6f /* "on: " */ This matches for a space at the end of the string but that space won't exist since there were only 7 characters originally allocated. Only by good luck would it match. The error is noticeable in a DBG_SYS_MALLOC build but not a PKG_MALLOC build - I assume the latter has a large arena allocated making the buffer overflow still valid memory. Assuming my analysis is correct I'd like to fix this by putting some length checking in places and using a READ call that accounts for it. Would this be an acceptable approach? It's pretty complex code and I don't want to mess up so I welcome advice on how to address the issue if there's a better way. -- http://bluishcoder.co.nz

2 2

db_free_columns and freeing column name
by Chris Double 01 Sep '15

01 Sep '15

This question relates to the commit in http://lists.sip-router.org/pipermail/sr-dev/2015-August/030514.html The db1_res_1 structure stores a string for the column names in the query. The commit in the email referenced above fixed an issue in the postgres backend where it was storing an internal postgres pointer in the result structure but by the time the results were used in sqlops a PQclear had been performed leaving that pointer dangling. Eventually a use after free occurs. The fix in the commit is to copy the column name into a pkg_malloc'd area. The question I have is where should this be free'd. I thought db_free_columns in lib/srdb1/db_res.c would be the place. In that function it frees the column str object (RES_NAMES(_r)[col]) but not the string char* (RES_NAMES(_r)[col]->s). Changing that function to free the string would seem to be the right fix - is it free'd anywhere else that I'm missing? Looking at the other database backends to work out whether they store anything there that needs to be free'd shows similar issues to the postgres one that was fixed in the commit referenced earlier: db_berkeley: Uses a pointer to internal database results db_mongodb: Uses a pointer to internal database results db_mysql: Uses a pointer to internal database results db_text: Uses a pointer to internal database results db_unixodbc: Uses a pointer to a stack variable The other backends (oracle, sqlite) seem to do the same as the fix made to the postgres backend. The unixodbc backend seem to have a dangling pointer into the stack which is problematic. Should these DB backends be changed to copy the column name instead of storing an internal pointer? If so, is db_free_columns the correct place to free that memory? Is there anywhere else that stores column name data in results which might need to be modified? -- http://bluishcoder.co.nz

2 1

kex & pkg_info
by Carsten Bock 01 Sep '15

01 Sep '15

Hi, quick question: We've compiled Kamailio with the following settings: version: kamailio 4.3.1 (x86_64/linux) 7cd85b 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, F_MALLOC, DBG_F_MALLOC, 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: 7cd85b (we are using F_MALLOC to do some memory debugging) However, with this compiler settings, we cannot use the "kex" module anymore: kamailio -c loading modules under config path: /usr/lib64/kamailio/modules/:/usr/lib/kamailio/modules/:/usr/lib/x86_64-linux-gnu/kamailio/modules/ 0(25506) ERROR: <core> [sr_module.c:574]: load_module(): could not open module </usr/lib/x86_64-linux-gnu/kamailio/modules/kex.so>: /usr/lib/x86_64-linux-gnu/kamailio/modules/kex.so: undefined symbol: pkg_info 0(25506) : <core> [cfg.y:3432]: yyerror_at(): parse error in config file /etc/kamailio/kamailio.cfg, line 63, column 12-16: failed to load module Looking at the files at "mem/*", the pkg_info seems to be defined for me. Does anyone (with more knowledge on the Memory stuff) have a clue, why this is not working? Thanks, Carsten -- Carsten Bock CEO (Geschäftsführer) ng-voice GmbH Schomburgstr. 80 D-22767 Hamburg / Germany http://www.ng-voice.com mailto:carsten@ng-voice.com Office +49 40 5247593-0 Fax +49 40 5247593-99 Sitz der Gesellschaft: Hamburg Registergericht: Amtsgericht Hamburg, HRB 120189 Geschäftsführer: Carsten Bock Ust-ID: DE279344284 Hier finden Sie unsere handelsrechtlichen Pflichtangaben: http://www.ng-voice.com/imprint/

2 3

[kamailio] pv: create command to copy script vars to xavp (#309)
by Victor Seva 01 Sep '15

01 Sep '15

pv_scriptvar2xavp(varname, xname) Copy script variables values to a xavp. First parameter can be '*' in order to copy all script variables. Second parameter is the name of the destination xavp. If xavp already exists it will be reset first. Function can be used from ANY_ROUTE. You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/309 -- Commit Summary -- * pv: scriptvar2xavp * pv: refresh README -- File Changes -- M modules/pv/README (58) M modules/pv/doc/pv_admin.xml (31) M modules/pv/pv.c (17) M modules/pv/pv_svar.c (4) M modules/pv/pv_svar.h (1) M modules/pv/pv_xavp.c (61) M modules/pv/pv_xavp.h (5) -- Patch Links -- https://github.com/kamailio/kamailio/pull/309.patch https://github.com/kamailio/kamailio/pull/309.diff --- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/309

2 2

[kamailio] Small typo fix in config. (#307)
by Matthias Wimmer 01 Sep '15

01 Sep '15

There is a small typo in kamailio.cfg where "Rotuing" has been written instead of "Routing". You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/307 -- Commit Summary -- * Small typo fix in config. -- File Changes -- M etc/kamailio.cfg (2) -- Patch Links -- https://github.com/kamailio/kamailio/pull/307.patch https://github.com/kamailio/kamailio/pull/307.diff --- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/307

2 2

git:4.3:558a65e4: utils: Fix buffer overflow; do not NULL-terminate HTTP result
by Carsten Bock 28 Aug '15

28 Aug '15

Module: kamailio Branch: 4.3 Commit: 558a65e48e6819140b409cf58a0340aa78b8c2cf URL: https://github.com/kamailio/kamailio/commit/558a65e48e6819140b409cf58a0340a… Author: Carsten Bock <carsten(a)ng-voice.com> Committer: Carsten Bock <carsten(a)ng-voice.com> Date: 2015-08-28T12:12:34+02:00 utils: Fix buffer overflow; do not NULL-terminate HTTP result Fix buffer overflow in the `write_function` that takes the resulting data from libcurl. The function was trying to NULL terminate the string, but this could result in overwriting the buffer by one byte when size*nmemb == 1. This also caused some memory corruptions, reported on sr-dev. Reported by: Travis Cross <tc(a)traviscross.com> --- Modified: modules/utils/functions.c --- Diff: https://github.com/kamailio/kamailio/commit/558a65e48e6819140b409cf58a0340a… Patch: https://github.com/kamailio/kamailio/commit/558a65e48e6819140b409cf58a0340a… --- diff --git a/modules/utils/functions.c b/modules/utils/functions.c index d32d970..f261cc0 100644 --- a/modules/utils/functions.c +++ b/modules/utils/functions.c @@ -2,7 +2,7 @@ * script functions of utils module * * Copyright (C) 2008 Juha Heinanen - * Copyright (C) 2013 Carsten Bock, ng-voice GmbH + * Copyright (C) 2013-2015 Carsten Bock, ng-voice GmbH * * This file is part of Kamailio, a free SIP server. * @@ -55,7 +55,7 @@ size_t write_function( void *ptr, size_t size, size_t nmemb, void *stream_ptr) http_res_stream_t *stream = (http_res_stream_t *) stream_ptr; stream->buf = (char *) pkg_realloc(stream->buf, stream->curr_size + - (size * nmemb) + 1); + (size * nmemb)); if (stream->buf == NULL) { LM_ERR("cannot allocate memory for stream\n"); @@ -64,15 +64,12 @@ size_t write_function( void *ptr, size_t size, size_t nmemb, void *stream_ptr) memcpy(&stream->buf[stream->pos], (char *) ptr, (size * nmemb)); - stream->curr_size += ((size * nmemb) + 1); + stream->curr_size += (size * nmemb); stream->pos += (size * nmemb); - stream->buf[stream->pos + 1] = '\0'; - return size * nmemb; } - /* * Performs http_query and saves possible result (first body line of reply) * to pvar.

2 1

git:4.2:0b18aea1: geoip: set null termination to the field used for query
by Daniel-Constantin Mierla 28 Aug '15

28 Aug '15

Module: kamailio Branch: 4.2 Commit: 0b18aea1f48584a4ee9ce445dfca7c25794c6c73 URL: https://github.com/kamailio/kamailio/commit/0b18aea1f48584a4ee9ce445dfca7c2… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-08-28T12:35:16+02:00 geoip: set null termination to the field used for query (cherry picked from commit ec2a72d75dc0a2c9c3d88361094fdda7a7477935) (cherry picked from commit 4c24d4acbca4e5ec99b5de1dd5f29d2cdeae9e2b) --- Modified: modules/geoip/geoip_pv.c --- Diff: https://github.com/kamailio/kamailio/commit/0b18aea1f48584a4ee9ce445dfca7c2… Patch: https://github.com/kamailio/kamailio/commit/0b18aea1f48584a4ee9ce445dfca7c2… --- diff --git a/modules/geoip/geoip_pv.c b/modules/geoip/geoip_pv.c index 60f37ca..bcc982d 100644 --- a/modules/geoip/geoip_pv.c +++ b/modules/geoip/geoip_pv.c @@ -415,7 +415,7 @@ int geoip_update_pv(str *tomatch, str *name) } strncpy(gr->tomatch, tomatch->s, tomatch->len); - tomatch->s[tomatch->len] = '\0'; + gr->tomatch[tomatch->len] = '\0'; gr->record = GeoIP_record_by_name(_handle_GeoIP, (const char*)gr->tomatch); LM_DBG("attempt to match: %s\n", gr->tomatch);

1 0

git:4.3:4c24d4ac: geoip: set null termination to the field used for query
by Daniel-Constantin Mierla 28 Aug '15

28 Aug '15

Module: kamailio Branch: 4.3 Commit: 4c24d4acbca4e5ec99b5de1dd5f29d2cdeae9e2b URL: https://github.com/kamailio/kamailio/commit/4c24d4acbca4e5ec99b5de1dd5f29d2… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-08-28T12:34:40+02:00 geoip: set null termination to the field used for query (cherry picked from commit ec2a72d75dc0a2c9c3d88361094fdda7a7477935) --- Modified: modules/geoip/geoip_pv.c --- Diff: https://github.com/kamailio/kamailio/commit/4c24d4acbca4e5ec99b5de1dd5f29d2… Patch: https://github.com/kamailio/kamailio/commit/4c24d4acbca4e5ec99b5de1dd5f29d2… --- diff --git a/modules/geoip/geoip_pv.c b/modules/geoip/geoip_pv.c index 46c4dff..ba12f16 100644 --- a/modules/geoip/geoip_pv.c +++ b/modules/geoip/geoip_pv.c @@ -422,7 +422,7 @@ int geoip_update_pv(str *tomatch, str *name) } strncpy(gr->tomatch, tomatch->s, tomatch->len); - tomatch->s[tomatch->len] = '\0'; + gr->tomatch[tomatch->len] = '\0'; gr->record = GeoIP_record_by_name(_handle_GeoIP, (const char*)gr->tomatch); LM_DBG("attempt to match: %s\n", gr->tomatch);

1 0

← Newer
1
2
3
4
5
6
7
8
...
20
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

sr-dev August 2015