[PATCH 0/4] Fixes for memory leaks and corruptions

[PATCH 0/4] Fixes for memory leaks and corruptions

[Robert Tiemann]

I've noticed that the settings file gets corrupted when disabling wifi
tethering. Wrong strings and/or binary junk are written to the file,
preventing Connman from starting up the next time it is run.

I have tracked down the breaking commit (481d08f108) and was able to
patch the problem. While at it, I found and patched two more memory
leaks.

Robert Tiemann (4):
  wifi: Fix use-after-free when tethering is disabled.
  wifi: Fix memory leak.
  wifi: Fix indentation.
  technology: Fix memory leak.

 plugins/wifi.c   | 66 +++++++++++++++++++++++-------------------------
 src/technology.c |  4 ++-
 2 files changed, 34 insertions(+), 36 deletions(-)

--
2.34.1

[PATCH 1/4] wifi: Fix use-after-free when tethering is disabled.

[Robert Tiemann]

The bug frequently causes junk being written to the
Tethering.Identifier and Tethering.Passphrase entries in the settings
file. This in turn prevents Connman from starting up at all the next
time it is run.

To reproduce with connmanctl, use the following commands:

    enable wifi
    tether wifi on MyNetwork ThePassphrase
    tether wifi off

Then have a look at the settings file.

Disabling tethering causes a call of remove_ssid() from
interface_select_network_result() in gsupplicant/supplicant.c, which
frees the SSID and passphrase strings of the given GSupplicantSSID
structure (originally initialized by the wifi plugin). These strings,
however, are shared with a connman_technology structure, and they are
accessed in technology_save() (called indirectly from set_property()
in technology.c) after they have been freed.

To fix this bug, the affected strings are copied into the
GSupplicantSSID structure instead of simply assigning them. We make
sure (1) to free them in case supplicant didn't take the
GSupplicantSSID structure due to an error, and (2) to avoid double
frees in ap_create_callback() and sta_remove_callback().

Originally, there were two GSupplicantSSID instances in the wifi
plugin: one for the supplicant, and one stored in the wifi_data's
tethering_param structure. The latter, however, was never used, so
this commit removes its allocation to avoid a new memory leak, to save
memory, and to generally avoid confusion.

The bug fixed by this commit was introduced in 481d08f108.
---
 plugins/wifi.c | 54 ++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/plugins/wifi.c b/plugins/wifi.c
index ed7437f5..ba5aa2d7 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -2547,6 +2547,16 @@ static bool handle_sae_authentication_failure(struct connman_network *network,
 	return true;
 }

+static void wifi_data_free_tethering_info(struct wifi_data *wifi)
+{
+	if (!wifi->tethering_param)
+		return;
+
+	g_assert(wifi->tethering_param->ssid == NULL);
+	g_free(wifi->tethering_param);
+	wifi->tethering_param = NULL;
+}
+
 static void interface_state(GSupplicantInterface *interface)
 {
 	struct connman_network *network;
@@ -2568,11 +2578,7 @@ static void interface_state(GSupplicantInterface *interface)
 		return;

 	if (state == G_SUPPLICANT_STATE_COMPLETED) {
-		if (wifi->tethering_param) {
-			g_free(wifi->tethering_param->ssid);
-			g_free(wifi->tethering_param);
-			wifi->tethering_param = NULL;
-		}
+		wifi_data_free_tethering_info(wifi);

 		if (wifi->tethering)
 			stop_autoscan(device);
@@ -2832,9 +2838,7 @@ static void ap_create_fail(GSupplicantInterface *interface)
 			connman_technology_tethering_notify(wifi_technology,false);
 		}

-		g_free(wifi->tethering_param->ssid);
-		g_free(wifi->tethering_param);
-		wifi->tethering_param = NULL;
+		wifi_data_free_tethering_info(wifi);
 	}
 }

@@ -3387,7 +3391,7 @@ static GSupplicantSSID *ssid_ap_init(const struct connman_technology *technology
 		return NULL;

 	ap->mode = G_SUPPLICANT_MODE_MASTER;
-	ap->ssid = ssid;
+	ap->ssid = g_strdup(ssid);
 	ap->ssid_len = strlen(ssid);
 	ap->scan_ssid = 0;
 	if (freq)
@@ -3403,7 +3407,7 @@ static GSupplicantSSID *ssid_ap_init(const struct connman_technology *technology
 	       ap->protocol = G_SUPPLICANT_PROTO_RSN;
 	       ap->pairwise_cipher = G_SUPPLICANT_PAIRWISE_CCMP;
 	       ap->group_cipher = G_SUPPLICANT_GROUP_CCMP;
-	       ap->passphrase = passphrase;
+	       ap->passphrase = g_strdup(passphrase);
 	}

 	return ap;
@@ -3423,9 +3427,7 @@ static void ap_start_callback(int result, GSupplicantInterface *interface,

 		if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
 			connman_technology_tethering_notify(info->technology, false);
-			g_free(info->wifi->tethering_param->ssid);
-			g_free(info->wifi->tethering_param);
-			info->wifi->tethering_param = NULL;
+			wifi_data_free_tethering_info(info->wifi);
 		}
 	}

@@ -3448,14 +3450,10 @@ static void ap_create_callback(int result,

 		if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
 			connman_technology_tethering_notify(info->technology, false);
-			g_free(info->wifi->tethering_param->ssid);
-			g_free(info->wifi->tethering_param);
-			info->wifi->tethering_param = NULL;
-
+			wifi_data_free_tethering_info(info->wifi);
 		}

 		g_free(info->ifname);
-		g_free(info->ssid);
 		g_free(info);
 		return;
 	}
@@ -3483,14 +3481,10 @@ static void sta_remove_callback(int result,
 		info->wifi->tethering = false;
 		connman_technology_tethering_notify(info->technology, false);

-		if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
-			g_free(info->wifi->tethering_param->ssid);
-			g_free(info->wifi->tethering_param);
-			info->wifi->tethering_param = NULL;
-		}
+		if (info->wifi->ap_supported == WIFI_AP_SUPPORTED)
+			wifi_data_free_tethering_info(info->wifi);

 		g_free(info->ifname);
-		g_free(info->ssid);
 		g_free(info);
 		return;
 	}
@@ -3563,9 +3557,6 @@ static int enable_wifi_tethering(struct connman_technology *technology,
 		info->ifname = g_strdup(ifname);

 		wifi->tethering_param->technology = technology;
-		wifi->tethering_param->ssid = ssid_ap_init(technology);
-		if (!wifi->tethering_param->ssid)
-			goto failed;

 		info->wifi->tethering = true;
 		info->wifi->ap_supported = WIFI_AP_SUPPORTED;
@@ -3584,10 +3575,13 @@ static int enable_wifi_tethering(struct connman_technology *technology,

 	failed:
 		g_free(info->ifname);
-		g_free(info->ssid);
+		if (info->ssid) {
+			g_free((char *)info->ssid->ssid);
+			g_free((char *)info->ssid->passphrase);
+			g_free(info->ssid);
+		}
 		g_free(info);
-		g_free(wifi->tethering_param);
-		wifi->tethering_param = NULL;
+		wifi_data_free_tethering_info(wifi);

 		/*
 		 * Remove bridge if it was correctly created but remove
--
2.34.1

[PATCH 2/4] wifi: Fix memory leak.

[Robert Tiemann]

A GSupplicantSSID instance was leaked in ssid_ap_init() in case
connman_technology_get_wifi_tethering() didn't succeed.
---
 plugins/wifi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/plugins/wifi.c b/plugins/wifi.c
index ba5aa2d7..cc30d258 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -3387,8 +3387,10 @@ static GSupplicantSSID *ssid_ap_init(const struct connman_technology *technology
 	ret = connman_technology_get_wifi_tethering(technology,
 						&ssid, &passphrase,
 						&freq);
-	if (ret == false)
+	if (ret == false) {
+		g_free(ap);
 		return NULL;
+	}

 	ap->mode = G_SUPPLICANT_MODE_MASTER;
 	ap->ssid = g_strdup(ssid);
--
2.34.1

[PATCH 3/4] wifi: Fix indentation.

[Robert Tiemann]

---
 plugins/wifi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/plugins/wifi.c b/plugins/wifi.c
index cc30d258..12389fa6 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -3405,11 +3405,11 @@ static GSupplicantSSID *ssid_ap_init(const struct connman_technology *technology
 		ap->security = G_SUPPLICANT_SECURITY_NONE;
 		ap->passphrase = NULL;
 	} else {
-	       ap->security = G_SUPPLICANT_SECURITY_PSK;
-	       ap->protocol = G_SUPPLICANT_PROTO_RSN;
-	       ap->pairwise_cipher = G_SUPPLICANT_PAIRWISE_CCMP;
-	       ap->group_cipher = G_SUPPLICANT_GROUP_CCMP;
-	       ap->passphrase = g_strdup(passphrase);
+		ap->security = G_SUPPLICANT_SECURITY_PSK;
+		ap->protocol = G_SUPPLICANT_PROTO_RSN;
+		ap->pairwise_cipher = G_SUPPLICANT_PAIRWISE_CCMP;
+		ap->group_cipher = G_SUPPLICANT_GROUP_CCMP;
+		ap->passphrase = g_strdup(passphrase);
 	}

 	return ap;
--
2.34.1

[PATCH 4/4] technology: Fix memory leak.

[Robert Tiemann]

---
 src/technology.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/technology.c b/src/technology.c
index 65fb9854..270d83d0 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -479,8 +479,10 @@ static void technology_load(struct connman_technology *technology)

 	enc = g_key_file_get_string(keyfile,
 				identifier, "Tethering.Passphrase", NULL);
-	if (enc)
+	if (enc) {
 		technology->tethering_passphrase = g_strcompress(enc);
+		g_free(enc);
+	}

 	technology->tethering_freq = g_key_file_get_integer(keyfile,
 				identifier, "Tethering.Freq", NULL);
--
2.34.1