[sr-dev] Buffer overflow in parse_hname2

Chris Double chris.double at double.co.nz
Mon Aug 31 02:34:46 CEST 2015


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



More information about the sr-dev mailing list