From 583150198b78c84d043455b0afcca58a9659eab3 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Sun, 11 Sep 2016 01:16:24 +0200 Subject: [PATCH] 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 Tested-by: Petri Dish Buildbot Reviewed-by: Roland Knall --- extcap.c | 73 ++++++++++++++++++++-------------- extcap.h | 4 ++ extcap_parser.h | 2 +- rawshark.c | 16 ++------ tfshark.c | 20 +++------- tshark.c | 20 +++------- ui/gtk/main.c | 6 +-- ui/qt/extcap_argument.cpp | 28 ++++++++----- ui/qt/extcap_argument_file.cpp | 6 +-- wireshark-qt.cpp | 6 +-- 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