[PATCH] sim: validate IMS private identity

[PATCH] sim: validate IMS private identity

[Sergey Matyukevich]

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

Make sure that IMPI is a valid UTF8 string before attempting
to report it via DBus. Otherwise ofono may crash on dbus assert.
This field may not be defined for ISIM in use. In this case the
field still can be read from ISIM, though it will not contain
a valid UTF8 string. For instance, it may contain 255 0xFF bytes.
---
 src/sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sim.c b/src/sim.c
index 33e1245f..f60f5d1b 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -423,7 +423,7 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
 		ofono_dbus_dict_append(&dict, "ServiceProviderName",
 					DBUS_TYPE_STRING, &sim->spn);
 
-	if (sim->impi)
+	if (sim->impi && g_utf8_validate(sim->impi, 255, NULL))
 		ofono_dbus_dict_append(&dict, "ImsPrivateIdentity",
 					DBUS_TYPE_STRING, &sim->impi);
 

Re: [PATCH] sim: validate IMS private identity

[Denis Kenzior]

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

Hi Sergey,

On 1/15/21 10:38 AM, Sergey Matyukevich wrote:
> Make sure that IMPI is a valid UTF8 string before attempting
> to report it via DBus. Otherwise ofono may crash on dbus assert.
> This field may not be defined for ISIM in use. In this case the
> field still can be read from ISIM, though it will not contain
> a valid UTF8 string. For instance, it may contain 255 0xFF bytes.

Ugh, seems like the SIM vendor can't follow RFC's either?  31.103 Section 4.2.2 
says:

"For contents and syntax of NAI TLV data object values see IETF RFC 2486 [24]. 
The NAI shall be encoded to an octet string according to UTF-8 encoding rules as 
specified in IETF RFC 3629 [27]. The tag value of the NAI TLV data object shall 
be '80'. "

> ---
>   src/sim.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sim.c b/src/sim.c
> index 33e1245f..f60f5d1b 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -423,7 +423,7 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
>   		ofono_dbus_dict_append(&dict, "ServiceProviderName",
>   					DBUS_TYPE_STRING, &sim->spn);
>   
> -	if (sim->impi)
> +	if (sim->impi && g_utf8_validate(sim->impi, 255, NULL))

Hmm, this would imply that we're setting sim->impi incorrectly..  Also, since we 
have __ofono_sim_get_impi() API, the better fix would be to make sure sim->impi 
is set correctly in impi_read_cb()

>   		ofono_dbus_dict_append(&dict, "ImsPrivateIdentity",
>   					DBUS_TYPE_STRING, &sim->impi);
>   
> _______________________________________________
> ofono mailing list -- ofono(a)ofono.org
> To unsubscribe send an email to ofono-leave(a)ofono.org
> 

Regards,
-Denis

Re: [PATCH] sim: validate IMS private identity

[Sergey Matyukevich]

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

Hi Denis,

> > Make sure that IMPI is a valid UTF8 string before attempting
> > to report it via DBus. Otherwise ofono may crash on dbus assert.
> > This field may not be defined for ISIM in use. In this case the
> > field still can be read from ISIM, though it will not contain
> > a valid UTF8 string. For instance, it may contain 255 0xFF bytes.
> 
> Ugh, seems like the SIM vendor can't follow RFC's either?  31.103 Section
> 4.2.2 says:
> 
> "For contents and syntax of NAI TLV data object values see IETF RFC 2486
> [24]. The NAI shall be encoded to an octet string according to UTF-8
> encoding rules as specified in IETF RFC 3629 [27]. The tag value of the NAI
> TLV data object shall be '80'. "

This crash occured during my experiments with eSIM. As I mentioned, the
content of that TLV data object was 0xff bytes. IIUC it looks like vendor
could just skip initialization of that particular TLV data object during
bootstrap. Though I am not yet familiar with eSIM init procedure...

> > ---
> >   src/sim.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/sim.c b/src/sim.c
> > index 33e1245f..f60f5d1b 100644
> > --- a/src/sim.c
> > +++ b/src/sim.c
> > @@ -423,7 +423,7 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
> >   		ofono_dbus_dict_append(&dict, "ServiceProviderName",
> >   					DBUS_TYPE_STRING, &sim->spn);
> > -	if (sim->impi)
> > +	if (sim->impi && g_utf8_validate(sim->impi, 255, NULL))
> 
> Hmm, this would imply that we're setting sim->impi incorrectly..  Also,
> since we have __ofono_sim_get_impi() API, the better fix would be to make
> sure sim->impi is set correctly in impi_read_cb()

Ok. I will set sim->impi in impi_read_cb only if it is a valid UTF8 string.

Regards,
Sergey

Re: [PATCH] sim: validate IMS private identity

[Denis Kenzior]

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

Hi Sergey,

>>> This field may not be defined for ISIM in use. In this case the
>>> field still can be read from ISIM, though it will not contain
>>> a valid UTF8 string. For instance, it may contain 255 0xFF bytes.
>>
>> Ugh, seems like the SIM vendor can't follow RFC's either?  31.103 Section
>> 4.2.2 says:
>>
>> "For contents and syntax of NAI TLV data object values see IETF RFC 2486
>> [24]. The NAI shall be encoded to an octet string according to UTF-8
>> encoding rules as specified in IETF RFC 3629 [27]. The tag value of the NAI
>> TLV data object shall be '80'. "
> 
> This crash occured during my experiments with eSIM. As I mentioned, the
> content of that TLV data object was 0xff bytes. IIUC it looks like vendor
> could just skip initialization of that particular TLV data object during
> bootstrap. Though I am not yet familiar with eSIM init procedure...
> 

I figured.  The likely reason for this is that SIM strings are generally encoded 
in another format.  If you're interested, see src/util.c sim_string_to_utf8(). 
0xff is used as padding in such cases and thus a field with only 0xffs is 
treated as empty.

However, the  above would seem not to apply to EFimpi and a few others that 3GPP 
wants encoded as a utf-8 string.  So, likely, a bug on the SIM, but we should 
have sanitized the contents better.

Regards,
-Denis