[PATCH v3 0/3] sim: validate IMS private identity

[PATCH v3 0/3] sim: validate IMS private identity

[Sergey Matyukevich]

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

Hello Denis and all,

These patches add UTF8 validation for IMS private identity. Without
this check ofono may crash on dbus assert when SIM properties are
reported via org.ofono.SimManager interface.

Regards,
Sergey

v2 -> v3

- support both null-terminated and non-null terminated TLV values
- move validation helper into simutil.c
- add unit tests

Sergey Matyukevich (3):
  simutil: add validate_utf8_tlv
  sim: validate IMS private identity
  unit: add validate_utf8_tlv tests

 src/sim.c           |  3 ++-
 src/simutil.c       | 14 ++++++++++++++
 src/simutil.h       |  1 +
 unit/test-simutil.c | 22 ++++++++++++++++++++++
 4 files changed, 39 insertions(+), 1 deletion(-)

[PATCH v3 1/3] simutil: add validate_utf8_tlv

[Sergey Matyukevich]

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

Add helper to validate if TLV value is a valid UTF8 string.
Note that both null-terminated and non null-terminated UTF8
strings are considered valid.
---
 src/simutil.c | 14 ++++++++++++++
 src/simutil.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/src/simutil.c b/src/simutil.c
index 4e0d3311..5808b14e 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -765,6 +765,20 @@ unsigned char *comprehension_tlv_builder_get_data(
 	return tlv + tag_size + len_size;
 }
 
+gboolean validate_utf8_tlv(const unsigned char *tlv)
+{
+	int len = tlv[1];
+
+	if (len == 0)
+		return FALSE;
+
+	/* support both null-termiated and non null-terminated UTF8 TLV value */
+	if (tlv[len + 1] == '\0')
+		len -= 1;
+
+	return g_utf8_validate_len((const char *)tlv + 2, len, NULL);
+}
+
 static char *sim_network_name_parse(const unsigned char *buffer, int length,
 					gboolean *add_ci)
 {
diff --git a/src/simutil.h b/src/simutil.h
index 14a39957..33b775a7 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -403,6 +403,7 @@ gboolean comprehension_tlv_builder_set_length(
 				unsigned int len);
 unsigned char *comprehension_tlv_builder_get_data(
 				struct comprehension_tlv_builder *builder);
+gboolean validate_utf8_tlv(const unsigned char *data);
 
 void ber_tlv_iter_init(struct ber_tlv_iter *iter, const unsigned char *pdu,
 			unsigned int len);

Re: [PATCH v3 1/3] simutil: add validate_utf8_tlv

[Denis Kenzior]

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

Hi Sergey,

On 1/16/21 1:21 PM, Sergey Matyukevich wrote:
> Add helper to validate if TLV value is a valid UTF8 string.
> Note that both null-terminated and non null-terminated UTF8
> strings are considered valid.
> ---
>   src/simutil.c | 14 ++++++++++++++
>   src/simutil.h |  1 +
>   2 files changed, 15 insertions(+)
> 

<snip>

> +gboolean validate_utf8_tlv(const unsigned char *tlv)
> +{
> +	int len = tlv[1];
> +
> +	if (len == 0)
> +		return FALSE;
> +
> +	/* support both null-termiated and non null-terminated UTF8 TLV value */

I tweaked this comment to fix the typo here and make fit to 80 chars

> +	if (tlv[len + 1] == '\0')
> +		len -= 1;
> +
> +	return g_utf8_validate_len((const char *)tlv + 2, len, NULL);
> +}
> +
>   static char *sim_network_name_parse(const unsigned char *buffer, int length,
>   					gboolean *add_ci)
>   {

Applied, thanks.

Regards,
-Denis

[PATCH v3 2/3] sim: validate IMS private identity

[Sergey Matyukevich]

[-- Attachment #1: Type: text/plain, Size: 709 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..793ff64a 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 (validate_utf8_tlv(data))
+		sim->impi = g_strndup((const char *)data + 2, data[1]);
 }
 
 static void discover_apps_cb(const struct ofono_error *error,

Re: [PATCH v3 2/3] sim: validate IMS private identity

[Denis Kenzior]

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

Hi Sergey,

On 1/16/21 1:21 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(-)
> 

Patch  2 and 3 applied, thanks.

Regards,
-Denis

[PATCH v3 3/3] unit: add validate_utf8_tlv tests

[Sergey Matyukevich]

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

---
 unit/test-simutil.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/unit/test-simutil.c b/unit/test-simutil.c
index 083bd4b2..6f8419b4 100644
--- a/unit/test-simutil.c
+++ b/unit/test-simutil.c
@@ -86,6 +86,27 @@ static void test_ber_tlv_iter(void)
 	test_buffer(valid_mms_params, sizeof(valid_mms_params));
 }
 
+static void test_validate_tlv(void)
+{
+	unsigned char impi_none[] = { 0x80, 0x0 };
+	unsigned char impi_empty[] = { 0x80, 0x1, '\0' };
+	unsigned char impi_term1[] = { 0x80, 0x3, 'F', 'O', 'O' };
+	unsigned char impi_term2[] = { 0x80, 0x4, 'F', 'O', 'O', '\0' };
+	unsigned char impi_term3[] = { 0x80, 0x3, 'F', 'O', 'O', 0xff, 0xff };
+	unsigned char impi_term4[] = { 0x80, 0x4, 'F', 'O', 'O', '\0', 0xff };
+	unsigned char impi_invalid1[] = { 0x80, 0x4, 'F', '\0', 'O', '\0' };
+	unsigned char impi_invalid2[] = { 0x80, 0x4, 0xff, 0xff, 0xff, 0xff };
+
+	g_assert(validate_utf8_tlv(impi_none) == FALSE);
+	g_assert(validate_utf8_tlv(impi_empty) == TRUE);
+	g_assert(validate_utf8_tlv(impi_term1) == TRUE);
+	g_assert(validate_utf8_tlv(impi_term2) == TRUE);
+	g_assert(validate_utf8_tlv(impi_term3) == TRUE);
+	g_assert(validate_utf8_tlv(impi_term4) == TRUE);
+	g_assert(validate_utf8_tlv(impi_invalid1) == FALSE);
+	g_assert(validate_utf8_tlv(impi_invalid2) == FALSE);
+}
+
 static void test_ber_tlv_builder_mms(void)
 {
 	struct ber_tlv_iter top_iter, nested_iter;
@@ -610,6 +631,7 @@ int main(int argc, char **argv)
 	g_test_init(&argc, &argv, NULL);
 
 	g_test_add_func("/testsimutil/ber tlv iter", test_ber_tlv_iter);
+	g_test_add_func("/testsimutil/ber tlv validate utf8", test_validate_tlv);
 	g_test_add_func("/testsimutil/ber tlv encode MMS",
 			test_ber_tlv_builder_mms);
 	g_test_add_func("/testsimutil/ber tlv encode EFpnn",