From 583150198b78c84d043455b0afcca58a9659eab3 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Sun, 11 Sep 2016 01:16:24 +0200 Subject: 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 --- tfshark.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) (limited to 'tfshark.c') 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; -- cgit v1.2.1