Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 2162] New: for() loop evaluation order in ep_strndup() can trigger valgrind "i

[bugzilla-daemon@xxxxxxxxxxxxx]

http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2162

           Summary: for() loop evaluation order in ep_strndup() can trigger
                    valgrind "invalid read of size 1"
           Product: Wireshark
           Version: SVN
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Minor
          Priority: Low
         Component: Wireshark
        AssignedTo: wireshark-bugs@xxxxxxxxxxxxx
        ReportedBy: jyoung@xxxxxxx


Build Information:
Version 0.99.8 (SVN Rev 23978)

Copyright 1998-2007 Gerald Combs <gerald@xxxxxxxxxxxxx> and contributors.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiled with GTK+ 2.8.3, with GLib 2.8.1, with libpcap 0.9.2, with libz 1.2.3,
without libpcre, without SMI, without ADNS, without Lua, with GnuTLS 1.2.5,
with
Gcrypt 1.2.1, without Kerberos, with PortAudio PortAudio V19-devel, without
AirPcap.
NOTE: this build doesn't support the "matches" operator for Wireshark filter
syntax.

Running on Linux 2.6.13-15.18-default, with libpcap version 0.9.2.

Built using gcc 4.0.2 20050901 (prerelease) (SUSE Linux).

Wireshark is Open Source Software released under the GNU General Public
License.

Check the man page and http://www.wireshark.org for more information.
--
Hello,

While using valgrind to track down another bug, I noticed that valgrind would
generate messages similar to the following whenever I had a non-empty
~./wireshark/snmp_users file.   

> <snip>
> ==00:00:01:31.386 19735== Invalid read of size 1
> ==00:00:01:31.386 19735==    at 0x445D672: ep_strndup (emem.c:515)
> ==00:00:01:31.387 19735==    by 0x449787C: uat_fld_chk_enum (uat.c:380)
> ==00:00:01:31.445 19735==    by 0x44A3313: uat_load_lex (uat_load.l:177)
> ==00:00:01:31.445 19735==    by 0x44A48A6: uat_load (uat_load.l:269)
> ==00:00:01:31.446 19735==    by 0x4497613: uat_load_all (uat.c:315)
> ==00:00:01:31.499 19735==    by 0x446AE57: init_prefs (prefs.c:1080)
> ==00:00:01:31.499 19735==    by 0x446B36F: read_prefs (prefs.c:1245)
> ==00:00:01:31.500 19735==    by 0x8087B69: main (main.c:2506)
> <snip>
> ==00:00:01:31.865 19735== Invalid read of size 1
> ==00:00:01:31.865 19735==    at 0x445D672: ep_strndup (emem.c:515)
> ==00:00:01:31.914 19735==    by 0x4B62319: snmp_users_auth_model_set_cb (packet-snmp-template.c:1824)
> ==00:00:01:31.915 19735==    by 0x44A33EF: uat_load_lex (uat_load.l:177)
> ==00:00:01:31.915 19735==    by 0x44A48A6: uat_load (uat_load.l:269)
> ==00:00:01:31.963 19735==    by 0x4497613: uat_load_all (uat.c:315)
> ==00:00:01:31.964 19735==    by 0x446AE57: init_prefs (prefs.c:1080)
> ==00:00:01:31.964 19735==    by 0x446B36F: read_prefs (prefs.c:1245)
> ==00:00:01:32.013 19735==    by 0x8087B69: main (main.c:2506)
> <snip>

These particulat valgrind messages occured when a character array of three
bytes was presented to the ep_strndup() function along with a length value of
3.  In this particular case, the 3 byte character array was NOT a legitimate
ASCII NUL terminated string. The current "copy" process within ep_strndup()
(and se_strndup()) is written as follows:

          for (i = 0; src[i] && i < len; i++)
                dst[i] = src[i];

Obviously the loop above copies one byte a time from src[i] to dst[i].  The
for() loop exits IF src[i] contains an ASCII NUL character or if i is equal to
or greater than len.    

Because the input data was NOT a legitimate ASCII NUL string but rather a three
byte character array, a test of src[4] for an ASCII NUL trigged the valgrind
error "invalid read of size 1".

I believe ep_strndup() and se_strndup() should be modified to change the order
of the tests within the for() loop as follows:   

          for (i = 0; (i < len) && src[i]; i++)
                dst[i] = src[i];

With the proposed change in place the for() loop will first test to see if "i <
len". If i is equal to or greather than len, then the for() loop will
immediately exit; this will pre-empt the src[i] test and avoid triggering the
valgrind() reported "invalid read of size 1" message.

Patch to be uploaded shortly.


-- 
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.