Re: possible memory error in the SnifferDecompress function?

[Lewis Burns <lewisurn () gmail com>]

Hi Guy,

Thanks for the tip. Another run with valgrind after using -g with gcc 
shows the line number. They point to the places that you suspected. I'll 
file a bug for both invalid write and memory overlap.

==17888== Memcheck, a memory error detector
==17888== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==17888== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==17888== Command: ./a.out
==17888==
==17888== Source and destination overlap in memcpy(0x51cc8a7, 0x51cc82b, 
146)
==17888==    at 0x4C2A690: memcpy (mc_replace_strmem.c:838)
==17888==    by 0x40098D: SnifferDecompress (ngsniffer_noklee.c:187)
==17888==    by 0x400B38: main (ngsniffer_noklee.c:250)
==17888==
==17888== Source and destination overlap in memcpy(0x51d752d, 0x51d7522, 13)
==17888==    at 0x4C2A690: memcpy (mc_replace_strmem.c:838)
==17888==    by 0x400A34: SnifferDecompress (ngsniffer_noklee.c:216)
==17888==    by 0x400B38: main (ngsniffer_noklee.c:250)
==17888==

Cheers,
Lewis

On 09/09/2014 05:09 PM, Guy Harris wrote:
> On Sep 9, 2014, at 3:11 PM, Lewis Burns <lewisurn () gmail com> wrote:
>
> > We've recently done some unit testing on open source projects. One of issues we've found is related to the SnifferDecompress 
> > function in the wiretap/ngsniffer.c file. We're unable to determine that the memory issues shown by valgrind can actually appear in 
> > the program due to our unfamiliarity with the code base. I'm sending in a small testcase to the list and hoping that some developers 
> > can validate or invalidate that this is a bug in the code.
> There's one place where the code isn't doing bounds checking, which probably accounts for
>
> > ==5795== Invalid write of size 1
> > ==5795==    at 0x400798: SnifferDecompress (in /home/chaoqiang/workspace/se/klee/exp/a.out)
> > ==5795==    by 0x400B6F: main (in /home/chaoqiang/workspace/se/klee/exp/a.out)
> > ==5795==  Address 0x521d080 is 0 bytes after a block of size 65,536 alloc'd
> > ==5795==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> > ==5795==    by 0x400AE8: main (in /home/chaoqiang/workspace/se/klee/exp/a.out)
> as, when I tried reproducing this, with a version of the test program built with -g, it reported
>
> ==55119== Invalid write of size 1
> ==55119==    at 0x100001618: SnifferDecompress (ngsniffer_noklee.c:90)
> ==55119==    by 0x100001E32: main (ngsniffer_noklee.c:250)
> ==55119==  Address 0x10003c150 is 0 bytes after a block of size 65,536 alloc'd
> ==55119==    at 0xC658: malloc (vg_replace_malloc.c:295)
> ==55119==    by 0x100001D83: main (ngsniffer_noklee.c:241)
>
> which is the "*(pout++) = *(pin++);" in
>
>                 /* Use the bits in bit_value to see what's encoded and what is raw data */
>                 if ( !(bit_mask & bit_value) )
>                 {
>                         /* bit not set - raw byte we just copy */
>                         *(pout++) = *(pin++);
>                 }
>
> which, unlike other code copying to the output buffer, doesn't do a bounds check.
>
> I couldn't reproduce the overlap complaint, but perhaps valgrind doesn't replace memcpy() on OS X or perhaps its replacement 
> memcpy() doesn't check for overlaps.  I *suspect* the overlap problems come from
>
>                         case 2  :   /* LZ77 long strings */
>                                 /*
>                                   Low 4 bits of offset to string is the low nybble of the
>                                   first code byte, upper 8 bits of offset is in
>                                   the next byte.
>                                   Length of string immediately follows.
>                                   Total code size: 3 bytes.
>                                 */
>                                 offset = code_low + ((unsigned int)(*pin++) << 4) + 3;
>                                 /* If we are already at end of input, there is no byte
>                                    to repeat */
>                                 if ( pin >= pin_end )
>                                 {
>                                         *err = WTAP_ERR_UNC_TRUNCATED;   /* data was oddly truncated */
>                                         return ( -1 );
>                                 }
>                                 /* Check if offset would put us back past begin of buffer */
>                                 if ( pout - offset < outbuf )
>                                 {
>                                         *err = WTAP_ERR_UNC_BAD_OFFSET;
>                                         return ( -1 );
>                                 }
>
>                                 /* get length from next byte, make sure it won't overrun buf */
>                                 length = (unsigned int)(*pin++) + 16;
>                                 if ( pout + length > pout_end )
>                                 {
>                                         *err = WTAP_ERR_UNC_OVERFLOW;
>                                         return ( -1 );
>                                 }
>
>                                 /* Copy the string from previous text to output position,
>                                    advance output pointer */
>                                 memcpy( pout, pout - offset, length );
>                                 pout += length;
>                                 break;
>                         default :   /* (3 to 15): LZ77 short strings */
>                                 /*
>                                   Low 4 bits of offset to string is the low nybble of the
>                                   first code byte, upper 8 bits of offset is in
>                                   the next byte.
>                                   Length of string to repeat is overloaded into code_type.
>                                   Total code size: 2 bytes.
>                                 */
>                                 offset = code_low + ((unsigned int)(*pin++) << 4) + 3;
>                                 /* Check if offset would put us back past begin of buffer */
>                                 if ( pout - offset < outbuf )
>                                 {
>                                         *err = WTAP_ERR_UNC_BAD_OFFSET;
>                                         return ( -1 );
>                                 }
>
>                                 /* get length from code_type, make sure it won't overrun buf */
>                                 length = code_type;
>                                 if ( pout + length > pout_end )
>                                 {
>                                         *err = WTAP_ERR_UNC_OVERFLOW;
>                                         return ( -1 );
>                                 }
>
>                                 /* Copy the string from previous text to output position,
>                                    advance output pointer */
>                                 memcpy( pout, pout - offset, length );
>                                 pout += length;
>                                 break;
>
> where it's *supposed* to be copying a string from earlier in the decompressed output (thus being before the current position 
> in the output, and thus not overlapping), but isn't checking to make sure that the source is fully within the 
> already-produced portion of the output.
>
> I'll check in fixes for both of those problems.
>
> Thanks.
>
> > Steps to repeat the issue:
> >   gcc -DRANDOM ngsniffer_noklee.c
> As per the above
>
>         gcc -g -DRANDOM ngsniffer_noklee.c
>
> may produce better debugging output from valgrind, with line numbers.
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>               mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe