Change in osmo-msc[master]: libmsc/db.c: fix storing SMS with empty TP-User-Data

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Vadim Yanitskiy gerrit-no-reply at lists.osmocom.org
Sun Apr 14 09:41:48 UTC 2019 

Vadim Yanitskiy has uploaded this change for review. ( https://gerrit.osmocom.org/13630


Change subject: libmsc/db.c: fix storing SMS with empty TP-User-Data
......................................................................

libmsc/db.c: fix storing SMS with empty TP-User-Data

Thanks to db_sms_test, it was discovered that storing an SMS with
empty TP-User-Data (TP-UDL=1) causes buffer overruns in libdbi
and it's SQLite3 driver (libdbdsqlite3):

  DDB NOTICE test_db_sms_store('Empty TP-UD'): ==7791== Invalid write of size 2
  ==7791==    at 0x857DC60: dbd_quote_binary (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
  ==7791==    by 0x5B2B321: dbi_conn_quote_binary_copy (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
  ==7791==    by 0x4073B1: db_sms_store (db.c:701)
  ==7791==    by 0x405BB5: test_db_sms_store (db_sms_test.c:310)
  ==7791==    by 0x405BB5: main (db_sms_test.c:546)
  ==7791==  Address 0x7ed1cf0 is 0 bytes after a block of size 0 alloc'd
  ==7791==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==7791==    by 0x857DC4B: dbd_quote_binary (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
  ==7791==    by 0x5B2B321: dbi_conn_quote_binary_copy (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
  ==7791==    by 0x4073B1: db_sms_store (db.c:701)
  ==7791==    by 0x405BB5: test_db_sms_store (db_sms_test.c:310)
  ==7791==    by 0x405BB5: main (db_sms_test.c:546)

  ...

  DDB NOTICE test_db_sms_get('Empty TP-UD'): ==8051== Invalid read of size 1
  ==8051==    at 0x5B30510: _dbd_decode_binary (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
  ==8051==    by 0x857D957: dbd_fetch_row (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
  ==8051==    by 0x5B2C86E: dbi_result_seek_row (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
  ==8051==    by 0x40828F: next_row (db.c:188)
  ==8051==    by 0x40828F: db_sms_get (db.c:805)
  ==8051==    by 0x406C29: test_db_sms_get (db_sms_test.c:390)
  ==8051==    by 0x405C14: main (db_sms_test.c:547)
  ==8051==  Address 0x8f74641 is 0 bytes after a block of size 1 alloc'd
  ==8051==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==8051==    by 0x5DBEB49: strdup (strdup.c:42)
  ==8051==    by 0x857D93C: dbd_fetch_row (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
  ==8051==    by 0x5B2C86E: dbi_result_seek_row (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
  ==8051==    by 0x40828F: next_row (db.c:188)
  ==8051==    by 0x40828F: db_sms_get (db.c:805)
  ==8051==    by 0x406C29: test_db_sms_get (db_sms_test.c:390)
  ==8051==    by 0x405C14: main (db_sms_test.c:547)
  ==8051==
  success, as expected
  DDB NOTICE verify_sms('Empty TP-UD'): user_data_len mismatch: E0 vs A3

Apparently, dbi_conn_quote_binary_copy() doesn't properly handle
zero-length input. Let's guard against this.

Observed with:

  - libdbi-dev 0.9.0-1
  - libdbd-sqlite3:amd64 0.9.0-2ubuntu2

Change-Id: If0b2bb557118c5f0e520a2e6c2816336f6028661
---
M src/libmsc/db.c
M tests/db_sms/db_sms_test.c
M tests/db_sms/db_sms_test.err
3 files changed, 14 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/30/13630/1

diff --git a/src/libmsc/db.c b/src/libmsc/db.c
index 423787b..3f9a9bd 100644
--- a/src/libmsc/db.c
+++ b/src/libmsc/db.c
@@ -692,14 +692,20 @@
 {
 	dbi_result result;
 	char *q_text, *q_daddr, *q_saddr;
-	unsigned char *q_udata;
+	unsigned char *q_udata = NULL;
 	time_t now, validity_timestamp;
 
 	dbi_conn_quote_string_copy(conn, (char *)sms->text, &q_text);
 	dbi_conn_quote_string_copy(conn, (char *)sms->dst.addr, &q_daddr);
 	dbi_conn_quote_string_copy(conn, (char *)sms->src.addr, &q_saddr);
-	dbi_conn_quote_binary_copy(conn, sms->user_data, sms->user_data_len,
-				   &q_udata);
+
+	/* Guard against zero-length input, as this may cause
+	 * buffer overruns in libdbi / libdbdsqlite3. */
+	if (sms->user_data_len > 0) {
+		dbi_conn_quote_binary_copy(conn, sms->user_data,
+					   sms->user_data_len,
+					   &q_udata);
+	}
 
 	now = time(NULL);
 	validity_timestamp = now + sms->validity_minutes * 60;
diff --git a/tests/db_sms/db_sms_test.c b/tests/db_sms/db_sms_test.c
index e78bb53..b4d782e 100644
--- a/tests/db_sms/db_sms_test.c
+++ b/tests/db_sms/db_sms_test.c
@@ -247,8 +247,6 @@
 		},
 		.ud = &sms_tp_ud_set[0],
 	},
-#if 0
-	/* FIXME: there is a bug that causes ASAN / Valgrind to complain */
 	{
 		.name = "Empty TP-UD",
 		.sms = {
@@ -267,7 +265,6 @@
 		},
 		.ud = NULL,
 	},
-#endif
 };
 
 static void prepare_sms_test_set(void)
diff --git a/tests/db_sms/db_sms_test.err b/tests/db_sms/db_sms_test.err
index 73dbd8e..e0a329d 100644
--- a/tests/db_sms/db_sms_test.err
+++ b/tests/db_sms/db_sms_test.err
@@ -11,6 +11,7 @@
 DDB NOTICE test_db_sms_store('Same MSISDN #1'): success, as expected
 DDB NOTICE test_db_sms_store('Same MSISDN #2'): success, as expected
 DDB NOTICE test_db_sms_store('Expired SMS'): success, as expected
+DDB NOTICE test_db_sms_store('Empty TP-UD'): success, as expected
 DDB INFO Testing db_sms_get()...
 DDB NOTICE test_db_sms_get('Regular MO SMS'): success, as expected
 DDB NOTICE verify_sms('Regular MO SMS'): match
@@ -32,6 +33,8 @@
 DDB NOTICE verify_sms('Same MSISDN #2'): match
 DDB NOTICE test_db_sms_get('Expired SMS'): success, as expected
 DDB NOTICE verify_sms('Expired SMS'): match
+DDB NOTICE test_db_sms_get('Empty TP-UD'): success, as expected
+DDB NOTICE verify_sms('Empty TP-UD'): match
 DDB INFO Testing db_sms_get_next_unsent() and db_sms_mark_delivered()...
 DDB NOTICE db_sms_get_next_unsent(#1): found
 DDB NOTICE verify_sms('Regular MO SMS'): match
@@ -67,4 +70,6 @@
 DDB NOTICE test_db_sms_get('Same MSISDN #2'): failure, as expected
 DDB NOTICE test_db_sms_get('Expired SMS'): unexpected result
 DDB NOTICE verify_sms('Expired SMS'): match
+DDB NOTICE test_db_sms_get('Empty TP-UD'): success, as expected
+DDB NOTICE verify_sms('Empty TP-UD'): match
 full talloc report on 'null_context' (total      0 bytes in   1 blocks)

-- 
To view, visit https://gerrit.osmocom.org/13630
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0b2bb557118c5f0e520a2e6c2816336f6028661
Gerrit-Change-Number: 13630
Gerrit-PatchSet: 1
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190414/27d6b147/attachment.htm>

More information about the gerrit-log mailing list