osmith
/
wireshark
forked from osmocom/wireshark
1
0
Fork 0
Code Pull Requests Releases Wiki Activity

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>
This commit is contained in:
Peter Wu 2016-09-11 01:16:24 +02:00 committed by Roland Knall
parent b82695d997
commit 583150198b
10 changed files with 87 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
extcap.c
View File

@ -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;

4
extcap.h
View File

@ -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
}

2
extcap_parser.h
View File

@ -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;

16
rawshark.c
View File

@ -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;
}

20
tfshark.c
View File

@ -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;

20
tshark.c
View File

@ -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;

6
ui/gtk/main.c
View File

@ -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

28
ui/qt/extcap_argument.cpp
View File

@ -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()

6
ui/qt/extcap_argument_file.cpp
View File

@ -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();

6
wireshark-qt.cpp
View File

@ -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