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.
On Mon, Aug 31, 2015 at 12:34 PM, Chris Double chris.double@double.co.nz wrote:
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.
I've done a minimal fix for the _reas_ case that we were hitting in this pull request:
https://github.com/kamailio/kamailio/pull/308
What are the thoughts on doing similar for the other cases?
For reference on the sr-dev mailing list -- a pull request was made and the discussion on this topic is continued at:
- https://github.com/kamailio/kamailio/pull/308
Cheers, Daniel
On 31/08/15 02:34, Chris Double wrote:
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.
-
Chris Double -
Daniel-Constantin Mierla