Fix a regulatory domain use-after-free on device power-on.

Since fe9307c987a0 ("device: Setup regdom when powering up to maintain consistency"), powering on a Wi-Fi device reapplies the technology's stored regulatory domain by passing 'technology->regdom' itself down to wpa_supplicant. gsupplicant keeps that pointer, without copying it, until the asynchronous 'Country' property set completes, and then hands it back to the callback, which reaches 'connman_technology_regdom_notify'. That function frees 'technology->regdom' and then duplicates 'alpha2' from the string it has just freed.

On a 32-bit ARM device whose regulatory domain is set from the cellular network by the oFono plugin, connmand built with AddressSanitizer aborts on every Wi-Fi 'connmanctl disable' followed by 'connmanctl enable':

ERROR: AddressSanitizer: heap-use-after-free on address 0x75324210 READ of size 3 at 0x75324210 thread T0
0x75324210 is located 0 bytes inside of 3-byte region
freed by thread T0 here:
#1 connman_technology_regdom_notify src/technology.c:299 #2 __connman_technology_notify_regdom_by_device src/technology.c:1150 #3 connman_device_regdom_notify src/device.c:1101
#4 wifi_regdom_callback plugins/wifi.c:2060
#5 country_result gsupplicant/supplicant.c:3715
#6 property_set_reply gsupplicant/dbus.c:359

The read, from gdb at the abort, is the 'g_strdup' at src/technology.c:300, in the same call. The request that carried the pointer came from 'connman_device_set_powered', by way of 'connman_device_set_regdom', 'wifi_set_regdom' and 'g_supplicant_interface_set_country'.

Patch 1 is the fix: gsupplicant owns a copy of 'alpha2' for the life of each 'Country' set request. That also covers two related cases in which the string is freed by a call other than the one that reads it: a new regulatory domain arriving, through 'connman_technology_set_regdom', while a power-on request is outstanding; and the timezone module, which frees its 'alpha2' right after passing it to 'connman_technology_set_regdom'.

Patch 2 makes 'connman_technology_regdom_notify' safe when 'alpha2' is the technology's own 'regdom'. On its own, it stops the crash above but not those two cases, so it is defense-in-depth rather than an alternative to patch 1.

Grant Erickson (2):
gsupplicant: Own a copy of 'alpha2' for the life of a 'Country' set.
technology: Make 'connman_technology_regdom_notify' alias-safe.

 gsupplicant/supplicant.c | 9 ++++++---
 src/technology.c         | 9 ++++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

2.52.0

'g_supplicant_set_country' and 'g_supplicant_interface_set_country' stored the caller's 'alpha2' pointer, without copying it, in the 'struct supplicant_regdom' request context, and 'country_result' passed that same pointer back to the caller's callback when wpa_supplicant answered the asynchronous D-Bus 'Country' property set. The string therefore had to stay valid for the whole D-Bus round trip, and nothing ensured that it did.

Since fe9307c987a0 ("device: Setup regdom when powering up to maintain consistency"), 'connman_device_set_powered' passes the string returned by '__connman_technology_get_regdom' to 'connman_device_set_regdom'. That string is the technology's own 'regdom' (or 'global_regdom'), which 'connman_technology_regdom_notify' frees when the reply to that very request arrives:

    connman_device_set_powered
      connman_device_set_regdom(device, technology->regdom)
        wifi_set_regdom
          g_supplicant_interface_set_country      /* borrows it */

    country_result
      wifi_regdom_callback(result, technology->regdom, device)
        connman_device_regdom_notify
          __connman_technology_notify_regdom_by_device
            connman_technology_regdom_notify
              g_free(technology->regdom);
              technology->regdom = g_strdup(alpha2); /* reads it */

So, once a regulatory domain has been set, for example from the cellular network by the oFono plugin, connmand built with AddressSanitizer reports a heap-use-after-free and aborts each time a Wi-Fi device is powered on again. Built without it, connmand may store a garbage regulatory domain or corrupt its heap.

The borrowed string can also be freed by a call other than the one that reads it:

* 'connman_technology_set_regdom' replaces every technology's 'regdom', and 'global_regdom', synchronously. A new regulatory domain arriving while a power-on request is outstanding therefore frees the string that request's reply hands back.

* The timezone module, with 'RegdomFollowsTimezone' set, frees its 'alpha2' immediately after passing it to 'connman_technology_set_regdom', which can hand it to either function, through 'set_regdom_by_device' or the technology driver 'set_regdom' method.

Make the request context own a copy of 'alpha2' for its lifetime:
duplicate it when the request is made and free it when the request completes in 'country_result', or when it cannot be sent. The D-Bus message already carries its own copy of the string once it is built, so the callback was the only use of the pointer after the call returned. Every callback now receives a string that is valid for its duration, whatever the caller has done with its own string meanwhile.

---
 gsupplicant/supplicant.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 5e2899536d76..134147229be4 100644
@@ -3690,7 +3690,7 @@ void g_supplicant_interface_cancel(GSupplicantInterface *interface)
 
 struct supplicant_regdom {
 	GSupplicantCountryCallback callback;
-	const char *alpha2;
+	char *alpha2;
 	const void *user_data;
 };
 

@@ -3714,6 +3714,7 @@ static void country_result(const char *error,
 		regdom->callback(result, regdom->alpha2,
 					(void *) regdom->user_data);
 
+	g_free(regdom->alpha2);
 	g_free(regdom);
 }
 

@@ -3742,7 +3743,7 @@ int g_supplicant_set_country(const char *alpha2,
 		return -ENOMEM;
 
 	regdom->callback = callback;
-	regdom->alpha2 = alpha2;
+	regdom->alpha2 = g_strdup(alpha2);
 	regdom->user_data = user_data;
 
 	ret =  supplicant_dbus_property_set(SUPPLICANT_PATH, SUPPLICANT_INTERFACE,

@@ -3750,6 +3751,7 @@ int g_supplicant_set_country(const char *alpha2,
 					country_params, country_result,
 					regdom, NULL);
 	if (ret < 0) {
+		g_free(regdom->alpha2);
 		dbus_free(regdom);
 		SUPPLICANT_DBG("Unable to set Country configuration");
 	}

@@ -3769,7 +3771,7 @@ int g_supplicant_interface_set_country(GSupplicantInterface *interface,
 		return -ENOMEM;
 
 	regdom->callback = callback;
-	regdom->alpha2 = alpha2;
+	regdom->alpha2 = g_strdup(alpha2);
 	regdom->user_data = user_data;
 
 	ret =  supplicant_dbus_property_set(interface->path,

@@ -3778,6 +3780,7 @@ int g_supplicant_interface_set_country(GSupplicantInterface *interface,
 				country_params, country_result,
 					regdom, NULL);
 	if (ret < 0) {
+		g_free(regdom->alpha2);
 		dbus_free(regdom);
 		SUPPLICANT_DBG("Unable to set Country configuration");
 	}

2.52.0

'connman_technology_regdom_notify' freed 'technology->regdom' and then duplicated 'alpha2' into it. When 'alpha2' is 'technology->regdom' itself, the duplication reads the string just freed.

Since fe9307c987a0 ("device: Setup regdom when powering up to maintain consistency"), that is the argument it received, by way of the gsupplicant 'Country' set callback, whenever a Wi-Fi device was powered on again after its technology had stored a regulatory domain. With gsupplicant now handing its callbacks a copy of its own, that path no longer aliases, but nothing in the function's contract rules such a caller out.

Duplicate 'alpha2' before freeing the old value, so that the function is correct for any 'alpha2', including the technology's own.

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

index 933070f0578a..1fa97949d662 100644
@@ -289,6 +289,8 @@ static int set_tethering(struct connman_technology *technology,
 void connman_technology_regdom_notify(struct connman_technology *technology,
 							const char *alpha2)
 {
+	char *regdom;
+
 	DBG("");
 
 	if (!alpha2)

@@ -296,8 +298,13 @@ void connman_technology_regdom_notify(struct connman_technology *technology,
 	else
 		DBG("Regulatory domain set to %s", alpha2);
 
+	/*
+	 * alpha2 may be technology->regdom itself, so copy it before
+	 * freeing the old value.
+	 */
+	regdom = g_strdup(alpha2);
 	g_free(technology->regdom);
-	technology->regdom = g_strdup(alpha2);
+	technology->regdom = regdom;
 }
 
 static int set_regdom_by_device(struct connman_technology *technology,

2.52.0