* [PATCH v2] sim: validate IMS private identity
@ 2021-01-15 19:56 Sergey Matyukevich
2021-01-15 20:10 ` Denis Kenzior
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Matyukevich @ 2021-01-15 19:56 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 740 bytes --]
Make sure that IMS private identity is a valid UTF8 string before
setting sim->impi field. Otherwise ofono may crash on dbus assert
when SIM properties are reported via org.ofono.SimManager interface.
---
src/sim.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/sim.c b/src/sim.c
index 33e1245f..2a663e2d 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int record,
return;
}
- sim->impi = g_strndup((const char *)data + 2, data[1]);
+ if (g_utf8_validate((const char *)data + 2, data[1], NULL))
+ sim->impi = g_strndup((const char *)data + 2, data[1]);
}
static void discover_apps_cb(const struct ofono_error *error,
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] sim: validate IMS private identity
2021-01-15 19:56 [PATCH v2] sim: validate IMS private identity Sergey Matyukevich
@ 2021-01-15 20:10 ` Denis Kenzior
2021-01-15 20:34 ` Sergey Matyukevich
0 siblings, 1 reply; 5+ messages in thread
From: Denis Kenzior @ 2021-01-15 20:10 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]
Hi Sergey,
On 1/15/21 1:56 PM, Sergey Matyukevich wrote:
> Make sure that IMS private identity is a valid UTF8 string before
> setting sim->impi field. Otherwise ofono may crash on dbus assert
> when SIM properties are reported via org.ofono.SimManager interface.
> ---
> src/sim.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/sim.c b/src/sim.c
> index 33e1245f..2a663e2d 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int record,
> return;
> }
>
> - sim->impi = g_strndup((const char *)data + 2, data[1]);
> + if (g_utf8_validate((const char *)data + 2, data[1], NULL))
> + sim->impi = g_strndup((const char *)data + 2, data[1]);
I assume this code path was tested with a file containing embedded NULs as that
is the only way it would have worked.
glib docs [1] say:
"Note that g_utf8_validate() returns FALSE if max_len is positive and any of the
max_len bytes are nul."
So I think the above logic would flag such a file as invalid, no?
> }
>
> static void discover_apps_cb(const struct ofono_error *error,
> _______________________________________________
> ofono mailing list -- ofono(a)ofono.org
> To unsubscribe send an email to ofono-leave(a)ofono.org
>
Regards,
-Denis
[1]
https://developer.gnome.org/glib/stable/glib-Unicode-Manipulation.html#g-utf8-validate
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] sim: validate IMS private identity
2021-01-15 20:10 ` Denis Kenzior
@ 2021-01-15 20:34 ` Sergey Matyukevich
2021-01-15 20:58 ` Denis Kenzior
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Matyukevich @ 2021-01-15 20:34 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]
> > Make sure that IMS private identity is a valid UTF8 string before
> > setting sim->impi field. Otherwise ofono may crash on dbus assert
> > when SIM properties are reported via org.ofono.SimManager interface.
> > ---
> > src/sim.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/sim.c b/src/sim.c
> > index 33e1245f..2a663e2d 100644
> > --- a/src/sim.c
> > +++ b/src/sim.c
> > @@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int record,
> > return;
> > }
> > - sim->impi = g_strndup((const char *)data + 2, data[1]);
> > + if (g_utf8_validate((const char *)data + 2, data[1], NULL))
> > + sim->impi = g_strndup((const char *)data + 2, data[1]);
>
> I assume this code path was tested with a file containing embedded NULs as
> that is the only way it would have worked.
>
> glib docs [1] say:
> "Note that g_utf8_validate() returns FALSE if max_len is positive and any of
> the max_len bytes are nul."
>
> So I think the above logic would flag such a file as invalid, no?
No, I tested using modem with attached SIM/eSIM. TLV data object appears
to be well-formed, but the contents is all padding 0xff bytes. Could you
please clarify your concern ? I assume we can not rely on the content
being properly null terminated string.
Regards,
Sergey
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] sim: validate IMS private identity
2021-01-15 20:34 ` Sergey Matyukevich
@ 2021-01-15 20:58 ` Denis Kenzior
2021-01-15 21:14 ` Sergey Matyukevich
0 siblings, 1 reply; 5+ messages in thread
From: Denis Kenzior @ 2021-01-15 20:58 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]
Hi Sergey,
On 1/15/21 2:34 PM, Sergey Matyukevich wrote:
>>> Make sure that IMS private identity is a valid UTF8 string before
>>> setting sim->impi field. Otherwise ofono may crash on dbus assert
>>> when SIM properties are reported via org.ofono.SimManager interface.
>>> ---
>>> src/sim.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/sim.c b/src/sim.c
>>> index 33e1245f..2a663e2d 100644
>>> --- a/src/sim.c
>>> +++ b/src/sim.c
>>> @@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int record,
>>> return;
>>> }
>>> - sim->impi = g_strndup((const char *)data + 2, data[1]);
>>> + if (g_utf8_validate((const char *)data + 2, data[1], NULL))
>>> + sim->impi = g_strndup((const char *)data + 2, data[1]);
>>
>> I assume this code path was tested with a file containing embedded NULs as
>> that is the only way it would have worked.
Ignore the last part of the above sentence. What I'm trying to say is that:
We in theory have two possibilities:
1. file with a string 'foo', no null:
0x80 0x03 'f' 'o' 'o'
2. file with a string 'foo' and null:
0x80 0x04 'f' 'o' 'o'
I suspect the spec really wants 1, but maybe it can be interpreted that 2 is
also a possibility?
The present logic should work for either of the above, but not what you have, i.e.:
0x80 0x03 0xff 0xff 0xff
>>
>> glib docs [1] say:
>> "Note that g_utf8_validate() returns FALSE if max_len is positive and any of
>> the max_len bytes are nul."
>>
>> So I think the above logic would flag such a file as invalid, no?
>
...but g_utf8_validate as invoked in this patch would flag possibility 2 as
invalid...
> No, I tested using modem with attached SIM/eSIM. TLV data object appears
> to be well-formed, but the contents is all padding 0xff bytes. Could you
> please clarify your concern ? I assume we can not rely on the content
> being properly null terminated string.
>
Yeah, for the case you have this logic works just fine.
Regards,
-Denis
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] sim: validate IMS private identity
2021-01-15 20:58 ` Denis Kenzior
@ 2021-01-15 21:14 ` Sergey Matyukevich
0 siblings, 0 replies; 5+ messages in thread
From: Sergey Matyukevich @ 2021-01-15 21:14 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]
Hi Denis,
> > > > Make sure that IMS private identity is a valid UTF8 string before
> > > > setting sim->impi field. Otherwise ofono may crash on dbus assert
> > > > when SIM properties are reported via org.ofono.SimManager interface.
> > > > ---
> > > > src/sim.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/sim.c b/src/sim.c
> > > > index 33e1245f..2a663e2d 100644
> > > > --- a/src/sim.c
> > > > +++ b/src/sim.c
> > > > @@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int record,
> > > > return;
> > > > }
> > > > - sim->impi = g_strndup((const char *)data + 2, data[1]);
> > > > + if (g_utf8_validate((const char *)data + 2, data[1], NULL))
> > > > + sim->impi = g_strndup((const char *)data + 2, data[1]);
> > >
> > > I assume this code path was tested with a file containing embedded NULs as
> > > that is the only way it would have worked.
>
> Ignore the last part of the above sentence. What I'm trying to say is that:
>
> We in theory have two possibilities:
>
> 1. file with a string 'foo', no null:
> 0x80 0x03 'f' 'o' 'o'
>
> 2. file with a string 'foo' and null:
> 0x80 0x04 'f' 'o' 'o'
>
> I suspect the spec really wants 1, but maybe it can be interpreted that 2 is
> also a possibility?
>
> The present logic should work for either of the above, but not what you have, i.e.:
>
> 0x80 0x03 0xff 0xff 0xff
>
> > >
> > > glib docs [1] say:
> > > "Note that g_utf8_validate() returns FALSE if max_len is positive and any of
> > > the max_len bytes are nul."
> > >
> > > So I think the above logic would flag such a file as invalid, no?
> >
>
> ...but g_utf8_validate as invoked in this patch would flag possibility 2 as
> invalid...
True. Thanks for detailed clarification. Indeed, both cases needs to be
supported. Let me double-check and come back with v3.
Regards,
Sergey
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-15 21:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15 19:56 [PATCH v2] sim: validate IMS private identity Sergey Matyukevich
2021-01-15 20:10 ` Denis Kenzior
2021-01-15 20:34 ` Sergey Matyukevich
2021-01-15 20:58 ` Denis Kenzior
2021-01-15 21:14 ` Sergey Matyukevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).