Wireshark-bugs: [Wireshark-bugs] [Bug 7824] New: Failure to check return values from ws_fopen ca

Date: Tue, 9 Oct 2012 09:33:37 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7824

           Summary: Failure to check return values from ws_fopen calls
           Product: Wireshark
           Version: 1.8.2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: Major
          Priority: Low
         Component: Common utilities (libwsutil)
        AssignedTo: bugzilla-admin@xxxxxxxxxxxxx
        ReportedBy: wp02855@xxxxxxxxx


Created attachment 9319
  --> https://bugs.wireshark.org/bugzilla/attachment.cgi?id=9319
contains all the patch files (diff -u format) listed in the description section

Build Information:
odie:/usr/local/src/wireshark-1.8.2 # ./wireshark -v
wireshark 1.8.2 (SVN Rev Unknown from unknown)

Copyright 1998-2012 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 (64-bit) with GTK+ 2.22.1, with Cairo 1.10.2, with Pango 1.28.3, with
GLib 2.28.0, with libpcap, with libz 1.2.5, without POSIX capabilities, without
SMI, without c-ares, without ADNS, without Lua, without Python, with GnuTLS
2.8.6, with Gcrypt 1.4.6, with MIT Kerberos, without GeoIP, without PortAudio,
with AirPcap.

Running on Linux 2.6.37.6-0.7-desktop, with locale POSIX, with libpcap version
1.1.1, with libz 1.2.5, GnuTLS 2.8.6, Gcrypt 1.4.6, without AirPcap.

Built using gcc 4.5.1 20101208 [gcc-4_5-branch revision 167585].
--
In reviewing calls to ws_fopen(), I found instances where the
return value of ws_fopen is not checked for the NULL condition,
which indicates failure of ws_fopen() in wireshark-1.8.2/1.8.3

In directory epan, file 'diam_cict.l', the value of fh is
not checked for a NULL condition after a call to ws_fopen,
this patch adds in a sanity check to the code in question.

Patch is below and attached to this bug report:

odie:/usr/local/src/wireshark-1.8.2/epan # diff -u diam_dict.l.orig diam_dict.l
--- diam_dict.l.orig    2012-10-09 06:02:52.061547597 -0700
+++ diam_dict.l 2012-10-09 08:26:33.753653037 -0700
@@ -607,11 +607,15 @@

        fh = ws_fopen(fname,"r");

+       if (fh == NULL) {
+               D(("unable to open file: %s for reading, fh: %p\n", fname,
fh));
+               return NULL;
+       }
+
        D(("fname: %s fh: %p\n",fname,fh));

        g_free(fname);

-
        return fh;
 }

The same issue occurs in directory wireshark-1.8.2/plugins/wimaxasncp
file wimaxasncp_dict.c where 'fh' is assigned to a ws_fopen call, but
no check is made to determine if a NULL value is assigned to 'fh'
which would indicate failure.

this patch adds in a sanity check to the code in question.

Patch is below and attached to this bug report:

--- wimaxasncp_dict.c.orig      2012-10-09 07:38:10.679869428 -0700
+++ wimaxasncp_dict.c   2012-10-09 08:33:29.602598815 -0700
@@ -2921,6 +2921,10 @@
        }

        fh = ws_fopen(fname,"r");
+       if (fh == NULL) {
+               D(("unable to open file: %s for reading, fh: %p\n", fname,
fh));
+               return NULL;
+       }

        D(("fname: %s fh: %p\n",fname,fh));

The same issue occurs in directory wireshark-1.8.2/plugins/wimaxasncp
file wimaxasncp_dict.l where 'fh' is assigned to a ws_fopen call, but
no check is made to determine if a NULL value is assigned to 'fh'
which would indicate failure.

this patch adds in a sanity check to the code in question.

Patch is below and attached to this bug report:

--- wimaxasncp_dict.l.orig      2012-10-09 07:46:54.148186118 -0700
+++ wimaxasncp_dict.l   2012-10-09 08:34:57.868926566 -0700
@@ -594,6 +594,10 @@
        }

        fh = ws_fopen(fname,"r");
+       if (fh == NULL) {
+               D(("unable to open file: %s for reading, fh: %p\n", fname,
fh));
+               return NULL;
+       }

        D(("fname: %s fh: %p\n",fname,fh));

The same issue occurs in directory wireshark-1.8.2/plugins/wimaxasncp
file packet-asn1.c where 'namelist' is assigned to a ws_fopen call, but
no check is made to determine if a NULL value is assigned to 'namelist'
which would indicate failure.

this patch adds in a sanity check to the code in question.

Patch is below and attached to this bug report:

--- packet-asn1.c.orig  2012-10-09 07:54:40.436461125 -0700
+++ packet-asn1.c       2012-10-09 07:56:57.072900547 -0700
@@ -4248,7 +4248,11 @@
                                   G_TYPE_STRING, G_TYPE_STRING);

        namelist = ws_fopen("namelist.txt", "w");
-       build_tree_view(model, PDUtree, NULL);
+       if (namelist == NULL) 
+               fprintf(stderr, "unable to open file: namelist.txt for
writing!\n");
+       else
+               build_tree_view(model, PDUtree, NULL);
+
        fclose(namelist);
        namelist = 0;

The same issue occurs in directory wireshark-1.8.2/wiretap file
'k12.c' where 'dbg_out' is assigned to a ws_fopen call, but
no check is made to determine if a NULL value is assigned to 'dbg_out'
which would indicate failure.

This code also converts 'unsigned <variable>' to 'unsigned int <variable>'
which was submitted in a previous bug report.

this patch adds in a sanity check to the code in question.

Patch is below and attached to this bug report:

--- k12.c.orig  2012-10-08 16:45:19.188657926 -0700
+++ k12.c       2012-10-09 08:12:53.355959060 -0700
@@ -64,7 +64,7 @@
 FILE* dbg_out = NULL;
 char* env_file = NULL;

-static unsigned debug_level = 0;
+static unsigned int debug_level = 0;

 void k12_fprintf(char* fmt, ...) {
     va_list ap;
@@ -81,7 +81,7 @@
        fprintf(dbg_out,"\n"); \
 } } while(0)

-void k12_hexdump(guint level, gint64 offset, char* label, unsigned char* b,
unsigned len) {
+void k12_hexdump(guint level, gint64 offset, char* label, unsigned char* b,
unsigned int len) {
     static const char* c2t[] = {
        
"00","01","02","03","04","05","06","07","08","09","0a","0b","0c","0d","0e","0f",
        
"10","11","12","13","14","15","16","17","18","19","1a","1b","1c","1d","1e","1f",
@@ -100,7 +100,7 @@
        
"e0","e1","e2","e3","e4","e5","e6","e7","e8","e9","ea","eb","ec","ed","ee","ef",
        
"f0","f1","f2","f3","f4","f5","f6","f7","f8","f9","fa","fb","fc","fd","fe","ff"
     };
-    unsigned i;
+    unsigned int i;

     if (debug_level < level) return;

@@ -682,8 +682,14 @@
 #ifdef DEBUG_K12
     gchar* env_level = getenv("K12_DEBUG_LEVEL");
     env_file = getenv("K12_DEBUG_FILENAME");
-    if ( env_file ) dbg_out = ws_fopen(env_file,"w");
-    else dbg_out = stderr;
+    if ( env_file ) {
+       dbg_out = ws_fopen(env_file,"w");
+       if (dog_out == NULL) {
+               K12_DBG(1,("unable to open K12 DEBUG FILENAME for writing!"));
+               return -1;
+       }
+    else
+       dbg_out = stderr;
     if ( env_level ) debug_level = strtoul(env_level,NULL,10);
     K12_DBG(1,("k12_open: ENTER debug_level=%u",debug_level));
 #endif

All code compiles cleanly via 'make'.

List of files in tarball ws_fopen-patches.tar.gz:

odie:/usr/local/src/patchfiles/wireshark-1.8.2 # tar tvf
ws_fopen-patches.tar.gz 
-rw-r--r-- root/root      1596 2012-10-09 08:19 k12.c.patch
-rw-r--r-- root/root       448 2012-10-09 07:59 packet-asn1.c.patch
-rw-r--r-- root/root       352 2012-10-09 08:27 diam_dict.l.patch
-rw-r--r-- root/root       326 2012-10-09 08:35 wimaxasncp_dict.l.patch
-rw-r--r-- root/root       328 2012-10-09 08:36 wimaxasncp_dict.c.patch

Bill Parker

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.