[PATCH 0/2] Fix a regulatory domain use-after-free on device power-on.

[PATCH 0/2] Fix a regulatory domain use-after-free on device power-on.

[Grant Erickson]

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

[PATCH 1/2] gsupplicant: Own a copy of 'alpha2' for the life of a 'Country' set.

[Grant Erickson]

'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.

Fixes: fe9307c987a0 ("device: Setup regdom when powering up to maintain consistency")
Fixes: bce73f1bdbf1 ("timezone: Change regdom along timezone, use localtime config")
---
 gsupplicant/supplicant.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 5e2899536d76..134147229be4 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -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

[PATCH 2/2] technology: Make 'connman_technology_regdom_notify' alias-safe.

[Grant Erickson]

'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.

Fixes: fe9307c987a0 ("device: Setup regdom when powering up to maintain consistency")
---
 src/technology.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/technology.c b/src/technology.c
index 933070f0578a..1fa97949d662 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -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