extcap: fix use-after-free for preferences

In commit v2.3.0rc0-117-g485bc45 (backported to v2.2.0rc0-44-g66721ca),
extcap_prefs_dynamic_vals and extcap_cleanup were added in an attempt to
address dangling pointers.

Unfortunately it is not sufficient:

 - A pointer to the preference value is stored in extcap_arg and passed
   to the prefs API, but this extcap_arg structure can become invalid
   which result in use-after-free whenever the preference is accessed.
 - On exit, a use-after-free occurs in prefs_cleanup when the preference
   value is being checked.

As the preference subsystem actually manages the memory for the string
value and consumers should only provide a pointer where the value can be
stored, convert the char* field in extcap to char**. This has as
additional benefit that values are not limited to 256 bytes anymore.

extcap_cleanup is moved after epan_cleanup to ensure that prefs_cleanup
does not operate on dangling pointers.

Crash is reproducible under ASAN with: tshark -i randpkt

Ping-Bug: 12183
Change-Id: Ibf1ba1102a5633aa085dc278a12ffc05a4f4a34b
Reviewed-on: https://code.wireshark.org/review/17631
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Roland Knall <rknall@gmail.com>

10 files changed, 87 insertions, 94 deletions

diff --git a/extcap.c b/extcap.c
index 6fbd0e15e6..948880de34 100644
--- a/extcap.c
+++ b/extcap.c
@@ -64,7 +64,6 @@
 static HANDLE pipe_h = NULL;
 #endif
 
-#define EXTCAP_PREF_SIZE 256
 static void extcap_child_watch_cb(GPid pid, gint status, gpointer user_data);
 
 /* internal container, for all the extcap interfaces that have been found.
@@ -80,9 +79,9 @@ static GHashTable *ifaces = NULL;
  */
 static GHashTable *tools = NULL;
 
-/* internal container, to map preferences for extcap utilities to dynamic
- * memory content, which survives extcap if garbage collection, and does
- * not lead to dangling pointers
+/* internal container, to map preference names to pointers that hold preference
+ * values. These ensure that preferences can survive extcap if garbage
+ * collection, and does not lead to dangling pointers in the prefs subsystem.
  */
 static GHashTable *extcap_prefs_dynamic_vals = NULL;
 
@@ -452,6 +451,10 @@ void extcap_register_preferences(void)
     }
 }
 
+/**
+ * Releases the dynamic preference value pointers. Must not be called before
+ * prefs_cleanup since these pointers could still be in use.
+ */
 void extcap_cleanup(void)
 {
     if (extcap_prefs_dynamic_vals) {
@@ -461,27 +464,34 @@ void extcap_cleanup(void)
 
 void extcap_pref_store(extcap_arg * arg, const char * newval)
 {
-    if (arg && arg->storeval != NULL)
+    if (arg && arg->pref_valptr != NULL)
     {
-        memset(arg->storeval, 0, EXTCAP_PREF_SIZE * sizeof(char));
-        if ( newval )
-            g_snprintf(arg->storeval, EXTCAP_PREF_SIZE, "%s", newval);
+        g_free(*arg->pref_valptr);
+        *arg->pref_valptr = g_strdup(newval);
     }
 
 }
 
-static gchar * extcap_prefs_dynamic_valptr(const char *name)
+/**
+ * Obtains a pointer which can store a value for the given preference name.
+ *
+ * Extcap interfaces (and their preferences) are dynamic, they can be created
+ * and destroyed at will. Thus their data structures are insufficient to pass to
+ * the preferences APIs which require pointers which are valid until the
+ * preferences are removed (at exit).
+ */
+static gchar ** extcap_prefs_dynamic_valptr(const char *name)
 {
-    gchar *valp;
+    gchar **valp;
     if (!extcap_prefs_dynamic_vals) {
         /* Initialize table only as needed, most preferences are not dynamic */
         extcap_prefs_dynamic_vals = g_hash_table_new_full(g_str_hash, g_str_equal,
                 g_free, g_free);
     }
-    valp = (gchar *)g_hash_table_lookup(extcap_prefs_dynamic_vals, name);
+    valp = (gchar **)g_hash_table_lookup(extcap_prefs_dynamic_vals, name);
     if (!valp) {
         /* New dynamic pref, allocate, initialize and store. */
-        valp = g_new0(gchar, EXTCAP_PREF_SIZE);
+        valp = g_new0(gchar *, 1);
         g_hash_table_insert(extcap_prefs_dynamic_vals, g_strdup(name), valp);
     }
     return valp;
@@ -563,21 +573,19 @@ static gboolean search_cb(const gchar *extcap _U_, const gchar *ifname _U_, gcha
                     gchar * pref_ifname = g_strconcat(ifname_lowercase, ".", pref_name, NULL);
 
                     if ( ( pref = prefs_find_preference(dev_module, pref_ifname) ) == NULL ) {
-                        /* Set an initial value */
-                        if ( ! arg->storeval && arg->default_complex )
-                        {
-                            arg->storeval = extcap_prefs_dynamic_valptr(pref_ifname);
-                            g_snprintf(arg->storeval, EXTCAP_PREF_SIZE, "%s", arg->default_complex->_val);
+                        arg->pref_valptr = extcap_prefs_dynamic_valptr(pref_ifname);
+                        /* Set an initial value if any (the string will be copied at registration) */
+                        if (arg->default_complex) {
+                            *arg->pref_valptr = arg->default_complex->_val;
                         }
 
                         prefs_register_string_preference(dev_module, g_strdup(pref_ifname),
-                                arg->display, arg->display, (const gchar **)&(arg->storeval));
+                                arg->display, arg->display, (const char **)arg->pref_valptr);
                     } else {
                         /* Been here before, restore stored value */
-                        if (! arg->storeval && pref->varp.string)
+                        if (! arg->pref_valptr && pref->varp.string)
                         {
-                            arg->storeval = extcap_prefs_dynamic_valptr(pref_ifname);
-                            g_snprintf(arg->storeval, EXTCAP_PREF_SIZE, "%s", *(pref->varp.string));
+                            arg->pref_valptr = pref->varp.string;
                         }
                     }
 
@@ -629,6 +637,13 @@ extcap_get_if_configuration(const char * ifname) {
     return ret;
 }
 
+/**
+ * If is_required is FALSE: returns TRUE if the extcap interface has
+ * configurable options.
+ * If is_required is TRUE: returns TRUE when the extcap interface has
+ * configurable options that required modification. (For example, when an
+ * argument is required but empty.)
+ */
 gboolean
 extcap_has_configuration(const char * ifname, gboolean is_required) {
     GList * arguments = 0;
@@ -648,11 +663,11 @@ extcap_has_configuration(const char * ifname, gboolean is_required) {
                 if ( ! is_required )
                     found = TRUE;
                 else if ( arg->is_required ) {
-                    gchar * stored = NULL;
-                    gchar * defval = NULL;
+                    const gchar * stored = NULL;
+                    const gchar * defval = NULL;
 
-                    if ( arg->storeval != NULL )
-                        stored = arg->storeval;
+                    if (arg->pref_valptr != NULL)
+                        stored = *arg->pref_valptr;
 
                     if ( arg->default_complex != NULL && arg->default_complex->_val != NULL )
                         defval = arg->default_complex->_val;
@@ -662,7 +677,7 @@ extcap_has_configuration(const char * ifname, gboolean is_required) {
                          * configuration is needed */
                         if ( defval && stored && g_strcmp0(stored, defval) == 0 )
                             found = TRUE;
-                        else if ( ! defval && (!stored || strlen(g_strchomp(stored)) <= (size_t)0) )
+                        else if ( ! defval && (!stored || !*stored) )
                             found = TRUE;
                     }
 
@@ -945,11 +960,11 @@ GPtrArray * extcap_prepare_arguments(interface_options interface_opts)
 
                 arg_list = g_list_first((GList *)elem->data);
                 while (arg_list != NULL) {
-                    gchar * stored = NULL, * defval = NULL;
+                    const gchar * stored = NULL, * defval = NULL;
                     /* In case of boolflags only first element in arg_list is relevant. */
                     arg_iter = (extcap_arg*) (arg_list->data);
-                    if ( arg_iter->storeval != NULL )
-                        stored = arg_iter->storeval;
+                    if (arg_iter->pref_valptr != NULL)
+                        stored = *arg_iter->pref_valptr;
 
                     if ( arg_iter->default_complex != NULL && arg_iter->default_complex->_val != NULL )
                         defval = arg_iter->default_complex->_val;
diff --git a/extcap.h b/extcap.h
index 5a78d214d8..dd39247dd0 100644
--- a/extcap.h
+++ b/extcap.h
@@ -113,7 +113,11 @@ void
 extcap_pref_store(struct _extcap_arg * arg, const char * newval);
 
 /* Clean up global extcap stuff on program exit */
+#ifdef HAVE_EXTCAP
 void extcap_cleanup(void);
+#else
+static inline void extcap_cleanup(void) {}
+#endif
 
 #ifdef __cplusplus
 }
diff --git a/extcap_parser.h b/extcap_parser.h
index 5f12037014..0f29625061 100644
--- a/extcap_parser.h
+++ b/extcap_parser.h
@@ -122,7 +122,7 @@ typedef struct _extcap_arg {
     extcap_complex *range_end;
     extcap_complex *default_complex;
 
-    gchar * storeval;
+    gchar ** pref_valptr; /**< A copy of the pointer containing the current preference value. */
     gchar * device_name;
 
     GList * values;
diff --git a/rawshark.c b/rawshark.c
index fe68b76f92..510ed80bcf 100644
--- a/rawshark.c
+++ b/rawshark.c
@@ -802,10 +802,8 @@ main(int argc, char *argv[])
                 cmdarg_err("%s", err_msg);
                 g_free(err_msg);
                 epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-                extcap_cleanup();
-#endif
                 epan_cleanup();
+                extcap_cleanup();
                 exit(2);
             }
             n_rfcodes++;
@@ -826,10 +824,8 @@ main(int argc, char *argv[])
 
         if (raw_cf_open(&cfile, pipe_name) != CF_OK) {
             epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-            extcap_cleanup();
-#endif
             epan_cleanup();
+            extcap_cleanup();
             exit(2);
         }
 
@@ -850,10 +846,8 @@ main(int argc, char *argv[])
         /* Process the packets in the file */
         if (!load_cap_file(&cfile)) {
             epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-            extcap_cleanup();
-#endif
             epan_cleanup();
+            extcap_cleanup();
             exit(2);
         }
     } else {
@@ -863,10 +857,8 @@ main(int argc, char *argv[])
     }
 
     epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-    extcap_cleanup();
-#endif
     epan_cleanup();
+    extcap_cleanup();
 
     return 0;
 }
diff --git a/tfshark.c b/tfshark.c
index bc7323eee6..d2d7db5621 100644
--- a/tfshark.c
+++ b/tfshark.c
@@ -866,10 +866,8 @@ main(int argc, char *argv[])
        * cruft getting in the way. Makes the results of running
        * $ ./tools/valgrind-wireshark -n
        * much more useful. */
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 0;
     case 'O':        /* Only output these protocols */
       /* already processed; just ignore it now */
@@ -996,10 +994,8 @@ main(int argc, char *argv[])
     if (!dfilter_compile(rfilter, &rfcode, &err_msg)) {
       cmdarg_err("%s", err_msg);
       g_free(err_msg);
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 2;
     }
   }
@@ -1009,10 +1005,8 @@ main(int argc, char *argv[])
     if (!dfilter_compile(dfilter, &dfcode, &err_msg)) {
       cmdarg_err("%s", err_msg);
       g_free(err_msg);
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 2;
     }
   }
@@ -1057,10 +1051,8 @@ main(int argc, char *argv[])
     /* TODO: if tfshark is ever changed to give the user a choice of which
        open_routine reader to use, then the following needs to change. */
     if (cf_open(&cfile, cf_name, WTAP_TYPE_AUTO, FALSE, &err) != CF_OK) {
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 2;
     }
 
@@ -1098,10 +1090,8 @@ main(int argc, char *argv[])
   draw_tap_listeners(TRUE);
   funnel_dump_all_text_windows();
   epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-  extcap_cleanup();
-#endif
   epan_cleanup();
+  extcap_cleanup();
 
   output_fields_free(output_fields);
   output_fields = NULL;
diff --git a/tshark.c b/tshark.c
index bb61320a0d..2e2ec5c76c 100644
--- a/tshark.c
+++ b/tshark.c
@@ -1310,10 +1310,8 @@ main(int argc, char *argv[])
        * cruft getting in the way. Makes the results of running
        * $ ./tools/valgrind-wireshark -n
        * much more useful. */
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 0;
     case 'O':        /* Only output these protocols */
       /* already processed; just ignore it now */
@@ -1732,10 +1730,8 @@ main(int argc, char *argv[])
     if (!dfilter_compile(rfilter, &rfcode, &err_msg)) {
       cmdarg_err("%s", err_msg);
       g_free(err_msg);
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
 #ifdef HAVE_PCAP_OPEN_DEAD
       {
         pcap_t *pc;
@@ -1761,10 +1757,8 @@ main(int argc, char *argv[])
     if (!dfilter_compile(dfilter, &dfcode, &err_msg)) {
       cmdarg_err("%s", err_msg);
       g_free(err_msg);
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
 #ifdef HAVE_PCAP_OPEN_DEAD
       {
         pcap_t *pc;
@@ -1875,10 +1869,8 @@ main(int argc, char *argv[])
      * We're reading a capture file.
      */
     if (cf_open(&cfile, cf_name, in_file_type, FALSE, &err) != CF_OK) {
-#ifdef HAVE_EXTCAP
-      extcap_cleanup();
-#endif
       epan_cleanup();
+      extcap_cleanup();
       return 2;
     }
 
@@ -2039,10 +2031,8 @@ main(int argc, char *argv[])
   draw_tap_listeners(TRUE);
   funnel_dump_all_text_windows();
   epan_free(cfile.epan);
-#ifdef HAVE_EXTCAP
-  extcap_cleanup();
-#endif
   epan_cleanup();
+  extcap_cleanup();
 
   output_fields_free(output_fields);
   output_fields = NULL;
diff --git a/ui/gtk/main.c b/ui/gtk/main.c
index 5c17338400..860e824fe2 100644
--- a/ui/gtk/main.c
+++ b/ui/gtk/main.c
@@ -2754,12 +2754,10 @@ main(int argc, char *argv[])
     gtk_iface_mon_stop();
 #endif
 
-#ifdef HAVE_EXTCAP
-    extcap_cleanup();
-#endif
-
     epan_cleanup();
 
+    extcap_cleanup();
+
     AirPDcapDestroyContext(&airpdcap_ctx);
 
 #ifdef HAVE_GTKOSXAPPLICATION
diff --git a/ui/qt/extcap_argument.cpp b/ui/qt/extcap_argument.cpp
index 5c2eff18c2..400bc3edae 100644
--- a/ui/qt/extcap_argument.cpp
+++ b/ui/qt/extcap_argument.cpp
@@ -61,7 +61,8 @@ QWidget * ExtArgSelector::createEditor(QWidget * parent)
 {
     int counter = 0;
     int selected = -1;
-    QString stored = _argument->storeval ? QString(_argument->storeval) : QString();
+    const char *prefval = _argument->pref_valptr ? *_argument->pref_valptr : NULL;
+    QString stored(prefval ? prefval : "");
 
     boxSelection = new QComboBox(parent);
 
@@ -73,9 +74,9 @@ QWidget * ExtArgSelector::createEditor(QWidget * parent)
         {
             boxSelection->addItem((*iter).value(), (*iter).call());
 
-            if ( ! _argument->storeval && (*iter).isDefault() )
+            if ( !prefval && (*iter).isDefault() )
                 selected = counter;
-            else if ( _argument->storeval && stored.compare((*iter).call()) == 0 )
+            else if ( prefval && stored.compare((*iter).call()) == 0 )
                 selected = counter;
 
             counter++;
@@ -227,11 +228,12 @@ QWidget * ExtArgBool::createEditor(QWidget * parent)
     if ( _argument->tooltip != NULL )
         boolBox->setToolTip(QString().fromUtf8(_argument->tooltip));
 
-    if ( _argument->storeval )
+    const char *prefval = _argument->pref_valptr ? *_argument->pref_valptr : NULL;
+    if ( prefval )
     {
         QRegExp regexp(EXTCAP_BOOLEAN_REGEX);
 
-        bool savedstate = ( regexp.indexIn(QString(_argument->storeval[0]), 0) != -1 );
+        bool savedstate = ( regexp.indexIn(QString(prefval[0]), 0) != -1 );
         if ( savedstate != state )
             state = savedstate;
     }
@@ -303,9 +305,9 @@ QWidget * ExtArgText::createEditor(QWidget * parent)
     QString storeValue;
     QString text = defaultValue();
 
-    if ( _argument->storeval )
+    if ( _argument->pref_valptr && *_argument->pref_valptr)
     {
-        QString storeValue = _argument->storeval;
+        QString storeValue(*_argument->pref_valptr);
 
         if ( storeValue.length() > 0 && storeValue.compare(text) != 0 )
             text = storeValue.trimmed();
@@ -367,9 +369,9 @@ QWidget * ExtArgNumber::createEditor(QWidget * parent)
     QString storeValue;
     QString text = defaultValue();
 
-    if ( _argument->storeval )
+    if ( _argument->pref_valptr && *_argument->pref_valptr)
     {
-        QString storeValue = _argument->storeval;
+        QString storeValue(*_argument->pref_valptr);
 
         if ( storeValue.length() > 0 && storeValue.compare(text) != 0 )
             text = storeValue;
@@ -596,8 +598,12 @@ QString ExtcapArgument::prefValue()
 
 void ExtcapArgument::resetValue()
 {
-    if (_argument && _argument->storeval)
-        memset(_argument->storeval, 0, 128 * sizeof(char));
+    // XXX consider using the preferences API which can store the default value
+    // and put that here instead of an empty value.
+    if (_argument->pref_valptr) {
+        g_free(*_argument->pref_valptr);
+        *_argument->pref_valptr = NULL;
+    }
 }
 
 bool ExtcapArgument::isValid()
diff --git a/ui/qt/extcap_argument_file.cpp b/ui/qt/extcap_argument_file.cpp
index df8da8caeb..5d7df3f5b5 100644
--- a/ui/qt/extcap_argument_file.cpp
+++ b/ui/qt/extcap_argument_file.cpp
@@ -55,7 +55,6 @@ ExtcapArgumentFileSelection::~ExtcapArgumentFileSelection()
 
 QWidget * ExtcapArgumentFileSelection::createEditor(QWidget * parent)
 {
-    QString storeval;
     QString text = defaultValue();
     QString buttonText(UTF8_HORIZONTAL_ELLIPSIS);
 
@@ -69,9 +68,10 @@ QWidget * ExtcapArgumentFileSelection::createEditor(QWidget * parent)
     textBox = new QLineEdit(text, parent);
     textBox->setReadOnly(true);
 
-    if ( _argument->storeval )
+    const char *prefval = _argument->pref_valptr ? *_argument->pref_valptr : NULL;
+    if (prefval)
     {
-        QString storeValue = _argument->storeval;
+        QString storeValue(prefval);
 
         if ( storeValue.length() > 0 && storeValue.compare(text) != 0 )
             text = storeValue.trimmed();
diff --git a/wireshark-qt.cpp b/wireshark-qt.cpp
index 8a1564af71..446bfa7cb3 100644
--- a/wireshark-qt.cpp
+++ b/wireshark-qt.cpp
@@ -853,12 +853,10 @@ int main(int argc, char *argv[])
 
     ret_val = wsApp->exec();
 
-#ifdef HAVE_EXTCAP
-    extcap_cleanup();
-#endif
-
     epan_cleanup();
 
+    extcap_cleanup();
+
     AirPDcapDestroyContext(&airpdcap_ctx);
 
 #ifdef _WIN32