[sr-dev] Buffer overflow in parse_hname2

[Daniel-Constantin Mierla]

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.
>

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Book: SIP Routing With Kamailio - http://www.asipto.com