sr-dev September 2015

sr-dev@lists.kamailio.org

37 participants
388 discussions

git:master:964ed0a5: parser: fix overflow access when parsing Reason header stored in short buffer
by Daniel-Constantin Mierla 02 Sep '15

02 Sep '15

Module: kamailio Branch: master Commit: 964ed0a5083413eb0a70bd8a952d5a91ee9e9883 URL: https://github.com/kamailio/kamailio/commit/964ed0a5083413eb0a70bd8a952d5a9… Author: Chris Double <chris.double(a)double.co.nz> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-09-02T13:15:37+02:00 parser: fix overflow access when parsing Reason header stored in short buffer - it can happen for fixup functions from textops module having header name as a parameter, with critical impact when using system malloc, the internal pkg malloc does a roundup of the allocated space - the issue is caused by the word (4 bytes) read step performed by parse_hname2() - second 4-byte read in buffer "Reason:" exceeds the size by 1 - added a safe read macro that reads 1, 2 or 3 bytes if the size of the buffer is not big enough for a 4 bytes read --- Modified: parser/case_reas.h Modified: parser/parse_hname2.c --- Diff: https://github.com/kamailio/kamailio/commit/964ed0a5083413eb0a70bd8a952d5a9… Patch: https://github.com/kamailio/kamailio/commit/964ed0a5083413eb0a70bd8a952d5a9… --- diff --git a/parser/case_reas.h b/parser/case_reas.h index e6b8d97..5df0a9d 100644 --- a/parser/case_reas.h +++ b/parser/case_reas.h @@ -49,7 +49,7 @@ #define reas_CASE \ p += 4; \ - val = READ(p); \ + val = SAFE_READ(p, end - p); \ ON_CASE; \ goto other; diff --git a/parser/parse_hname2.c b/parser/parse_hname2.c index 42fb865..e4188a0 100644 --- a/parser/parse_hname2.c +++ b/parser/parse_hname2.c @@ -95,11 +95,26 @@ static inline char* skip_ws(char* p, unsigned int size) /*@} */ +#define SAFE_READ(val, len) \ +((len) == 1 ? READ1(val) : ((len) == 2 ? READ2(val) : ((len) == 3 ? READ3(val) : ((len) > 3 ? READ4(val) : READ0(val))))) + #define READ(val) \ -(*(val + 0) + (*(val + 1) << 8) + (*(val + 2) << 16) + (*(val + 3) << 24)) +READ4(val) + +#define READ4(val) \ +(*((val) + 0) + (*((val) + 1) << 8) + (*((val) + 2) << 16) + (*((val) + 3) << 24)) #define READ3(val) \ -(*(val + 0) + (*(val + 1) << 8) + (*(val + 2) << 16)) +(*((val) + 0) + (*((val) + 1) << 8) + (*((val) + 2) << 16)) + +#define READ2(val) \ +(*((val) + 0) + (*((val) + 1) << 8)) + +#define READ1(val) \ +(*((val) + 0)) + +#define READ0(val) \ +(0) #define FIRST_QUATERNIONS \ case _via1_: via1_CASE; \

1 0

git:master:cbc5034b: parser: safer function to parse header name stored in short buffer
by Daniel-Constantin Mierla 02 Sep '15

02 Sep '15

Module: kamailio Branch: master Commit: cbc5034b26189371c2ffb80424414aa8b1b8dea5 URL: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-09-02T13:07:45+02:00 parser: safer function to parse header name stored in short buffer --- Modified: parser/parse_hname2.c Modified: parser/parse_hname2.h --- Diff: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… Patch: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… --- diff --git a/parser/parse_hname2.c b/parser/parse_hname2.c index c90d3e5..42fb865 100644 --- a/parser/parse_hname2.c +++ b/parser/parse_hname2.c @@ -242,3 +242,31 @@ char* parse_hname2(char* const begin, const char* const end, struct hdr_field* c } } +/** + * parse_hname2_short() - safer version to parse header name stored in short buffers + * - parse_hanem2() reads 4 bytes at once, expecting to walk through a buffer + * that contains more than the header name (e.g., sip msg buf, full header buf + * with name and body) + */ +char* parse_hname2_short(char* const begin, const char* const end, struct hdr_field* const hdr) +{ +#define HBUF_MAX_SIZE 256 + char hbuf[HBUF_MAX_SIZE]; + char *p; + + if(end-begin>=HBUF_MAX_SIZE-4) { + p = q_memchr(p, ':', end - begin); + if(p && p-4> begin) { + /* header name termination char found and enough space in buffer after it */ + return parse_hname2(begin, end, hdr); + } + /* not enough space */ + return NULL; + } + /* pad with whitespace - tipycal char after the ':' of the header name */ + memset(hbuf, ' ', HBUF_MAX_SIZE); + memcpy(hbuf, begin, end-begin); + p = parse_hname2(hbuf, hbuf + 4 + (end-begin), hdr); + if(!p) return NULL; + return begin + (p-hbuf); +} diff --git a/parser/parse_hname2.h b/parser/parse_hname2.h index 6f5b76b..d44abb7 100644 --- a/parser/parse_hname2.h +++ b/parser/parse_hname2.h @@ -36,5 +36,6 @@ * @file */ char* parse_hname2(char* const begin, const char* const end, struct hdr_field* const hdr); +char* parse_hname2_safe(char* const begin, const char* const end, struct hdr_field* const hdr); #endif /* PARSE_HNAME2_H */

1 0

git:master:aad248fc: tmx: err level replaced with info when getting t reply code pv
by Daniel-Constantin Mierla 02 Sep '15

02 Sep '15

Module: kamailio Branch: master Commit: aad248fc39279f84879929772e9306fc01473860 URL: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-09-02T09:18:09+02:00 tmx: err level replaced with info when getting t reply code pv --- Modified: modules/tmx/t_var.c --- Diff: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… Patch: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… --- diff --git a/modules/tmx/t_var.c b/modules/tmx/t_var.c index 0338233..fa1478c 100644 --- a/modules/tmx/t_var.c +++ b/modules/tmx/t_var.c @@ -518,7 +518,8 @@ int pv_get_tm_reply_code(struct sip_msg *msg, pv_param_t *param, } break; default: - LM_ERR("unsupported route_type %d\n", get_route_type()); + LM_INFO("unsupported route_type %d - code set to 0\n", + get_route_type()); code = 0; } }

1 0

git:4.2:99ea3939: tsilo: Fix transaction removal to update list head
by Federico Cabiddu 01 Sep '15

01 Sep '15

Module: kamailio Branch: 4.2 Commit: 99ea393959af90a9ea612f431a7c6e3648434f75 URL: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… Author: Chris Double <chris.double(a)double.co.nz> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:30:56+02:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. (cherry picked from commit 6ce6803d57dabe287d7d6fa859e93c1df402d821) --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… Patch: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index 31e1dcf..44f6dce 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

1 0

git:4.3:4d1b658b: tsilo: Fix transaction removal to update list head
by Federico Cabiddu 01 Sep '15

01 Sep '15

Module: kamailio Branch: 4.3 Commit: 4d1b658b2b380349b7d74fae7083661a18f04bf1 URL: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… Author: Chris Double <chris.double(a)double.co.nz> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:30:30+02:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. (cherry picked from commit 6ce6803d57dabe287d7d6fa859e93c1df402d821) --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… Patch: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

1 0

git:master:199202f4: Merge pull request #311 from doublec/tsilo_transaction_fix
by Federico Cabiddu 01 Sep '15

01 Sep '15

Module: kamailio Branch: master Commit: 199202f453b4a5f01783a4c2ba5c5ed2e10b0739 URL: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… Author: Federico Cabiddu <federico.cabiddu(a)gmail.com> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:28:29+02:00 Merge pull request #311 from doublec/tsilo_transaction_fix Change remove_ts_transaction so it updates head pointer of list --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… Patch: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

1 0

git:master:6ce6803d: tsilo: Fix transaction removal to update list head
by Chris Double 01 Sep '15

01 Sep '15

Module: kamailio Branch: master Commit: 6ce6803d57dabe287d7d6fa859e93c1df402d821 URL: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… Author: Chris Double <chris.double(a)double.co.nz> Committer: Chris Double <chris.double(a)double.co.nz> Date: 2015-09-02T00:45:58+12:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… Patch: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

1 0

[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

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

sr-dev September 2015