[PATCH] network: Fix use-after-free caused by Scan() in poor reception.

[PATCH] network: Fix use-after-free caused by Scan() in poor reception.

[John Ernberg]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

From: John Ernberg <john.ernberg@actia.se>

When issuing a Scan() in poor reception while attached to an operator it's
fully possible to get no results, which causes the attached operator to be
cleaned up. In certain scenarios this would cause a use-after-free.
Make sure to clean up all the references to the operator when it's destroyed.
---
 src/network.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/network.c b/src/network.c
index 1dddcac..5329c28 100644
--- a/src/network.c
+++ b/src/network.c
@@ -257,6 +257,9 @@ static void network_operator_destroy(gpointer user_data)
 {
 	struct network_operator_data *op = user_data;
 
+	if (op->netreg->current_operator == op)
+		op->netreg->current_operator = NULL;
+
 	g_free(op);
 }
 
-- 
1.9.1

Re: [PATCH] network: Fix use-after-free caused by Scan() in poor reception.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

Hi John,

On 12/21/2015 04:03 AM, John Ernberg wrote:
> From: John Ernberg <john.ernberg@actia.se>
>
> When issuing a Scan() in poor reception while attached to an operator it's
> fully possible to get no results, which causes the attached operator to be
> cleaned up. In certain scenarios this would cause a use-after-free.
> Make sure to clean up all the references to the operator when it's destroyed.
> ---
>   src/network.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/src/network.c b/src/network.c
> index 1dddcac..5329c28 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -257,6 +257,9 @@ static void network_operator_destroy(gpointer user_data)
>   {
>   	struct network_operator_data *op = user_data;
>
> +	if (op->netreg->current_operator == op)
> +		op->netreg->current_operator = NULL;
> +

I'm not sure this is the right fix.  This will result in subsequent API 
calls to return inconsistent information related to the network 
operator.  For example, NetworkRegistration.Name, 
NetworkRegistration.MobileNetworkCode, 
NetworkRegistration.MobileCountryCode will be omitted.

Can we make sure that the current operator is not destroyed / 
unregistered in this particular situation?

>   	g_free(op);
>   }
>
>

Regards,
-Denis

Re: [PATCH] network: Fix use-after-free caused by Scan() in poor reception.

[John Ernberg]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

Hi Dennis,

On 12/22/2015 01:10 AM, Denis Kenzior wrote:
> Hi John,
>
> On 12/21/2015 04:03 AM, John Ernberg wrote:
>> From: John Ernberg <john.ernberg@actia.se>
>>
>> When issuing a Scan() in poor reception while attached to an operator 
>> it's
>> fully possible to get no results, which causes the attached operator 
>> to be
>> cleaned up. In certain scenarios this would cause a use-after-free.
>> Make sure to clean up all the references to the operator when it's 
>> destroyed.
>> ---
>>   src/network.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/network.c b/src/network.c
>> index 1dddcac..5329c28 100644
>> --- a/src/network.c
>> +++ b/src/network.c
>> @@ -257,6 +257,9 @@ static void network_operator_destroy(gpointer 
>> user_data)
>>   {
>>       struct network_operator_data *op = user_data;
>>
>> +    if (op->netreg->current_operator == op)
>> +        op->netreg->current_operator = NULL;
>> +
>
> I'm not sure this is the right fix.  This will result in subsequent 
> API calls to return inconsistent information related to the network 
> operator.  For example, NetworkRegistration.Name, 
> NetworkRegistration.MobileNetworkCode, 
> NetworkRegistration.MobileCountryCode will be omitted.
>
> Can we make sure that the current operator is not destroyed / 
> unregistered in this particular situation?
It may be possible but I could not figure out a way to do that. So I did 
it like this to at least prevent the resulting SIGSEGV.
>
>>       g_free(op);
>>   }
>>
>>
>
> Regards,
> -Denis
Best regards // John Ernberg

Re: [PATCH] network: Fix use-after-free caused by Scan() in poor reception.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

Hi John,

 >> I'm not sure this is the right fix.  This will result in subsequent
>> API calls to return inconsistent information related to the network
>> operator.  For example, NetworkRegistration.Name,
>> NetworkRegistration.MobileNetworkCode,
>> NetworkRegistration.MobileCountryCode will be omitted.
>>
>> Can we make sure that the current operator is not destroyed /
>> unregistered in this particular situation?
> It may be possible but I could not figure out a way to do that. So I did
> it like this to at least prevent the resulting SIGSEGV.

Understood, but this fix is not really acceptable.

In update_operator_list(), the old operators (that weren't found during 
Scan()) are unregistered.

         for (o = netreg->operator_list; o; o = o->next)
                 network_operator_dbus_unregister(netreg, o->data);

One way of accomplishing the above, would be to detect that 
netreg->current_operator is being removed and handle that appropriately. 
  E.g. by moving it from the old list to the new list.

Regards,
-Denis