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
This is fixing the buffer overflow access, but it is not fixing the matching, right? There is no case for reading "on:" string.
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308#issuecomment-136713318
The _on1_ case matches for "on: ". The result of SAFE_READ has LOWER_DWORD called on it which makes the "on:" result become "on: " so the case matches.
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308#issuecomment-136890059
Analyzed the code and the match was happening anyhow because the case for match last part of "Reason:" is having IF conditions to cover the last three bytes, instead of the full 4 bytes value - inside parser/case_reas.h:
``` if ((LOWER_DWORD(val)&0x00ffffff) ==\ (_on1_&0x00ffffff)){ \ hdr->type = HDR_REASON_T; \ hdr->name.len = 6; \ return (p+3); \ } ```
I am going to merge manually, to adjust the commit message. Also, I will push some other patches to make the use of parse_hname2() safer for fixups and parsing when only the header name is in the input buffer.
Given that the header name parsing became again actual, I may revive my plans to rework it with a better approach.
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308#issuecomment-137021784
Closed #308.
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308#event-398832473
Pushed manually to the master branch for now (commit 964ed0a5083413eb0a70bd8a952d5a91ee9e9883).
--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/308#issuecomment-137044001
-
Chris Double -
Daniel-Constantin Mierla